Adds Batch client connection and async RPC calls to the bitcoind client by Vib-UX · Pull Request #1 · Vib-UX/btcwallet

Vib-UX · 2022-04-13T20:34:14Z

PR serves in relation to fixing issue 5041

With this PR we enable batch client connection and async RPC calls to the bitcoind client providing testcoverages for the same. This helps us in batching the required requests needed for speeding up operations by reducing round trips.

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.

📝 Please see our Contribution Guidelines for further guidance.

guggero

Looks pretty good! Mostly style nits, functionality looks to there 👍

bhandras

Nice work @Vib-UX ⚡ One general comment I can add is to please add some more detail to your commit messages. In general a good commit message is similar to what's describe in this doc:https://github.com/lightningnetwork/lnd/blob/master/docs/code_contribution_guidelines.md#model-git-commit-messages

Obviously it's not a requirement, but a good practice. At a minimum add some short description about what your commit changes, eg. packagename: adding a new shiny func to achieve some goals.

Looking forward for some test coverage as well :)

bhandras

Great work! Getting very close. RescanBlocks() needs some more fixing but other than that almost good to go.

bhandras

LGTM, just a few nits remain, great work @Vib-UX 🎉

bhandras · 2022-08-02T08:30:44Z

+// TestBitcoindRescanBlocks ensures that we correctly retrieve the required details
+// for the watched txn.
+func TestBitcoindRescanBlocks(t *testing.T) {
+	testBitcoindRescanBlocks(t, true)


nit: can create a subtest here with:

t.Run(fmt.Sprintf("rpcpoll=true"), func(t *testing.T) { ... }) etc

bhandras · 2022-08-02T08:51:03Z

Actually one last minute comment that maybe needs more work: it is a bit unfortunate that we need to add back old code in order to test expectations, perhaps a better way would be to instead check the output of the new batched call since we know what the output should look like. Otherwise there'll always remain some unused code as part of the public interface that we'll never use.

Alternatively you can also rename the "Old" functions as "Sync", meaning they're non batched, slow sequential calls. Also what do you think @guggero ?

Vib-UX · 2022-08-04T18:31:36Z

Actually one last minute comment that maybe needs more work: it is a bit unfortunate that we need to add back old code in order to test expectations, perhaps a better way would be to instead check the output of the new batched call since we know what the output should look like. Otherwise there'll always remain some unused code as part of the public interface that we'll never use.

Alternatively you can also rename the "Old" functions as "Sync", meaning they're non batched, slow sequential calls. Also what do you think @guggero ?

The updated testcoverage removes dependency from the previous Sync functions making it redundant. It was earlier there to compare speedup.

guggero · 2022-08-05T08:29:09Z

Alternatively you can also rename the "Old" functions as "Sync", meaning they're non batched, slow sequential calls. Also what do you think @guggero ?

Just jumping in to answer the question, I'm probably lacking most of the context of the previous discussion. But I'd generally not rename any existing functions, so other users (lnd isn't the only software that uses btcwallet AFAIK) don't need to change any code when updating. And using the same name as before for something that works differently is not a good idea either. So I'd keep everything existing as it is and add *Async methods for new stuff (which I believe is what's currently being done?).

bhandras · 2022-08-05T12:12:52Z

Alternatively you can also rename the "Old" functions as "Sync", meaning they're non batched, slow sequential calls. Also what do you think @guggero ?

Just jumping in to answer the question, I'm probably lacking most of the context of the previous discussion. But I'd generally not rename any existing functions, so other users (lnd isn't the only software that uses btcwallet AFAIK) don't need to change any code when updating. And using the same name as before for something that works differently is not a good idea either. So I'd keep everything existing as it is and add *Async methods for new stuff (which I believe is what's currently being done?).

Yeah maybe best to keep existing functionality and unrenamed. However since the new functionality is not exactly the same semantics as when using the "Async" versions of the RPC API, we could name them "Batched" perhaps to refer that it is doing batched calls under the hood.

Vib-UX · 2022-08-06T15:54:15Z

rebase master : a6ba11e to 36bcc1e

bhandras

LGTM, just a few nits remain, great work! 🎉 Make sure to add a detailed description when you open the pull request against the main repo! 🚀

bhandras · 2022-08-08T14:45:17Z

+func (c *BitcoindConn) GetBlocksBatch(
+	hashes []*chainhash.Hash) ([]*wire.MsgBlock, error) {
+
+	batchRequest := make(


nit: batchRequest could actually just be a slice, no need to key it with the hashes since we always iterate on hashes.

bhandras · 2022-08-08T14:59:23Z

+
+	// requests map will store rpcclient.FutureGetBlockHeaderVerboseResult
+	// for each blockHash.
+	requests := make(


nit: i think this also could just be an array since we always iterate on blockHashes.

bhandras · 2022-08-08T14:59:39Z

+// a FilterBlocksReponse for the first block containing a matching address.
+// If no matches are found in the range of blocks requested, the returned
+// response will be nil.
+


nit: extra nl

bhandras · 2022-08-08T15:14:44Z

+		require.NoError(t, err)
+		msgBlocksStandard = append(msgBlocksStandard, msgBlock)
+	}
+	msgBlocksBatchClient, err := bitcoindClient.GetBlocksBatch(blockHashes)


nit: add an extra nl after the for block

bhandras · 2022-08-08T15:16:21Z

+	// linearly with GetBlock() (store the result for each GetBlock() in a
+	// slice) further also try to retrieve blocks in batch with GetBlocksBatch().
+	// At last will compare the results from both which should be equal.
+	if _, err := miner.Client.Generate(5); err != nil {


nit: I think it'd be better to generate something like 2x15 + some blocks to test that we do create multiple batches correctly.

guggero · 2022-08-08T15:22:19Z

I think it makes sense to open the PR against the main repo now. I'll happily do a review there. Is that okay for everyone?

bhandras · 2022-08-08T15:31:02Z

I think it makes sense to open the PR against the main repo now. I'll happily do a review there. Is that okay for everyone?

Yeah, sure. Actually we also just agree with @Vib-UX to open it against upstream, since the PR is quite ready as is.

This commit adds a batch client connection and async RPC calls to the bitcoind client which will enable batching multiple requests speeding up operations by reducing round trips.

This will leverage the updated batch client to fetch multiple blocks in one go which can speed up rescan and filter blocks.

Both RescanBlocksBatched() and FilterBlocksBatched() leverages GetBlocksBatch() which works with the updated batch client for speed ups.

Testcoverage to ensure that GetBlocksBatch() which leverages batchAPI works as expected with enhanced performance.

Testcoverage to ensure that RescanBlocksBatched() which leverages updated batchAPI for scanning the chain works as expected with enhanced performance.

Testcoverage to ensure that FilterBlocksBatched() which leverages batchAPI for scanning the chain works as expected with enhanced performance.

guggero reviewed Apr 14, 2022

View reviewed changes

Comment thread chain/bitcoind_client.go Outdated

Comment thread chain/bitcoind_client.go Outdated

Comment thread chain/bitcoind_conn.go Outdated

Comment thread chain/bitcoind_client.go Outdated

Vib-UX self-assigned this Apr 15, 2022

Vib-UX added the enhancement New feature or request label Apr 15, 2022

Vib-UX requested a review from guggero April 18, 2022 05:40

guggero removed their request for review May 5, 2022 13:31

bhandras reviewed May 7, 2022

View reviewed changes

Vib-UX force-pushed the BatchClient branch 2 times, most recently from ef5c3f5 to 1a97548 Compare May 10, 2022 04:59

Vib-UX force-pushed the BatchClient branch 4 times, most recently from a457655 to 7045f44 Compare June 1, 2022 20:07

Vib-UX force-pushed the BatchClient branch from 70a981e to 75a30c3 Compare June 16, 2022 12:41

bhandras reviewed Jun 23, 2022

View reviewed changes

Comment thread chain/bitcoind_client.go Outdated

Comment thread chain/bitcoind_client.go Outdated

Comment thread chain/bitcoind_client.go Outdated

Comment thread chain/bitcoind_conn.go Outdated

Vib-UX force-pushed the BatchClient branch from de5cc60 to b7bed3b Compare July 5, 2022 08:22

bhandras reviewed Jul 7, 2022

View reviewed changes

Comment thread chain/bitcoind_client.go

Comment thread chain/bitcoind_conn.go Outdated

Comment thread chain/bitcoind_conn.go

Vib-UX force-pushed the BatchClient branch from b7bed3b to 0be15cc Compare July 11, 2022 17:02

bhandras suggested changes Jul 14, 2022

View reviewed changes

Vib-UX force-pushed the BatchClient branch 2 times, most recently from 760fa75 to a13d267 Compare July 17, 2022 11:31

Vib-UX force-pushed the BatchClient branch 4 times, most recently from 8aecdbc to 3b15834 Compare July 29, 2022 18:06

bhandras approved these changes Aug 2, 2022

View reviewed changes

Vib-UX force-pushed the BatchClient branch from 98f1a30 to 31364a9 Compare August 4, 2022 18:26

Vib-UX force-pushed the BatchClient branch 2 times, most recently from 556f769 to a6ba11e Compare August 6, 2022 02:23

Vib-UX requested a review from guggero August 6, 2022 02:54

Vib-UX force-pushed the BatchClient branch from a6ba11e to 36bcc1e Compare August 6, 2022 06:31

bhandras approved these changes Aug 8, 2022

View reviewed changes

guggero removed their request for review August 8, 2022 15:22

Vib-UX force-pushed the BatchClient branch from 36bcc1e to fb5f608 Compare August 8, 2022 18:49

Vib-UX changed the title ~~Batch client~~ Adds Batch client connection and async RPC calls to the bitcoind client Aug 8, 2022

Vib-UX force-pushed the BatchClient branch 2 times, most recently from 759043b to 7f29aa8 Compare August 21, 2022 16:08

Vib-UX added 8 commits November 20, 2022 23:07

chain: adds batched async calls support for bitcoind client

5e3f9d9

This commit adds a batch client connection and async RPC calls to the bitcoind client which will enable batching multiple requests speeding up operations by reducing round trips.

chain: adds GetBlocksBatch() to the bitcoind client

51c887f

This will leverage the updated batch client to fetch multiple blocks in one go which can speed up rescan and filter blocks.

chain: adds RescanBlocksBatched() and FilterBlocksBatched()

41ce8a8

Both RescanBlocksBatched() and FilterBlocksBatched() leverages GetBlocksBatch() which works with the updated batch client for speed ups.

chain: testcoverage for GetBlocksBatch()

418f510

Testcoverage to ensure that GetBlocksBatch() which leverages batchAPI works as expected with enhanced performance.

chain: testcoverage for RescanBlocksBatched()

e9caa42

Testcoverage to ensure that RescanBlocksBatched() which leverages updated batchAPI for scanning the chain works as expected with enhanced performance.

chain: adds utility functions for testing

1aff327

chain: testcoverage for FilterBlocksBatched()

ad84975

Testcoverage to ensure that FilterBlocksBatched() which leverages batchAPI for scanning the chain works as expected with enhanced performance.

chain: refactors and adds generic fnc()

931e55d

Vib-UX force-pushed the BatchClient branch from 7f29aa8 to 931e55d Compare November 20, 2022 17:53

Conversation

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

Uh oh!

PR serves in relation to fixing issue 5041

Pull Request Checklist

Testing

Code Style and Documentation

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bhandras 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bhandras left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bhandras left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bhandras Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bhandras commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

guggero commented Aug 5, 2022

Uh oh!

bhandras commented Aug 5, 2022

Uh oh!

Vib-UX commented Aug 6, 2022

Uh oh!

bhandras left a comment

Choose a reason for hiding this comment

Uh oh!

bhandras Aug 8, 2022

Vib-UX commented Apr 13, 2022 •

edited

Loading

bhandras left a comment •

edited

Loading

bhandras commented Aug 2, 2022 •

edited

Loading

Vib-UX commented Aug 4, 2022 •

edited

Loading