net processing: Make it more obvious that we'll never upgrade a pre-segwit node to high-bandwidth #18461

pull jnewbery wants to merge 1 commits into bitcoin:master from jnewbery:2020-03-sendcmpct-downgrade changing 1 files +5 −2

jnewbery commented at 11:45 PM on March 28, 2020: member

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 nCMPCTBLOCKVersion based on the peer's services.

Closes #18460

[net processing] Never try to upgrade a non-segwit peer to high-bandwidth

This shouldn't happen anyway since
MaybeSetPeerAsAnnouncingHeaderAndIDs() is called from BlockChecked()
only when the node is out of IBD, but make it explicit.

5079ebed79

fanquake added the label P2P on Mar 28, 2020
DrahtBot commented at 4:51 AM on March 29, 2020: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #19174 (refactor: replace CConnman pointers by references in net_processing.cpp by theStack)
- #17785 (p2p: Unify Send and Receive protocol versions by hebasto)
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.
in src/net_processing.cpp:560 in 5079ebed79
```
 556 | @@ -557,17 +557,20 @@ static void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman* connma
 557 |          }
 558 |          connman->ForNode(nodeid, [connman](CNode* pfrom){
 559 |              AssertLockHeld(cs_main);
 560 | -            uint64_t nCMPCTBLOCKVersion = (pfrom->GetLocalServices() & NODE_WITNESS) ? 2 : 1;
 561 | +            // Never try to upgrade a pre-segwit node to high-bandwidth compact block provider
```
ariard commented at 1:15 AM on April 25, 2020:

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 ?

jnewbery commented at 8:39 AM on July 15, 2020:

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.
jnewbery commented at 8:40 AM on July 15, 2020: member

Closing. This is low priority, and perhaps can be revisited as part of a larger compact blocks code clean up.
jnewbery closed this on Jul 15, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#18460 Disabling high-bandwidth compact block relay from pre-segwit nodes doesn't work

net processing: Make it more obvious that we'll never upgrade a pre-segwit node to high-bandwidth #18461

Conflicts