feat: added headers first sync #1156
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1156 +/- ##
==========================================
+ Coverage 69.55% 69.81% +0.25%
==========================================
Files 158 159 +1
Lines 26603 26725 +122
==========================================
+ Hits 18505 18659 +154
+ Misses 8098 8066 -32
☔ View full report in Codecov by Sentry. |
masterchief164
force-pushed
the
headers_first_sync
branch
3 times, most recently
from
abc8bc2
to
b03ac47
Compare
masterchief164
force-pushed
the
headers_first_sync
branch
from
b03ac47
to
d213418
Compare
There was a problem hiding this comment.
I left quite a few comments already but I wasn't sure if I was leading you astray or not, so I took a swing a branch myself: https://github.com/pinheadmz/bcoin/commits/neutrino-mz
Let's talk about it on the call this week
lib/blockchain/chain.js
Outdated
| return; | ||
|
|
||
| if (!this.hasChainwork()) | ||
| return; | ||
|
|
||
| this.synced = true; | ||
| this.emit('full'); | ||
| if (this.options.neutrino) | ||
| this.emit('headersFull'); |
There was a problem hiding this comment.
nit: we usually use spaces for event strings if its more than one word
| this.emit('headersFull'); | |
| this.emit('headers full'); |
lib/blockchain/chain.js
Outdated
| @@ -2616,6 +2618,7 @@ class ChainOptions { | |||
| this.compression = true; | |||
|
|
|||
| this.spv = false; | |||
| this.neutrino = false; | |||
There was a problem hiding this comment.
I still think we can live without a neutrino mode in chain. It really is more of a pool option, and then in node/neutrino we can set pool.neutrino along with chain.spv and that should work
lib/net/pool.js
Outdated
| if (this.options.neutrino) { | ||
| this.headerChain.push(new HeaderEntry(tip.hash, tip.height)); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Why do we need to return early like this in neutrino mode? It doesn't finish resetting the headers chain
lib/net/pool.js
Outdated
| * @return {Promise} | ||
| */ | ||
|
|
||
| async startHeadersSync() { |
There was a problem hiding this comment.
Let's see if we can delete this function
lib/net/pool.js
Outdated
| return; | ||
| } | ||
|
|
||
| node = new HeaderEntry(hash, height); | ||
|
|
||
| if (node.height === this.headerTip.height) { | ||
| if (!this.options.neutrino && node.height === this.headerTip.height) { |
There was a problem hiding this comment.
why is this necessary? It looks like the checkpoints-headers chain is getting mixed up with neutrino maybe we should chat about it on the weekly call. We might just be able to ignore the in memory headers chain in pool and just let it continue to do its thing with the one difference that we save the headers to disk throughout sync.
lib/net/pool.js
Outdated
| this.headerChain.shift(); | ||
| this.resolveHeaders(peer); | ||
| return; | ||
| } | ||
|
|
||
| // Request more headers. | ||
| peer.sendGetHeaders([node.hash], this.headerTip.hash); | ||
| if (this.chain.synced) |
There was a problem hiding this comment.
Is there any harm in just sending the request anyway even if we think we are synced?
lib/net/pool.js
Outdated
| @@ -2293,7 +2346,7 @@ class Pool extends EventEmitter { | |||
|
|
|||
| const hash = block.hash(); | |||
|
|
|||
| if (!this.resolveBlock(peer, hash)) { | |||
| if (!this.options.neutrino && !this.resolveBlock(peer, hash)) { | |||
There was a problem hiding this comment.
Since we never add blocks to blockMap I wonder if it might be cleaner to just add if neutrino return at the top of resolveBlock()?
lib/net/pool.js
Outdated
| if (options.neutrino != null) { | ||
| assert(typeof options.neutrino === 'boolean'); | ||
| this.neutrino = options.neutrino; | ||
| assert(options.compact === false || |
There was a problem hiding this comment.
can this just assert !options.compactor am I missing something?
masterchief164
force-pushed
the
headers_first_sync
branch
from
d213418
to
0d6615e
Compare
This PR adds support for headers first sync in neutrino mode.