Neutrino: Sync #1168

manavdesai27 · 2023-07-20T18:13:12Z

This PR implements Headers-sync and the synchronisation of filters for Neutrino.

codecov · 2023-07-22T20:47:24Z

Codecov Report

Patch coverage: 77.32% and project coverage change: +0.84% 🎉

Comparison is base (a4bf281) 70.26% compared to head (d1d8e80) 71.10%.
Report is 6 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1168      +/-   ##
==========================================
+ Coverage   70.26%   71.10%   +0.84%     
==========================================
  Files         174      175       +1     
  Lines       27367    27664     +297     
==========================================
+ Hits        19229    19671     +442     
+ Misses       8138     7993     -145

Files Changed	Coverage Δ
lib/bcoin-browser.js	`0.00% <0.00%> (ø)`
lib/bcoin.js	`0.00% <0.00%> (ø)`
lib/client/node.js	`26.78% <0.00%> (-0.49%)`	⬇️
lib/net/netaddress.js	`87.03% <ø> (ø)`
lib/net/seeds/main.js	`100.00% <ø> (ø)`
lib/node/fullnode.js	`85.42% <ø> (+0.88%)`	⬆️
lib/node/http.js	`59.79% <0.00%> (-0.16%)`	⬇️
lib/wallet/nodeclient.js	`73.01% <50.00%> (-6.99%)`	⬇️
lib/node/node.js	`75.67% <62.50%> (-0.76%)`	⬇️
lib/net/pool.js	`64.25% <73.86%> (+5.00%)`	⬆️
... and 4 more

... and 9 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lib/blockchain/chain.js

lib/blockchain/chaindb.js

lib/net/pool.js

pinheadmz · 2023-07-26T18:42:15Z

lib/net/pool.js

+    const stopHash = packet.stopHash;
+    if (!stopHash.equals(this.requestedStopHash)) {
+      this.logger.warning('Received CFHeaders packet with wrong stopHash');
+      peer.ban();
+      return;
+    }


is there a possibility of race condition here? like, could this value be different per peer? might make more sense in peer.js like hashContinue

Yeah, this is recommended in the BIP-157

test/neutrino-test.js

test/p2p-test.js

manavdesai27 · 2023-08-02T16:58:31Z

@masterchief164 Could you please review this PR?

lib/net/peer.js

lib/net/pool.js

masterchief164 · 2023-08-02T18:28:57Z

lib/net/pool.js

+    this.header = header;
+    this.height = height;


rename them to filter header and filter height.
also add a field for the filterType

I think we should keep height the same, as it is by logic the height of a chain. We could rename header to filterHeader.

lib/node/neutrino.js

lib/blockchain/layout.js

manavdesai27 · 2023-08-04T10:50:09Z

lib/bcoin-browser.js

@@ -89,6 +89,7 @@ bcoin.node = require('./node');
 bcoin.Node = require('./node/node');
 bcoin.FullNode = require('./node/fullnode');
 bcoin.SPVNode = require('./node/spvnode');
+bcoin.Neutrino = require('./node/neutrino');


@masterchief164 Do we need to add neutrino to bcoin-browser? AFAIK, bcoin-browser is obselete.

theanmolsharma

Did a first-pass review. Some suggestions and nits. Nice work!!

theanmolsharma · 2023-08-05T06:56:54Z

lib/indexer/filterindexer.js

+   * Save filter
+   * @param {Hash} blockHash
+   * @param {BasicFilter} basicFilter
+   * @param {Hash} filterHeader


nit: add blockheight

theanmolsharma · 2023-08-05T06:57:59Z

lib/indexer/filterindexer.js

+    assert(blockHash);
+    assert(blockHeight);
+    assert(basicFilter);
+    assert(filterHeader);


We can probably use 1 assertion here

Suggested change

assert(blockHash);

assert(blockHeight);

assert(basicFilter);

assert(filterHeader);

assert(blockHash & blockHeight & basicFilter & filterHeader);

We won't be able to know which value gives out the error.

And why would you do a logical and here? What if for example my bits are 1000 and 0001?

I'm guessing he meant &&? but if you are going to do this you mine as well check each input value type is correct too

theanmolsharma · 2023-08-05T07:03:18Z

lib/indexer/filterindexer.js

+    const filter = new Filter();
+    filter.filter = basicFilter.toRaw();
+    filter.header = filterHeader;


maybe I'm being too picky about code styles but I'd prefer if you pass an options object instead of assigning values to class variables

I see that many functions in filterindexer.js is doing this. We should make this change everywhere since we are touching this file anyway.

theanmolsharma · 2023-08-05T07:30:29Z

lib/net/pool.js

+    const cFHeaderHeight = this.cfHeaderChain.tail.height;
+    const startHeight = cFHeaderHeight
+    ? cFHeaderHeight + 1 : 1;
+    const chainHeight = await this.chain.height;


Suggested change

const chainHeight = await this.chain.height;

const chainHeight = this.chain.height;

theanmolsharma · 2023-08-05T07:40:07Z

lib/net/pool.js

+    const stopHeight = chainHeight - startHeight + 1 > 2000
+     ? startHeight + 1999 : chainHeight;


this should do the same

Suggested change

const stopHeight = chainHeight - startHeight + 1 > 2000

? startHeight + 1999 : chainHeight;

const stopHeight = Math.min(chainHeight, startHeight + 1999);

theanmolsharma · 2023-08-05T07:56:49Z

lib/net/pool.js

+      const nextStopHeight = stopHeight + 2000 < this.chain.height
+        ? stopHeight + 2000 : this.chain.height;


Suggested change

const nextStopHeight = stopHeight + 2000 < this.chain.height

? stopHeight + 2000 : this.chain.height;

const nextStopHeight = Math.min(stopHeight + 2000, this.chain.height);

theanmolsharma · 2023-08-05T07:59:30Z

lib/net/pool.js

+      return;
+    }
+
+    const stopHash = packet.stopHash;


L2132 - L2168 is very hard to read. Let's add some line breaks to make it more readable.

theanmolsharma · 2023-08-05T08:03:07Z

lib/net/pool.js

