-
Notifications
You must be signed in to change notification settings - Fork 812
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 #1155
base: master
Are you sure you want to change the base?
Neutrino #1155
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this! Off to a great start. I know you're just drafting right now but also at some point be sure to clean up the commit history, there's some funny rebase stuff in there I think.
@@ -583,7 +583,7 @@ class Chain extends AsyncEmitter { | |||
// UASF is now enforced (bip148) (mainnet-only). | |||
if (this.options.bip148 && this.network === Network.main) { | |||
if (witness !== thresholdStates.LOCKED_IN | |||
&& witness !== thresholdStates.ACTIVE) { | |||
&& witness !== thresholdStates.ACTIVE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change automatic from you linter? The original code doesn't fail the bcoin lint tests, lets leave it alone since it just makes reviewing your work a bit more confusing (there's several nits like this in the PR)
lib/blockchain/chain.js
Outdated
if (this.options.neutrino && this.tip.time < 1686851917) // TODO change this later | ||
return; | ||
|
||
if (!this.hasChainwork()) | ||
else if (!this.options.neutrino && | ||
this.tip.time < util.now() - this.network.block.maxTipAge) | ||
return; | ||
|
||
if (!this.options.neutrino && !this.hasChainwork()) | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all these changes just temporary? I don't understand why we'd emit "full" in any mode without chainwork and tip time
@@ -2616,6 +2617,7 @@ class ChainOptions { | |||
this.compression = true; | |||
|
|||
this.spv = false; | |||
this.neutrino = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still feeling like we can get away with a chain in SPV mode but a pool in neutrino mode, which might mean we can leave chain.js mostly alone. But I'm just starting to dig in here so I defer to you
lib/net/pool.js
Outdated
// todo: verify the filterHeader | ||
// todo: save the filterHeader | ||
previousFilterHeader = filterHeader; | ||
this.chain.db.neutrinoState.headerHeight = blockHeight; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think pool should ever call anything in chain.db
. There should probably helper functions in this.chain
instead that manage the incoming data and just tell pool if its valid (do we need to ban the peer that sent it to us, etc). See pool.addBlock()
for example
lib/net/pool.js
Outdated
@@ -2123,6 +2330,7 @@ class Pool extends EventEmitter { | |||
return await this._handleHeaders(peer, packet); | |||
} finally { | |||
unlock(); | |||
this.emit('headersFull'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whats this for? should it be in _handleHeaders()
instead?
lib/net/pool.js
Outdated
@@ -2200,6 +2410,8 @@ class Pool extends EventEmitter { | |||
this.headerNext = node; | |||
|
|||
this.headerChain.push(node); | |||
if (this.options.neutrino) | |||
await this._addBlock(peer, header, chainCommon.flags.VERIFY_NONE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VERIFY_NONE
literally means verify nothing, that can't be safe! Lets look at this together
@@ -2139,7 +2347,8 @@ class Pool extends EventEmitter { | |||
async _handleHeaders(peer, packet) { | |||
const headers = packet.items; | |||
|
|||
if (!this.checkpoints) | |||
if (!this.checkpoints && !this.options.neutrino) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets talk about how _handleHeaders()
should look. I think it should look like handleBlock essentially which makes the most sense, in which we just pass the headers to chain to verify and store. However FullNode has this headerchain thing in pool which should I think we should move this logic to a new function and call that from here in full mode. (and sigh yes in spv mode too just to not break anything, but in the future we can make that a lot smarter too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example in the pool-only headers thing, we verify the header PoW and prevblock right here, but all that should be done by chain in neutrino mode
if (this.neutrino) { | ||
this.requiredServices |= common.services.NODE_COMPACT_FILTERS; | ||
this.checkpoints = true; | ||
this.compact = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could also add (like SPV):
this.noRelay = true;
this.checkpoints = true;
this.compact = false;
this.bip37 = false;
this.bip157 = false;
this.listen = false;
05cacfc
to
a19e1b2
Compare
This PR implements the client part of the neutrino project.