From 651d70fe16fcd77f734bbf5e32eee79a5990b850 Mon Sep 17 00:00:00 2001 From: Vib-UX Date: Mon, 22 Aug 2022 12:08:15 +0530 Subject: [PATCH 1/2] bitcoindnotify: enables bitcoindnotify to use updated batch-clients. With this RegisterSpendNtfn() & RegisterConfirmationsNtfn() services will leverage the support of updated batchAPI for bitcoind making it lightning fast. --- chainntnfs/bitcoindnotify/bitcoind.go | 197 +++++++++++++++++++------- go.mod | 3 + go.sum | 4 +- 3 files changed, 153 insertions(+), 51 deletions(-) diff --git a/chainntnfs/bitcoindnotify/bitcoind.go b/chainntnfs/bitcoindnotify/bitcoind.go index 424291351a..4ced5a9afe 100644 --- a/chainntnfs/bitcoindnotify/bitcoind.go +++ b/chainntnfs/bitcoindnotify/bitcoind.go @@ -10,6 +10,7 @@ import ( "github.com/btcsuite/btcd/btcutil" "github.com/btcsuite/btcd/chaincfg" "github.com/btcsuite/btcd/chaincfg/chainhash" + "github.com/btcsuite/btcd/rpcclient" "github.com/btcsuite/btcd/txscript" "github.com/btcsuite/btcd/wire" "github.com/btcsuite/btcwallet/chain" @@ -525,8 +526,24 @@ func (b *BitcoindNotifier) confDetailsManually(confRequest chainntnfs.ConfReques heightHint, currentHeight uint32) (*chainntnfs.TxConfirmation, chainntnfs.TxConfStatus, error) { - // Begin scanning blocks at every height to determine where the - // transaction was included in. + // batchRequest will store the scan requests at every height to determine + // where the transaction was included in. + batchRequest := make( + map[uint32]rpcclient.FutureGetBlockHashResult, currentHeight-heightHint, + ) + + for height := currentHeight; height >= heightHint && height > 0; height-- { + batchRequest[height] = b.chainConn.GetBlockHashAsync(int64(height)) + } + + // Sending the bulk request using the updated batchClient. + err := b.chainConn.SendAsyncQueue() + if err != nil { + return nil, chainntnfs.TxNotFoundManually, err + } + + blockHashes := make([]*chainhash.Hash, 0, len(batchRequest)) + for height := currentHeight; height >= heightHint && height > 0; height-- { // Ensure we haven't been requested to shut down before // processing the next height. @@ -537,35 +554,67 @@ func (b *BitcoindNotifier) confDetailsManually(confRequest chainntnfs.ConfReques default: } - blockHash, err := b.chainConn.GetBlockHash(int64(height)) + // Receive the next block hash from the async queue. + blockHash, err := batchRequest[height].Receive() if err != nil { return nil, chainntnfs.TxNotFoundManually, - fmt.Errorf("unable to get hash from block "+ - "with height %d", height) + fmt.Errorf("unable to retrieve hash for block "+ + "with height %d: %v", height, err) } - block, err := b.GetBlock(blockHash) - if err != nil { + blockHashes = append(blockHashes, blockHash) + } + + // Now we fetch blocks in interval of 15 to avoid out of memory + // errors in case of fetching too many blocks with GetBlocksBatch(). + const batchSize = 15 + total := len(blockHashes) + + for i := 0; i < total; i += batchSize { + // Ensure we haven't been requested to shut down before + // processing next set of blocks. + select { + case <-b.quit: return nil, chainntnfs.TxNotFoundManually, - fmt.Errorf("unable to get block with hash "+ - "%v: %v", blockHash, err) + chainntnfs.ErrChainNotifierShuttingDown + default: } - // For every transaction in the block, check which one matches - // our request. If we find one that does, we can dispatch its - // confirmation details. - for txIndex, tx := range block.Transactions { - if !confRequest.MatchesTx(tx) { - continue - } + start := i + end := i + batchSize + + if end > total { + end = total + } + + blocks, err := b.chainConn.GetBlocksBatch( + blockHashes[start:end], + ) - return &chainntnfs.TxConfirmation{ - Tx: tx, - BlockHash: blockHash, - BlockHeight: height, - TxIndex: uint32(txIndex), - Block: block, - }, chainntnfs.TxFoundManually, nil + if err != nil { + return nil, chainntnfs.TxNotFoundManually, err + } + + // Note:- blockHashes are stored in reverse order + // currentHeight --> heightHint, so we maintain the same refs + // of currentHeight to return the correct BlockHeight. + height := int(currentHeight) - start + + for j := range blocks { + // For every transaction in the block, check which one + // matches our request. If we find one that does, we can + // dispatch its confirmation details. + for txIndex, tx := range blocks[j].Transactions { + if confRequest.MatchesTx(tx) { + return &chainntnfs.TxConfirmation{ + Tx: tx, + BlockHash: blockHashes[start+j], + BlockHeight: uint32(height - j), + TxIndex: uint32(txIndex), + Block: blocks[j], + }, chainntnfs.TxFoundManually, nil + } + } } } @@ -786,8 +835,24 @@ func (b *BitcoindNotifier) historicalSpendDetails( spendRequest chainntnfs.SpendRequest, startHeight, endHeight uint32) ( *chainntnfs.SpendDetail, error) { - // Begin scanning blocks at every height to determine if the outpoint - // was spent. + // batchRequest will store the scan requests at every height to determine + // if the output was spent. + batchRequest := make( + map[uint32]rpcclient.FutureGetBlockHashResult, endHeight-startHeight, + ) + + for height := endHeight; height >= startHeight && height > 0; height-- { + batchRequest[height] = b.chainConn.GetBlockHashAsync(int64(height)) + } + + // Sending the bulk request using updated batchClient. + err := b.chainConn.SendAsyncQueue() + if err != nil { + return nil, err + } + + blockHashes := make([]*chainhash.Hash, 0, len(batchRequest)) + for height := endHeight; height >= startHeight && height > 0; height-- { // Ensure we haven't been requested to shut down before // processing the next height. @@ -797,38 +862,72 @@ func (b *BitcoindNotifier) historicalSpendDetails( default: } - // First, we'll fetch the block for the current height. - blockHash, err := b.chainConn.GetBlockHash(int64(height)) + // Receive the next block hash from the async queue. + blockHash, err := batchRequest[height].Receive() if err != nil { return nil, fmt.Errorf("unable to retrieve hash for "+ "block with height %d: %v", height, err) } - block, err := b.GetBlock(blockHash) + + blockHashes = append(blockHashes, blockHash) + } + + // Now we fetch blocks in interval of 15 to avoid out of memory errors + // in case of fetching too many blocks with GetBlocksBatch(). + const batchSize = 15 + total := len(blockHashes) + + for i := 0; i < total; i += batchSize { + // Ensure we haven't been requested to shut down before + // processing next set of blocks. + select { + case <-b.quit: + return nil, chainntnfs.ErrChainNotifierShuttingDown + default: + } + + start := i + end := i + batchSize + + if end > total { + end = total + } + + blocks, err := b.chainConn.GetBlocksBatch( + blockHashes[start:end], + ) if err != nil { - return nil, fmt.Errorf("unable to retrieve block "+ - "with hash %v: %v", blockHash, err) + return nil, err } - // Then, we'll manually go over every input in every transaction - // in it and determine whether it spends the request in - // question. If we find one, we'll dispatch the spend details. - for _, tx := range block.Transactions { - matches, inputIdx, err := spendRequest.MatchesTx(tx) - if err != nil { - return nil, err - } - if !matches { - continue - } + // Note:- blockHashes are stored in reverse order + // endHeight --> startHeight, so we maintain the same refs + // of endHeight to return the correct SpendHeight. + height := int(endHeight) - start + + for j := range blocks { + // Now we'll manually go over every input in every + // transaction in it and determine whether it spends the + // request in question. If we find one, we'll dispatch + // the spend details. + for _, tx := range blocks[j].Transactions { + matches, inputIdx, err := spendRequest.MatchesTx(tx) + if err != nil { + return nil, err + } + if !matches { + continue + } - txHash := tx.TxHash() - return &chainntnfs.SpendDetail{ - SpentOutPoint: &tx.TxIn[inputIdx].PreviousOutPoint, - SpenderTxHash: &txHash, - SpendingTx: tx, - SpenderInputIndex: inputIdx, - SpendingHeight: int32(height), - }, nil + txHash := tx.TxHash() + return &chainntnfs.SpendDetail{ + SpentOutPoint: &tx.TxIn[inputIdx].PreviousOutPoint, + SpenderTxHash: &txHash, + SpendingTx: tx, + SpenderInputIndex: inputIdx, + SpendingHeight: int32(height - j), + }, nil + } } } diff --git a/go.mod b/go.mod index caf9e614d4..b03e979583 100644 --- a/go.mod +++ b/go.mod @@ -157,6 +157,9 @@ require ( sigs.k8s.io/yaml v1.2.0 // indirect ) +// This replace is to leverage added batch-json rpc +replace github.com/btcsuite/btcwallet => github.com/Vib-UX/btcwallet v0.14.1-0.20220821160837-7f29aa81832b + // This replace is for https://github.com/advisories/GHSA-w73w-5m7g-f7qc replace github.com/dgrijalva/jwt-go => github.com/golang-jwt/jwt v3.2.1+incompatible diff --git a/go.sum b/go.sum index 05950e5265..21a7bda4a7 100644 --- a/go.sum +++ b/go.sum @@ -43,6 +43,8 @@ github.com/NebulousLabs/fastrand v0.0.0-20181203155948-6fb6489aac4e/go.mod h1:Bd github.com/NebulousLabs/go-upnp v0.0.0-20180202185039-29b680b06c82 h1:MG93+PZYs9PyEsj/n5/haQu2gK0h4tUtSy9ejtMwWa0= github.com/NebulousLabs/go-upnp v0.0.0-20180202185039-29b680b06c82/go.mod h1:GbuBk21JqF+driLX3XtJYNZjGa45YDoa9IqCTzNSfEc= github.com/OneOfOne/xxhash v1.2.2/go.mod h1:HSdplMjZKSmBqAxg5vPj2TmRDmfkzw+cTzAElWljhcU= +github.com/Vib-UX/btcwallet v0.14.1-0.20220821160837-7f29aa81832b h1:I5eWrHvR05kLkQBHYe8BgU5DRwfMcAoU0PwG2frWZGc= +github.com/Vib-UX/btcwallet v0.14.1-0.20220821160837-7f29aa81832b/go.mod h1:7OFsQ8ypiRwmr67hE0z98uXgJgXGAihE79jCib9x6ag= github.com/Yawning/aez v0.0.0-20211027044916-e49e68abd344 h1:cDVUiFo+npB0ZASqnw4q90ylaVAbnYyx0JYqK4YcGok= github.com/Yawning/aez v0.0.0-20211027044916-e49e68abd344/go.mod h1:9pIqrY6SXNL8vjRQE5Hd/OL5GyK/9MrGUWs87z/eFfk= github.com/aead/chacha20 v0.0.0-20180709150244-8b13a72661da h1:KjTM2ks9d14ZYCvmHS9iAKVt9AyzRSqNU1qabPih5BY= @@ -97,8 +99,6 @@ github.com/btcsuite/btcd/chaincfg/chainhash v1.0.1/go.mod h1:7SFka0XMvUgj3hfZtyd github.com/btcsuite/btclog v0.0.0-20170628155309-84c8d2346e9f h1:bAs4lUbRJpnnkd9VhRV3jjAVU7DJVjMaK+IsvSeZvFo= github.com/btcsuite/btclog v0.0.0-20170628155309-84c8d2346e9f/go.mod h1:TdznJufoqS23FtqVCzL0ZqgP5MqXbb4fg/WgDys70nA= github.com/btcsuite/btcutil v0.0.0-20190425235716-9e5f4b9a998d/go.mod h1:+5NJ2+qvTyV9exUAL/rxXi3DcLg2Ts+ymUAY5y4NvMg= -github.com/btcsuite/btcwallet v0.15.1 h1:SKfh/l2Bgz9sJwHZvfiVbZ8Pl3N/8fFcWWXzsAPz9GU= -github.com/btcsuite/btcwallet v0.15.1/go.mod h1:7OFsQ8ypiRwmr67hE0z98uXgJgXGAihE79jCib9x6ag= github.com/btcsuite/btcwallet/wallet/txauthor v1.2.3 h1:M2yr5UlULvpqtxUqpMxTME/pA92Z9cpqeyvAFk9lAg0= github.com/btcsuite/btcwallet/wallet/txauthor v1.2.3/go.mod h1:T2xSiKGpUkSLCh68aF+FMXmKK9mFqNdHl9VaqOr+JjU= github.com/btcsuite/btcwallet/wallet/txrules v1.2.0 h1:BtEN5Empw62/RVnZ0VcJaVtVlBijnLlJY+dwjAye2Bg= From f08757a854f938ed8cbb038080653f3d579aae25 Mon Sep 17 00:00:00 2001 From: Vib-UX Date: Mon, 22 Aug 2022 12:12:00 +0530 Subject: [PATCH 2/2] bitcoindnotify: testcoverage for refactored bitcoindnotify. Testcase to ensure the updated historicalSpendDetails() and confDetailsManually() which leverages batchAPI for scanning the chain manually works as expected with enhanced performance. --- chainntnfs/bitcoindnotify/bitcoind_test.go | 100 ++++++++++++++++++++- 1 file changed, 97 insertions(+), 3 deletions(-) diff --git a/chainntnfs/bitcoindnotify/bitcoind_test.go b/chainntnfs/bitcoindnotify/bitcoind_test.go index 39a29e9b3b..223c80ec59 100644 --- a/chainntnfs/bitcoindnotify/bitcoind_test.go +++ b/chainntnfs/bitcoindnotify/bitcoind_test.go @@ -5,12 +5,14 @@ package bitcoindnotify import ( "bytes" + "io/ioutil" "testing" "time" "github.com/btcsuite/btcd/chaincfg/chainhash" "github.com/btcsuite/btcd/integration/rpctest" + "github.com/btcsuite/btcd/wire" "github.com/btcsuite/btcwallet/chain" "github.com/lightningnetwork/lnd/blockcache" "github.com/lightningnetwork/lnd/chainntnfs" @@ -243,13 +245,15 @@ func testHistoricalConfDetailsNoTxIndex(t *testing.T, rpcpolling bool) { // ensured above. outpoint, output, privKey := chainntnfs.CreateSpendableOutput(t, miner) spendTx := chainntnfs.CreateSpendTx(t, outpoint, output, privKey) + const noOfBlocks = 100 spendTxHash, err := miner.Client.SendRawTransaction(spendTx, true) require.NoError(t, err, "unable to broadcast tx") + broadcastHeight = syncNotifierWithMiner(t, notifier, miner) if err := chainntnfs.WaitForMempoolTx(miner, spendTxHash); err != nil { t.Fatalf("tx not relayed to miner: %v", err) } - if _, err := miner.Client.Generate(1); err != nil { - t.Fatalf("unable to generate block: %v", err) + if _, err := miner.Client.Generate(noOfBlocks); err != nil { + t.Fatalf("unable to generate blocks: %v", err) } // Ensure the notifier and miner are synced to the same height to ensure @@ -257,18 +261,108 @@ func testHistoricalConfDetailsNoTxIndex(t *testing.T, rpcpolling bool) { confReq, err := chainntnfs.NewConfRequest(&outpoint.Hash, output.PkScript) require.NoError(t, err, "unable to create conf request") currentHeight := syncNotifierWithMiner(t, notifier, miner) - _, txStatus, err = notifier.historicalConfDetails( + txConfirmation, txStatus, err := notifier.historicalConfDetails( confReq, uint32(broadcastHeight), uint32(currentHeight), ) require.NoError(t, err, "unable to retrieve historical conf details") + blockHash, err := notifier.chainConn.GetBlockHash(int64(broadcastHeight)) + require.NoError(t, err, "unable to get blockHash") // Since the backend node's txindex is disabled and the transaction has // confirmed, we should be able to find it by falling back to scanning // the chain manually. switch txStatus { case chainntnfs.TxFoundManually: + require.Equal( + t, txConfirmation.BlockHash, blockHash, "blockhash mismatch", + ) + require.Equal( + t, txConfirmation.BlockHeight, broadcastHeight, "height mismatch", + ) default: t.Fatal("should have found the transaction by manually " + "scanning the chain, but did not") } } + +// TestHistoricalSpendDetailsNoTxIndex ensures that we correctly retrieve +// historical spend details using the set of fallback methods when the +// backend node's txindex is disabled. +func TestHistoricalSpendDetailsNoTxIndex(t *testing.T) { + testHistoricalSpendDetailsNoTxIndex(t, true) + testHistoricalSpendDetailsNoTxIndex(t, false) +} + +func testHistoricalSpendDetailsNoTxIndex(t *testing.T, rpcPolling bool) { + miner, tearDown := chainntnfs.NewMiner(t, nil, true, 25) + defer tearDown() + + bitcoindConn, cleanUp := chainntnfs.NewBitcoindBackend( + t, miner.P2PAddress(), false, rpcPolling, + ) + defer cleanUp() + + hintCache := initHintCache(t) + blockCache := blockcache.NewBlockCache(10000) + + notifier := setUpNotifier( + t, bitcoindConn, hintCache, hintCache, blockCache, + ) + defer func() { + err := notifier.Stop() + require.NoError(t, err) + }() + + // Since the node has its txindex disabled, we fall back to scanning the + // chain manually. A outpoint unknown to the network should not be + // notified. + var unknownHash chainhash.Hash + copy(unknownHash[:], bytes.Repeat([]byte{0x10}, 32)) + invalidOutpoint := wire.NewOutPoint(&unknownHash, 0) + unknownSpendReq, err := chainntnfs.NewSpendRequest(invalidOutpoint, testScript) + require.NoError(t, err, "unable to create spend request") + broadcastHeight := syncNotifierWithMiner(t, notifier, miner) + spendDetails, errSpend := notifier.historicalSpendDetails( + unknownSpendReq, broadcastHeight, broadcastHeight, + ) + require.NoError(t, errSpend, "unable to retrieve historical spend details") + require.Equal(t, spendDetails, (*chainntnfs.SpendDetail)(nil)) + + // Now, we'll create a test transaction and attempt to retrieve its + // spending details. In order to fall back to manually scanning the + // chain, the transaction must be in the chain and not contain any + // unspent outputs. To ensure this, we'll create a transaction with only + // one output, which we will manually spend. The backend node's + // transaction index should also be disabled, which we've already + // ensured above. + outpoint, output, privKey := chainntnfs.CreateSpendableOutput(t, miner) + spendTx := chainntnfs.CreateSpendTx(t, outpoint, output, privKey) + const noOfBlocks = 100 + spendTxHash, err := miner.Client.SendRawTransaction(spendTx, true) + require.NoError(t, err, "unable to broadcast tx") + broadcastHeight = syncNotifierWithMiner(t, notifier, miner) + err = chainntnfs.WaitForMempoolTx(miner, spendTxHash) + require.NoError(t, err, "tx not relayed to miner") + _, err = miner.Client.Generate(noOfBlocks) + require.NoError(t, err, "unable to generate blocks") + + // Ensure the notifier and miner are synced to the same height to ensure + // we can find the transaction spend details when manually scanning the + // chain. + spendReq, err := chainntnfs.NewSpendRequest(outpoint, output.PkScript) + require.NoError(t, err, "unable to create conf request") + currentHeight := syncNotifierWithMiner(t, notifier, miner) + validSpendDetails, err := notifier.historicalSpendDetails( + spendReq, broadcastHeight, currentHeight, + ) + require.NoError(t, err, "unable to retrieve historical spend details") + + // Since the backend node's txindex is disabled and the transaction has + // confirmed, we should be able to find it by falling back to scanning + // the chain manually. + require.NotNil(t, validSpendDetails) + require.Equal(t, validSpendDetails.SpentOutPoint, outpoint) + require.Equal(t, validSpendDetails.SpenderTxHash, spendTxHash) + require.Equal(t, validSpendDetails.SpendingTx, spendTx) + require.Equal(t, validSpendDetails.SpendingHeight, int32(broadcastHeight+1)) +}