Neutrino: Sync - #1168
Conversation
Codecov ReportPatch coverage:
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
☔ View full report in Codecov by Sentry. |
| const stopHash = packet.stopHash; | ||
| if (!stopHash.equals(this.requestedStopHash)) { | ||
| this.logger.warning('Received CFHeaders packet with wrong stopHash'); | ||
| peer.ban(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yeah, this is recommended in the BIP-157
bf60150 to
a86cd04
Compare
|
@masterchief164 Could you please review this PR? |
a86cd04 to
135d726
Compare
| this.header = header; | ||
| this.height = height; |
There was a problem hiding this comment.
rename them to filter header and filter height.
also add a field for the filterType
There was a problem hiding this comment.
I think we should keep height the same, as it is by logic the height of a chain. We could rename header to filterHeader.
135d726 to
b0a37aa
Compare
d583ebc to
5fedebc
Compare
| bcoin.Node = require('./node/node'); | ||
| bcoin.FullNode = require('./node/fullnode'); | ||
| bcoin.SPVNode = require('./node/spvnode'); | ||
| bcoin.Neutrino = require('./node/neutrino'); |
There was a problem hiding this comment.
@masterchief164 Do we need to add neutrino to bcoin-browser? AFAIK, bcoin-browser is obselete.
5fedebc to
df6d023
Compare
df6d023 to
d1d8e80
Compare
theanmolsharma
left a comment
theanmolsharma
left a comment
There was a problem hiding this comment.
Did a first-pass review. Some suggestions and nits. Nice work!!
| * Save filter | ||
| * @param {Hash} blockHash | ||
| * @param {BasicFilter} basicFilter | ||
| * @param {Hash} filterHeader |
There was a problem hiding this comment.
nit: add blockheight
| assert(blockHash); | ||
| assert(blockHeight); | ||
| assert(basicFilter); | ||
| assert(filterHeader); |
There was a problem hiding this comment.
We can probably use 1 assertion here
| assert(blockHash); | |
| assert(blockHeight); | |
| assert(basicFilter); | |
| assert(filterHeader); | |
| assert(blockHash & blockHeight & basicFilter & filterHeader); |
There was a problem hiding this comment.
We won't be able to know which value gives out the error.
There was a problem hiding this comment.
And why would you do a logical and here? What if for example my bits are 1000 and 0001?
There was a problem hiding this comment.
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
| const filter = new Filter(); | ||
| filter.filter = basicFilter.toRaw(); | ||
| filter.header = filterHeader; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
| const chainHeight = await this.chain.height; | |
| const chainHeight = this.chain.height; |
| const stopHeight = chainHeight - startHeight + 1 > 2000 | ||
| ? startHeight + 1999 : chainHeight; |
There was a problem hiding this comment.
this should do the same
| 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; |
There was a problem hiding this comment.
| 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; |
There was a problem hiding this comment.
L2132 - L2168 is very hard to read. Let's add some line breaks to make it more readable.
| if (cFilterHeight === stopHeight | ||
| && stopHeight < this.chain.height) { |
There was a problem hiding this comment.
| if (cFilterHeight === stopHeight | |
| && stopHeight < this.chain.height) { | |
| if (cFilterHeight === stopHeight && stopHeight < this.chain.height) { |
There was a problem hiding this comment.
I did it in one line only Anmol. But we gotta keep linting in mind as well.
There was a problem hiding this comment.
I don't think it'll throw a lint error
There was a problem hiding this comment.
I think the original is best for readability and consistency with codebase although we usually double indent the second condition like so:
Lines 585 to 586 in 6ae1752
| startHeight, | ||
| stopHash | ||
| ); | ||
| return; |
There was a problem hiding this comment.
return is unnecessary here.
| const basicFilter = new BasicFilter(); | ||
| const gcsFilter = basicFilter.fromNBytes(filter); | ||
|
|
||
| const filterHeader = this.cfHeaderChain.head.header; |
There was a problem hiding this comment.
add some line breaks in L2208 -L2243
| const filterHeight = filterIndexer.height; | ||
| assert.equal(filterHeight, neutrinoNode.chain.height); |
There was a problem hiding this comment.
these two lines are redundant after the forValue
| assert.equal(neutrinoNode.chain.height, fullNode.chain.height); | ||
| }); | ||
|
|
||
| it('should cfheaders and getcfilters', async () => { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
are you sure this is all you need to test? that the filter exists?
| logConsole: true, | ||
| logLevel: 'debug', |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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 callsendSync(). It maybe even could be combined withsendSync()if neutirno? i dunno whats best
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
| this.requestedFilterType = common.FILTERS.BASIC; | ||
| this.getcfiltersStartHeight = startHeight; | ||
| this.requestedStopHash = stopHash; |
There was a problem hiding this comment.
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
| this.requestedFilterType = null; | ||
| this.getcfiltersStartHeight = null; | ||
| this.requestedStopHash = null; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I worked on this commit which has several sync improvements related to my comments, and we can discuss on a call this week maybe:
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) |
There was a problem hiding this comment.
supposed to be ...?
| 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() |
There was a problem hiding this comment.
this should not be a hard coded 100 but log the actual # of CFHeaders we received!
| const startHeight = cFHeaderHeight | ||
| ? cFHeaderHeight + 1 : 1; |
There was a problem hiding this comment.
actually since we intialize the cfHeaderChain with a dummy genesis block header at height 0, we shouldn't need the condition? Just +1 always?
4 participants
This PR implements Headers-sync and the synchronisation of filters for Neutrino.