+    if (cFilterHeight === stopHeight
+      && stopHeight < this.chain.height) {


Suggested change

if (cFilterHeight === stopHeight

&& stopHeight < this.chain.height) {

if (cFilterHeight === stopHeight && stopHeight < this.chain.height) {

I did it in one line only Anmol. But we gotta keep linting in mind as well.

I don't think it'll throw a lint error

I think the original is best for readability and consistency with codebase although we usually double indent the second condition like so:

bcoin/lib/blockchain/chain.js

Lines 585 to 586 in 6ae1752

if (witness !== thresholdStates.LOCKED_IN

&& witness !== thresholdStates.ACTIVE) {

theanmolsharma · 2023-08-05T08:09:02Z

lib/net/pool.js

+          startHeight,
+          stopHash
+        );
+        return;


return is unnecessary here.

theanmolsharma · 2023-08-05T08:11:14Z

lib/net/pool.js

+    const basicFilter = new BasicFilter();
+    const gcsFilter = basicFilter.fromNBytes(filter);
+
+    const filterHeader = this.cfHeaderChain.head.header;


add some line breaks in L2208 -L2243

pinheadmz · 2023-08-06T14:39:16Z

test/neutrino-test.js

+      const filterHeight = filterIndexer.height;
+      assert.equal(filterHeight, neutrinoNode.chain.height);


these two lines are redundant after the forValue

pinheadmz · 2023-08-06T14:41:19Z

test/neutrino-test.js

+      assert.equal(neutrinoNode.chain.height, fullNode.chain.height);
+    });
+
+    it('should cfheaders and getcfilters', async () => {


I think this entire test should be copied and run twice: after initial sync on L67, and then a second time after the 10 new blocks where it is now.

pinheadmz · 2023-08-06T14:42:09Z

test/neutrino-test.js

+      for (let i = 0; i < neutrinoNode.chain.height; i++) {
+          const hash = await neutrinoNode.chain.getHash(i);
+          const filter = await filterIndexer.getFilter(hash);
+          assert(filter);


are you sure this is all you need to test? that the filter exists?

pinheadmz · 2023-08-06T14:42:45Z

test/neutrino-test.js

+      logConsole: true,
+      logLevel: 'debug',


TODO: remove these before merge. doesnt have to be now

pinheadmz · 2023-08-06T14:46:09Z

lib/net/pool.js

+    const stopHash = await this.chain.getHash(stopHeight);
+    this.requestedFilterType = common.FILTERS.BASIC;
+    this.requestedStopHash = stopHash;
+    await this.peers.load.sendGetCFHeaders(


two thoughts here

i don't think this needs to be exclusive to the loader peer

this may get called before there are ANY peers connected

It should handle that case graefully (like use for each peer logic so if there are no peers, nothing happens)

the function should ALSO be called from (for example maybe) handleOpen() just like how we call sendSync(). It maybe even could be combined with sendSync() if neutirno? i dunno whats best

I think everytime we add a new block header after IBD we should ask for filters from there too

pinheadmz · 2023-08-06T17:59:47Z

lib/net/pool.js

      return;

-    if (!this.syncing)
+    if (!this.syncing || this.filterSyncing)


I don't think this is correct. In fact I don't think we need a this.filterSyncing flag because after IBD we always want both block headers and filters, they shouldn't be mutually exclusive. What's the case where syncing is false but filterSyncing is true?

pinheadmz · 2023-08-06T18:09:36Z

lib/net/pool.js

+    this.requestedFilterType = common.FILTERS.BASIC;
+    this.getcfiltersStartHeight = startHeight;
+    this.requestedStopHash = stopHash;


I still think these should not be set at the pool class level. See pool.resolveBlock() and pool.resolveTX() for an idea of what I'm thinking: we might send different requests to different peers and that should be ok. I'm playing with the code now and getting a lot of "wrong stophash" warnings

pinheadmz · 2023-08-06T18:10:42Z

lib/net/pool.js

+    this.requestedFilterType = null;
+    this.getcfiltersStartHeight = null;
+    this.requestedStopHash = null;


these should be set per-request, not at the class level

pinheadmz · 2023-08-06T18:12:58Z

lib/net/pool.js

+    if (!(blockHeight >= this.getcfiltersStartHeight
+       && blockHeight <= stopHeight)) {
+      this.logger.warning('Received CFilter packet with wrong blockHeight');
+      peer.increaseBan(10);


Lets not increase ban for this right now, this check should be changed to the per-request basis as well

pinheadmz · 2023-08-06T18:18:23Z

lib/node/neutrino.js

+    this.pool.on('headers', async () => {
+      if (this.chain.height === 0)
+        return;
+      this.logger.info('Block Headers are fully synced');


I don't think this is correct. The headers event is emitted by pool in _handleHeaders() every time we handle a headers packet, not when we are fully synced.

I think what we should do, in _handleHeaders(), is once we are done processing new headers, we check is chain is synced (i.e. IBD is done). If chain is synced we immedeately call get filters. otherwise, wait for chain to emit 'full' and trigger the first getfilters off of that.

pinheadmz

I worked on this commit which has several sync improvements related to my comments, and we can discuss on a call this week maybe:

pinheadmz@5c002d4

it passes the tests and syncs mainnet really well. it's not completely done though. it gets all the block headers and filter headers but only the first 1000 filters. I will try to finish the implementation today or tomorrow.

We also might want to add a test that generates thousands of blocks to make sure that full packets are working. We can guard it with BCOIN_RUN_LONG_TESTS if its annoyingly long

pinheadmz · 2023-08-06T19:17:13Z

lib/net/pool.js

+      blockHeight++;
+    }
+    this.logger.info('CFHeader height: %d', this.cfHeaderChain.tail.height);
+    if (this.headerChain.tail.height <= stopHeight)


supposed to be ...?

Suggested change

if (this.headerChain.tail.height <= stopHeight)

if (this.cfHeaderChain.tail.height <= stopHeight)

pinheadmz · 2023-08-06T19:20:57Z

lib/net/pool.js

+    const indexer = this.getFilterIndexer(filtersByVal[filterType]);
+    if (indexer.height % 100 === 0)
+      this.logger.debug(
+        'Received CFilter 100 packets from %s', peer.hostname()


this should not be a hard coded 100 but log the actual # of CFHeaders we received!

pinheadmz · 2023-08-06T19:34:47Z

lib/net/pool.js

+    const startHeight = cFHeaderHeight
+    ? cFHeaderHeight + 1 : 1;


actually since we intialize the cfHeaderChain with a dummy genesis block header at height 0, we shouldn't need the condition? Just +1 always?

manavdesai27 added 4 commits July 20, 2023 23:32

feat<node>: Headers-Sync

0d84694

feat<node>: Filters-Sync

ed0ce73

test<node>

059acb4

fix<node> Headers-Sync (maybeSync)

9750e82

pinheadmz reviewed Jul 26, 2023

View reviewed changes

manavdesai27 requested a review from pinheadmz August 1, 2023 19:47

manavdesai27 force-pushed the neutrino-sync branch 3 times, most recently from bf60150 to a86cd04 Compare August 1, 2023 21:15

manavdesai27 force-pushed the neutrino-sync branch from a86cd04 to 135d726 Compare August 2, 2023 18:44

masterchief164 suggested changes Aug 2, 2023

View reviewed changes

manavdesai27 force-pushed the neutrino-sync branch from 135d726 to b0a37aa Compare August 2, 2023 19:22

manavdesai27 requested a review from masterchief164 August 2, 2023 19:24

manavdesai27 force-pushed the neutrino-sync branch 5 times, most recently from d583ebc to 5fedebc Compare August 4, 2023 07:26

manavdesai27 commented Aug 4, 2023

View reviewed changes

lib/blockchain/layout.js Outdated Show resolved Hide resolved

manavdesai27 commented Aug 4, 2023

View reviewed changes

manavdesai27 force-pushed the neutrino-sync branch from 5fedebc to df6d023 Compare August 4, 2023 19:30

fix<node,test>: headers-sync, refactor and test bip157

d1d8e80

manavdesai27 force-pushed the neutrino-sync branch from df6d023 to d1d8e80 Compare August 4, 2023 19:46

theanmolsharma suggested changes Aug 5, 2023

View reviewed changes

pinheadmz reviewed Aug 6, 2023

View reviewed changes

	const chainHeight = await this.chain.height;
	const chainHeight = this.chain.height;

		const stopHeight = chainHeight - startHeight + 1 > 2000
		? startHeight + 1999 : chainHeight;

	const stopHeight = chainHeight - startHeight + 1 > 2000
	? startHeight + 1999 : chainHeight;
	const stopHeight = Math.min(chainHeight, startHeight + 1999);

		const nextStopHeight = stopHeight + 2000 < this.chain.height
		? stopHeight + 2000 : this.chain.height;

	const nextStopHeight = stopHeight + 2000 < this.chain.height
	? stopHeight + 2000 : this.chain.height;
	const nextStopHeight = Math.min(stopHeight + 2000, this.chain.height);

		if (cFilterHeight === stopHeight
		&& stopHeight < this.chain.height) {

	if (cFilterHeight === stopHeight
	&& stopHeight < this.chain.height) {
	if (cFilterHeight === stopHeight && stopHeight < this.chain.height) {

	if (witness !== thresholdStates.LOCKED_IN
	&& witness !== thresholdStates.ACTIVE) {

		const filterHeight = filterIndexer.height;
		assert.equal(filterHeight, neutrinoNode.chain.height);

	if (this.headerChain.tail.height <= stopHeight)
	if (this.cfHeaderChain.tail.height <= stopHeight)

Neutrino: Sync #1168

Are you sure you want to change the base?

Neutrino: Sync #1168

Conversation

manavdesai27 commented Jul 20, 2023

codecov bot commented Jul 22, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manavdesai27 commented Aug 2, 2023

Choose a reason for hiding this comment

manavdesai27 Aug 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theanmolsharma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manavdesai27 Aug 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinheadmz left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 22, 2023 •

edited

manavdesai27 Aug 2, 2023 •

edited

manavdesai27 Aug 6, 2023 •

edited

pinheadmz left a comment •

edited