Updates Chainnotifier Service to leverage batchAPI support #6811

Vib-UX · 2022-08-09T18:15:14Z

Change Description

Fixes #5041 Dependency : #807 btcwallet
Related discussions and making : Vib-UX#1

Steps to Test

cd chainntnfs/bitcoindnotify/
go test -v -tags=dev

Pull Request Checklist

Testing

Your PR passes all CI checks.
Tests covering the positive and negative (error paths) are included.
Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
Commits follow the Ideal Git Commit Structure.
Any new logging statements use an appropriate subsystem and logging level.
There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

positiveblue

This PR looks really good. We will gain quite a lot of perf in some execution paths 👍, thank you for it Vib-UX

I left a few non-blocking comments, but I think we need to wait for btcwallet, which looks almost ready.

Needs rebase

P.S: you had me at support of updated batchAPI for bitcoind making it **lightning fast**

positiveblue · 2022-08-20T18:45:49Z

chainntnfs/bitcoindnotify/bitcoind.go

@@ -525,8 +526,31 @@ 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


I wonder if we should put the logic for fetching blocks in its own struct/interface. That way we would not have to copy twice the same logic structure in confDetailsManually and historicalSpendDetails. It would also abstract how it is done (in batches or one by one).

positiveblue · 2022-08-20T18:49:04Z

chainntnfs/bitcoindnotify/bitcoind.go

+
+	// 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


I checked in bitcoin core but I did not see if there is any limit for this. The idea is to set a reasonable limit so bitcoind does not run OOM? It is hard for me to see what is a good number here (5, 15 or 50 😕 )

I think the concern for OOM is in btcwallet itself if it needs to process and keep in memory more than 15 blocks at a time.

positiveblue · 2022-08-20T18:53:36Z

chainntnfs/bitcoindnotify/bitcoind.go

-				continue
-			}
+		// Note:- blockHashes are stored in reverse order
+		// endHeight --> startHeight, so we maintain the same refs


is this only because we are doing

