From d8924d26ced4e18e2668d2701a7fa77e5c0904a6 Mon Sep 17 00:00:00 2001
From: yyforyongyu <yy2452@columbia.edu>
Date: Wed, 23 Aug 2023 06:43:16 +0800
Subject: [PATCH 1/4] chainfee: fix unit test for `WebAPIEstimator`
---
lnwallet/chainfee/estimator.go | 2 +-
lnwallet/chainfee/estimator_test.go | 154 ++++++++++++++++------------
2 files changed, 88 insertions(+), 68 deletions(-)
diff --git a/lnwallet/chainfee/estimator.go b/lnwallet/chainfee/estimator.go
index 93912d368..1b766fe80 100644
--- a/lnwallet/chainfee/estimator.go
+++ b/lnwallet/chainfee/estimator.go
@@ -610,7 +610,7 @@ func (w *WebAPIEstimator) EstimateFeePerKW(numBlocks uint32) (
// returned. We will log the error and return the fall back fee rate
// instead.
if err != nil {
- log.Errorf("unable to query estimator: %v", err)
+ log.Errorf("Unable to query estimator: %v", err)
}
// If the result is too low, then we'll clamp it to our current fee
diff --git a/lnwallet/chainfee/estimator_test.go b/lnwallet/chainfee/estimator_test.go
index f3ec4efc6..d40bb42d1 100644
--- a/lnwallet/chainfee/estimator_test.go
+++ b/lnwallet/chainfee/estimator_test.go
@@ -155,82 +155,81 @@ func TestSparseConfFeeSource(t *testing.T) {
// as expected.
func TestWebAPIFeeEstimator(t *testing.T) {
t.Parallel()
- feeFloor := uint32(FeePerKwFloor.FeePerKVByte())
- testFeeRate := feeFloor * 100
+
+ var (
+ minTarget uint32 = 2
+ maxTarget uint32 = 6
+
+ // Fee rates are in sat/kb.
+ minFeeRate uint32 = 2000 // 500 sat/kw
+ maxFeeRate uint32 = 4000 // 1000 sat/kw
+ )
testCases := []struct {
- name string
- target uint32
- apiEst uint32
- est uint32
- err string
+ name string
+ target uint32
+ expectedFeeRate uint32
+ expectedErr string
}{
{
- name: "target_below_min",
- target: 0,
- apiEst: 0,
- est: 0,
- err: "too low, minimum",
+ // When requested target is below minBlockTarget, an
+ // error is returned.
+ name: "target_below_min",
+ target: 0,
+ expectedFeeRate: 0,
+ expectedErr: "too low, minimum",
},
{
- name: "target_w_too-low_fee",
- target: 100,
- apiEst: 42,
- est: feeFloor,
- err: "",
+ // When requested target is larger than the max cached
+ // target, the fee rate of the max cached target is
+ // returned.
+ name: "target_w_too-low_fee",
+ target: maxTarget + 100,
+ expectedFeeRate: minFeeRate,
+ expectedErr: "",
},
{
- name: "API-omitted_target",
- target: 2,
- apiEst: 0,
- est: testFeeRate,
- err: "",
+ // When requested target is smaller than the min cahced
+ // target, the fee rate of the min cached target is
+ // returned.
+ name: "API-omitted_target",
+ target: minTarget - 1,
+ expectedFeeRate: maxFeeRate,
+ expectedErr: "",
},
{
- name: "valid_target",
- target: 20,
- apiEst: testFeeRate,
- est: testFeeRate,
- err: "",
- },
- {
- name: "valid_target_extrapolated_fee",
- target: 25,
- apiEst: 0,
- est: testFeeRate,
- err: "",
+ // When the target is found, return it.
+ name: "valid_target",
+ target: maxTarget,
+ expectedFeeRate: minFeeRate,
+ expectedErr: "",
},
}
// Construct mock fee source for the Estimator to pull fees from.
//
// This will create a `feeByBlockTarget` map with the following values,
- // - 20: testFeeRate
- // - 100: 42, which will be floored to feeFloor.
- testFees := make(map[uint32]uint32)
- for _, tc := range testCases {
- if tc.apiEst != 0 {
- testFees[tc.target] = tc.apiEst
- }
+ // - 2: 4000 sat/kb
+ // - 6: 2000 sat/kb.
+ feeRateResp := map[uint32]uint32{
+ minTarget: maxFeeRate,
+ maxTarget: minFeeRate,
}
feeSource := mockSparseConfFeeSource{
url: "https://www.github.com",
- fees: testFees,
+ fees: feeRateResp,
}
estimator := NewWebAPIEstimator(feeSource, false)
- // Test that requesting a fee when no fees have been cached fails.
+ // Test that requesting a fee when no fees have been cached won't fail.
feeRate, err := estimator.EstimateFeePerKW(5)
require.NoErrorf(t, err, "expected no error")
require.Equalf(t, FeePerKwFloor, feeRate, "expected fee rate floor "+
"returned when no cached fee rate found")
- if err := estimator.Start(); err != nil {
- t.Fatalf("unable to start fee estimator, got: %v", err)
- }
- defer estimator.Stop()
+ require.NoError(t, estimator.Start(), "unable to start fee estimator")
for _, tc := range testCases {
tc := tc
@@ -238,9 +237,9 @@ func TestWebAPIFeeEstimator(t *testing.T) {
est, err := estimator.EstimateFeePerKW(tc.target)
// Test an error case.
- if tc.err != "" {
+ if tc.expectedErr != "" {
require.Error(t, err, "expected error")
- require.ErrorContains(t, err, tc.err)
+ require.ErrorContains(t, err, tc.expectedErr)
return
}
@@ -249,57 +248,78 @@ func TestWebAPIFeeEstimator(t *testing.T) {
require.NoErrorf(t, err, "error from target %v",
tc.target)
- exp := SatPerKVByte(tc.est).FeePerKWeight()
+ exp := SatPerKVByte(tc.expectedFeeRate).FeePerKWeight()
require.Equalf(t, exp, est, "target %v failed, fee "+
"map is %v", tc.target, feeSource.fees)
})
}
+
+ // Stop the estimator when test ends.
+ require.NoError(t, estimator.Stop(), "unable to stop fee estimator")
}
// TestGetCachedFee checks that the fee caching logic works as expected.
func TestGetCachedFee(t *testing.T) {
- target := uint32(2)
- fee := uint32(100)
+ var (
+ minTarget uint32 = 2
+ maxTarget uint32 = 6
+
+ minFeeRate uint32 = 100
+ maxFeeRate uint32 = 1000
+ )
// Create a dummy estimator without WebAPIFeeSource.
estimator := NewWebAPIEstimator(nil, false)
// When the cache is empty, an error should be returned.
- cachedFee, err := estimator.getCachedFee(target)
+ cachedFee, err := estimator.getCachedFee(minTarget)
require.Zero(t, cachedFee)
require.ErrorIs(t, err, errEmptyCache)
- // Store a fee rate inside the cache.
- estimator.feeByBlockTarget[target] = fee
+ // Store a fee rate inside the cache. The cache map now looks like,
+ // {2: 1000, 6: 100}
+ estimator.feeByBlockTarget = map[uint32]uint32{
+ minTarget: maxFeeRate,
+ maxTarget: minFeeRate,
+ }
testCases := []struct {
name string
confTarget uint32
expectedFee uint32
- expectErr error
}{
{
// When the target is cached, return it.
name: "return cached fee",
- confTarget: target,
- expectedFee: fee,
- expectErr: nil,
+ confTarget: minTarget,
+ expectedFee: maxFeeRate,
},
{
// When the target is not cached, return the next
- // lowest target that's cached.
+ // lowest target that's cached. In this case,
+ // requesting fee rate for target 7 will give the
+ // result for target 6.
+ name: "return lowest cached fee",
+ confTarget: maxTarget + 1,
+ expectedFee: minFeeRate,
+ },
+ {
+ // When the target is not cached, return the next
+ // lowest target that's cached. In this case,
+ // requesting fee rate for target 5 will give the
+ // result for target 2.
name: "return next cached fee",
- confTarget: target + 1,
- expectedFee: fee,
- expectErr: nil,
+ confTarget: maxTarget - 1,
+ expectedFee: maxFeeRate,
},
{
// When the target is not cached, and the next lowest
// target is not cached, return the nearest fee rate.
+ // In this case, requesting fee rate for target 1 will
+ // give the result for target 2.
name: "return highest cached fee",
- confTarget: target - 1,
- expectedFee: fee,
- expectErr: nil,
+ confTarget: minTarget - 1,
+ expectedFee: maxFeeRate,
},
}
@@ -309,8 +329,8 @@ func TestGetCachedFee(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
cachedFee, err := estimator.getCachedFee(tc.confTarget)
+ require.NoError(t, err)
require.Equal(t, tc.expectedFee, cachedFee)
- require.ErrorIs(t, err, tc.expectErr)
})
}
}
From 19680340759703b75cad6c0f12ae6970f0165248 Mon Sep 17 00:00:00 2001
From: yyforyongyu <yy2452@columbia.edu>
Date: Thu, 24 Aug 2023 23:01:27 +0800
Subject: [PATCH 2/4] chainntnfs: fix `testHistoricalConfDetailsTxIndex`
Fix the following uint test flake,
```
--- FAIL: TestHistoricalConfDetailsTxIndex (0.00s)
--- FAIL: TestHistoricalConfDetailsTxIndex/rpc_polling_enabled (1.16s)
bitcoind_test.go:174: should have found the transaction within the mempool, but did not: TxNotFoundIndex
FAIL
```
---
chainntnfs/bitcoindnotify/bitcoind_test.go | 27 +++++++++++++---------
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/chainntnfs/bitcoindnotify/bitcoind_test.go b/chainntnfs/bitcoindnotify/bitcoind_test.go
index 124872de0..3eefe8e32 100644
--- a/chainntnfs/bitcoindnotify/bitcoind_test.go
+++ b/chainntnfs/bitcoindnotify/bitcoind_test.go
@@ -5,6 +5,7 @@ package bitcoindnotify
import (
"bytes"
+ "fmt"
"testing"
"time"
@@ -14,6 +15,7 @@ import (
"github.com/lightningnetwork/lnd/blockcache"
"github.com/lightningnetwork/lnd/chainntnfs"
"github.com/lightningnetwork/lnd/channeldb"
+ "github.com/lightningnetwork/lnd/lntest/wait"
"github.com/stretchr/testify/require"
)
@@ -162,18 +164,21 @@ func testHistoricalConfDetailsTxIndex(t *testing.T, rpcPolling bool) {
confReq, err := chainntnfs.NewConfRequest(txid, pkScript)
require.NoError(t, err, "unable to create conf request")
- // The transaction should be found in the mempool at this point.
- _, txStatus, err = notifier.historicalConfDetails(confReq, 0, 0)
- require.NoError(t, err, "unable to retrieve historical conf details")
+ // The transaction should be found in the mempool at this point. We use
+ // wait here to give miner some time to propagate the tx to our node.
+ err = wait.NoError(func() error {
+ // The call should return no error.
+ _, txStatus, err = notifier.historicalConfDetails(confReq, 0, 0)
+ require.NoError(t, err)
- // Since it has yet to be included in a block, it should have been found
- // within the mempool.
- switch txStatus {
- case chainntnfs.TxFoundMempool:
- default:
- t.Fatalf("should have found the transaction within the "+
- "mempool, but did not: %v", txStatus)
- }
+ if txStatus != chainntnfs.TxFoundMempool {
+ return fmt.Errorf("cannot the tx in mempool, status "+
+ "is: %v", txStatus)
+ }
+
+ return nil
+ }, wait.DefaultTimeout)
+ require.NoError(t, err, "timeout waitinfg for historicalConfDetails")
if _, err := miner.Client.Generate(1); err != nil {
t.Fatalf("unable to generate block: %v", err)
From a252cb4528cc53c1aacbaa9e2e13ed884b1d1c86 Mon Sep 17 00:00:00 2001
From: yyforyongyu <yy2452@columbia.edu>
Date: Mon, 4 Sep 2023 14:16:31 +0800
Subject: [PATCH 3/4] contractcourt: fix test flake in `TestTaprootBriefcase`
When the numTweaks is zero, we should return a nil instead of
initializing an empty map as we'd get the following error,
```
Diff:
--- Expected
+++ Actual
@@ -11007,4 +11007,3 @@
},
- BreachedHtlcTweaks: (contractcourt.htlcTapTweaks) {
- },
+ BreachedHtlcTweaks: (contractcourt.htlcTapTweaks) <nil>,
```
---
contractcourt/taproot_briefcase_test.go | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/contractcourt/taproot_briefcase_test.go b/contractcourt/taproot_briefcase_test.go
index 40c7b620f..38471ed74 100644
--- a/contractcourt/taproot_briefcase_test.go
+++ b/contractcourt/taproot_briefcase_test.go
@@ -29,8 +29,14 @@ func randResolverCtrlBlocks(t *testing.T) resolverCtrlBlocks {
func randHtlcTweaks(t *testing.T) htlcTapTweaks {
numTweaks := rand.Int() % 256
- tweaks := make(htlcTapTweaks, numTweaks)
+ // If the numTweaks happens to be zero, we return a nil to avoid
+ // initializing the map.
+ if numTweaks == 0 {
+ return nil
+ }
+
+ tweaks := make(htlcTapTweaks, numTweaks)
for i := 0; i < numTweaks; i++ {
var id resolverID
_, err := rand.Read(id[:])
From 50f2c277d763bf85ba6315b454f79839582dc207 Mon Sep 17 00:00:00 2001
From: yyforyongyu <yy2452@columbia.edu>
Date: Tue, 5 Sep 2023 07:43:33 +0800
Subject: [PATCH 4/4] aezeed: fix flake in `TestDecipherIncorrectMnemonic`
---
aezeed/cipherseed_test.go | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/aezeed/cipherseed_test.go b/aezeed/cipherseed_test.go
index 7547f25e9..158b69bcf 100644
--- a/aezeed/cipherseed_test.go
+++ b/aezeed/cipherseed_test.go
@@ -533,7 +533,17 @@ func TestDecipherIncorrectMnemonic(t *testing.T) {
// a checksum failure.
swapIndex1 := 9
swapIndex2 := 13
- mnemonic[swapIndex1], mnemonic[swapIndex2] = mnemonic[swapIndex2], mnemonic[swapIndex1]
+
+ mnemonic[swapIndex1], mnemonic[swapIndex2] =
+ mnemonic[swapIndex2], mnemonic[swapIndex1]
+
+ // If the words happen to be the same by pure chance, we'll try again
+ // with different indexes.
+ if mnemonic[swapIndex1] == mnemonic[swapIndex2] {
+ swapIndex1 = 3
+ mnemonic[swapIndex1], mnemonic[swapIndex2] =
+ mnemonic[swapIndex2], mnemonic[swapIndex1]
+ }
// If we attempt to decrypt now, we should get a checksum failure.
// If we attempt to map back to the original cipher seed now, then we