net processing: Make it more obvious that we'll never upgrade a pre-segwit node to high-bandwidth #18461
Conversation
…idth This shouldn't happen anyway since MaybeSetPeerAsAnnouncingHeaderAndIDs() is called from BlockChecked() only when the node is out of IBD, but make it explicit.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
| @@ -557,17 +557,20 @@ static void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman* connma | |||
| } | |||
| connman->ForNode(nodeid, [connman](CNode* pfrom){ | |||
| AssertLockHeld(cs_main); | |||
| uint64_t nCMPCTBLOCKVersion = (pfrom->GetLocalServices() & NODE_WITNESS) ? 2 : 1; | |||
| // Never try to upgrade a pre-segwit node to high-bandwidth compact block provider | |||
There was a problem hiding this comment.
Here or in commit message maybe lay out the rationalbetter "As a pre-segwit node won't send us new consensus data there is no reason to receive a block from them after softfork activation, for which we won't able to fully validate" (unless there is no segwit transaction in the block but did this happen these days?).
Reading more GetFetchFlags, did you spend time thinking why there is any reason we want to INV(MSG_BLOCK) no pre-segwit nodes ?
There was a problem hiding this comment.
As a pre-segwit node won't send us new consensus data there is no reason to receive a block from them after softfork activation, for which we won't able to fully validate
I don't think this is the place to document such a fundamental fact about segwit - that we won't download blocks from pre-segwit nodes.
Reading more GetFetchFlags, did you spend time thinking why there is any reason we want to INV(MSG_BLOCK) no pre-segwit nodes ?
I'm afraid I don't understand this question. It doesn't seem related to this PR, at least.
|
Closing. This is low priority, and perhaps can be revisited as part of a larger compact blocks code clean up. |
This shouldn't happen anyway since
MaybeSetPeerAsAnnouncingHeaderAndIDs() is called from BlockChecked()
only when the node is out of IBD, but make it explicit.
Also set the
nCMPCTBLOCKVersionbased on the peer's services.Closes #18460