Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Neutrino: Sync #1168

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

manavdesai27
Copy link
Contributor

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

@codecov
Copy link

codecov bot commented Jul 22, 2023

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 Outdated Show resolved Hide resolved
lib/blockchain/chaindb.js Outdated Show resolved Hide resolved
lib/blockchain/chaindb.js Outdated Show resolved Hide resolved
lib/blockchain/chaindb.js Outdated Show resolved Hide resolved
lib/net/pool.js Outdated Show resolved Hide resolved
lib/net/pool.js Outdated Show resolved Hide resolved
lib/net/pool.js Outdated
Comment on lines 2164 to 2169
const stopHash = packet.stopHash;
if (!stopHash.equals(this.requestedStopHash)) {
this.logger.warning('Received CFHeaders packet with wrong stopHash');
peer.ban();
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is recommended in the BIP-157

test/neutrino-test.js Outdated Show resolved Hide resolved
test/neutrino-test.js Show resolved Hide resolved
test/p2p-test.js Show resolved Hide resolved
@manavdesai27 manavdesai27 force-pushed the neutrino-sync branch 3 times, most recently from bf60150 to a86cd04 Compare August 1, 2023 21:15
@manavdesai27
Copy link
Contributor Author

@masterchief164 Could you please review this PR?

lib/net/peer.js Outdated Show resolved Hide resolved
lib/net/pool.js Outdated Show resolved Hide resolved
lib/net/pool.js Outdated Show resolved Hide resolved
lib/net/pool.js Show resolved Hide resolved
lib/net/pool.js Outdated Show resolved Hide resolved
lib/net/pool.js Outdated Show resolved Hide resolved
lib/net/pool.js Outdated Show resolved Hide resolved
lib/net/pool.js Outdated Show resolved Hide resolved
Comment on lines +4799 to +4763
this.header = header;
this.height = height;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@manavdesai27 manavdesai27 Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
lib/blockchain/layout.js Outdated Show resolved Hide resolved
@@ -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');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@theanmolsharma theanmolsharma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* Save filter
* @param {Hash} blockHash
* @param {BasicFilter} basicFilter
* @param {Hash} filterHeader
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add blockheight

Comment on lines +97 to +100
assert(blockHash);
assert(blockHeight);
assert(basicFilter);
assert(filterHeader);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably use 1 assertion here

Suggested change
assert(blockHash);
assert(blockHeight);
assert(basicFilter);
assert(filterHeader);
assert(blockHash & blockHeight & basicFilter & filterHeader);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@manavdesai27 manavdesai27 Aug 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +102 to +104
const filter = new Filter();
filter.filter = basicFilter.toRaw();
filter.header = filterHeader;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +747 to +748
const stopHeight = chainHeight - startHeight + 1 > 2000
? startHeight + 1999 : chainHeight;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should do the same

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

Comment on lines +2163 to +2164
const nextStopHeight = stopHeight + 2000 < this.chain.height
? stopHeight + 2000 : this.chain.height;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return;
}

const stopHash = packet.stopHash;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +2216 to +2217
if (cFilterHeight === stopHeight
&& stopHeight < this.chain.height) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

startHeight,
stopHash
);
return;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return is unnecessary here.

const basicFilter = new BasicFilter();
const gcsFilter = basicFilter.fromNBytes(filter);

const filterHeader = this.cfHeaderChain.head.header;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add some line breaks in L2208 -L2243

Comment on lines +77 to +78
const filterHeight = filterIndexer.height;
assert.equal(filterHeight, neutrinoNode.chain.height);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two lines are redundant after the forValue

assert.equal(neutrinoNode.chain.height, fullNode.chain.height);
});

it('should cfheaders and getcfilters', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +46 to +47
logConsole: true,
logLevel: 'debug',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two thoughts here

  1. i don't think this needs to be exclusive to the loader peer
  2. 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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return;

if (!this.syncing)
if (!this.syncing || this.filterSyncing)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment on lines +777 to +779
this.requestedFilterType = common.FILTERS.BASIC;
this.getcfiltersStartHeight = startHeight;
this.requestedStopHash = stopHash;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +92 to +94
this.requestedFilterType = null;
this.getcfiltersStartHeight = null;
this.requestedStopHash = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@pinheadmz pinheadmz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supposed to be ...?

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +744 to +745
const startHeight = cFHeaderHeight
? cFHeaderHeight + 1 : 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants