Ignore BIP-152 HB requests from non-witness peers. #15633
Conversation
To speed up block propagation BIP-152 allows peers to signal that they would like us to announce new blocks to them by just sending a compact block. But propagation speed through non-witness peers is irrelevant, so ignore the request for these and save bandwidth. Instead we'll just send them a header and they can getdata the (compact)block if they actually need it from us.
|
|
|
nevermind. after spending an hour trying to update the tests to support this, I give up. it's not worth the effort. |
|
Sorry for the pain here — I’ll try to rework the test to be more maintainable (and update for this PR). |
|
it would be unfortunate if a clear improvement couldn't been done because of test impact, but if you're sure it's not worth the effort then let's leave it be
oh great! |
|
It's a small improvement-- but the test makes really extensive assumptions about the protocol flow, and unfortunately its base case is a v1 peer (so it breaks much more than one case that tries HB vs non-HB for v1 peers). The easy change would be switching almost all the test to v2, but then I'm concerned that it would leave v1 under-tested. Some of this is the product of the test design, it's much closer to a unit test that expects an EXACT behaviour, than a system test.... and some of that is just a consequence of using the mininode to interrogate behaviour. Another possibility would be dropping most of the v1 CB support. E.g. only support answering getdata for v1 compact block. At least that might simplify the codebase some. |
| @@ -2058,6 +2058,8 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr | |||
| State(pfrom->GetId())->fProvidesHeaderAndIDs = true; | |||
| State(pfrom->GetId())->fWantsCmpctWitness = nCMPCTBLOCKVersion == 2; | |||
| } | |||
| // ignore HB requests from peers without witness support | |||
| fAnnounceUsingCMPCTBLOCK = fAnnounceUsingCMPCTBLOCK && (pfrom->GetLocalServices() & NODE_WITNESS); | |||
There was a problem hiding this comment.
Isn't GetLocalServices() always equal to what our services are, rather than our peers? (I find this very confusing, but tracing through the code I can't see how this relates to what our peer sends us.)
I think this code should be checking against the fWantsCmpctWitness variable for the peer, not GetLocalServices().
There was a problem hiding this comment.
Indeed, it's gated on nCMPCTBLOCKVersion on my branch now, but it isn't showing up here because the PR is closed. What I was actually intending to match on was pfrom->nServices ... I'm not actually sure what will happen if a peer requests v2 compact blocks while not sending NODE_WITNESS
There was a problem hiding this comment.
I think (but haven't checked recently) that NODE_WITNESS is only used for finding outbound peers that we want to be connected to, and that we don't use it anywhere else in our p2p protocol logic (eg deciding how to serialize transactions/blocks and negotiating compact block versions). So I suspect it would work just fine?
|
Is this any easier now that #15660 is merged? |
To speed up block propagation BIP-152 allows peers to signal that
they would like us to announce new blocks to them by just sending
a compact block. But propagation speed through non-witness peers
is irrelevant, so ignore the request for these and save bandwidth.
Instead we'll just send them a header and they can getdata the
(compact)block if they actually need it from us.