blockHashes := make([]*chainhash.Hash, 0, len(batchRequest)) for height := endHeight; height >= startHeight && height > 0; height-- { // ... blockHashes = append(blockHashes, blockHash) }

If so we could directly put them in order (we know the size) or simply reverse the array before start using it.

that way it may be easier to follow

for i := 0; i < total; i += batchSize { //... start := i end := i + batchSize // ... height := int(endHeight) - start for j := range blocks { for _, tx := range blocks[j].Transactions { // ... return &chainntnfs.SpendDetail{ // ... SpendingHeight: int32(height - j), # this part is correct, but I needed to think about it twice }, nil } } }

I mean sure we can reverse the slice and traverse on it, changes would be in +ve sign that's all, something like:

height:= int(startHeight) + start
SpendingHeight: int32(height + j)

Yes, i just found it counter intuitive because we are counting backwards. It was just a small nit, nothing to be fixed 👍 .

SpendingHeight: int32(startHeight + start + j) vs SpendingHeight: int32(endHeight - start - j)

Again, probably a good idea to refactor this into a method so we have the same reverse loop logic only once in the code. Then the batch size of 15 could also be a parameter there.

positiveblue · 2022-08-20T18:59:40Z

go.mod

@@ -60,6 +60,9 @@ require (
 	gopkg.in/macaroon.v2 v2.0.0
 )

+// This replace is to leverage added batch-json rpc


The changes look good, we will have to wait for them to be merged in btcwallet before merging this one

With this RegisterSpendNtfn() & RegisterConfirmationsNtfn() services will leverage the support of updated batchAPI for bitcoind making it lightning fast.

Testcase to ensure the updated historicalSpendDetails() and confDetailsManually() which leverages batchAPI for scanning the chain manually works as expected with enhanced performance.

guggero

Thanks a lot for your work on this!
The test cases looks good and should also be able to show the speed up potential if we add benchmarks to them in the future (for example to determine a good value for the batch size).

Though I think the whole reverse loop logic should be extracted into btcwallet itself so we don't have the same, hard to understand logic in 3 places.

guggero · 2022-08-22T18:51:16Z

chainntnfs/bitcoindnotify/bitcoind.go

-	// transaction was included in.
+	// batchRequest will store the scan requests at every height to determine
+	// where the transaction was included in.
+	batchRequest := make(


nit: use plural as it's a bunch of requests?

guggero · 2022-08-22T18:56:21Z

chainntnfs/bitcoindnotify/bitcoind.go

+
+	// 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


I think the concern for OOM is in btcwallet itself if it needs to process and keep in memory more than 15 blocks at a time.

guggero · 2022-08-22T18:58:45Z

chainntnfs/bitcoindnotify/bitcoind.go

+	const batchSize = 15
+	total := len(blockHashes)
+
+	for i := 0; i < total; i += batchSize {


This whole logic is almost exactly the same as in BitcoindClient.FilterBlocksBatched. The only difference is that there we call blockFilterer.FilterBlock() and here we call confRequest.MatchesTx(). So this could be refactored into a more generic method on the BitcoindClient that just takes a callback cb func(block *wire.MsgBlock) bool.

guggero · 2022-08-22T19:03:48Z

chainntnfs/bitcoindnotify/bitcoind.go

-				continue
-			}
+		// Note:- blockHashes are stored in reverse order
+		// endHeight --> startHeight, so we maintain the same refs


Again, probably a good idea to refactor this into a method so we have the same reverse loop logic only once in the code. Then the batch size of 15 could also be a parameter there.

guggero · 2022-08-22T19:07:01Z

chainntnfs/bitcoindnotify/bitcoind_test.go

+	var unknownHash chainhash.Hash
+	copy(unknownHash[:], bytes.Repeat([]byte{0x10}, 32))
+	invalidOutpoint := wire.NewOutPoint(&unknownHash, 0)
+	unknownSpendReq, err := chainntnfs.NewSpendRequest(invalidOutpoint, testScript)


nit: the code in this whole commit is very "blocky" and hard to read. Can we group this into smaller logical pieces separated by empty lines? For example:

// 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) 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") const noOfBlocks = 100 _, 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))

guggero · 2022-08-22T19:08:41Z

chainntnfs/bitcoindnotify/bitcoind_test.go

@@ -243,32 +245,124 @@ 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


nit: move constant to directly above where it's used?

guggero · 2022-08-22T19:09:00Z

chainntnfs/bitcoindnotify/bitcoind_test.go

+		unknownSpendReq, broadcastHeight, broadcastHeight,
+	)
+	require.NoError(t, errSpend, "unable to retrieve historical spend details")
+	require.Equal(t, spendDetails, (*chainntnfs.SpendDetail)(nil))


nit: use require.Nil()?

lightninglabs-deploy · 2023-07-25T11:00:27Z

@Vib-UX, remember to re-request review from reviewers when ready

lightninglabs-deploy · 2023-07-28T13:45:58Z

Closing due to inactivity

lightninglabs-deploy · 2023-07-28T14:49:25Z

Closing due to inactivity

lightninglabs-deploy · 2023-07-28T15:52:40Z

Closing due to inactivity

Vib-UX force-pushed the BatchRPC branch from 4b9947c to f50a430 Compare August 9, 2022 20:04

Roasbeef added this to the v0.16.0 milestone Aug 9, 2022

Roasbeef added optimization json-rpc labels Aug 9, 2022

guggero self-requested a review August 15, 2022 16:24

positiveblue reviewed Aug 20, 2022

View reviewed changes

Vib-UX added 2 commits August 22, 2022 12:08

bitcoindnotify: enables bitcoindnotify to use updated batch-clients.

651d70f

With this RegisterSpendNtfn() & RegisterConfirmationsNtfn() services will leverage the support of updated batchAPI for bitcoind making it lightning fast.

bitcoindnotify: testcoverage for refactored bitcoindnotify.

f08757a

Testcase to ensure the updated historicalSpendDetails() and confDetailsManually() which leverages batchAPI for scanning the chain manually works as expected with enhanced performance.

Vib-UX force-pushed the BatchRPC branch from f50a430 to f08757a Compare August 22, 2022 06:42

guggero reviewed Aug 22, 2022

View reviewed changes

saubyk removed this from the v0.16.0 milestone Aug 27, 2022

ellemouton closed this Jul 28, 2023

Updates Chainnotifier Service to leverage batchAPI support #6811

Updates Chainnotifier Service to leverage batchAPI support #6811

Uh oh!

Conversation

Vib-UX commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Description

Steps to Test

Pull Request Checklist

Testing

Code Style and Documentation

Uh oh!

positiveblue left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lightninglabs-deploy commented Jul 25, 2023

Uh oh!

lightninglabs-deploy commented Jul 28, 2023

Uh oh!

lightninglabs-deploy commented Jul 28, 2023

Uh oh!

lightninglabs-deploy commented Jul 28, 2023

Uh oh!

Uh oh!

Vib-UX commented Aug 9, 2022 •

edited

Loading

positiveblue left a comment •

edited

Loading