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.
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.
p2p_compactblocks.py is failing with this change:
bash-3.2$ test/functional/test_runner.py p2p_compactblocks.py
Temporary test directory at /var/folders/z2/cn877pxd3czdfh47mfkmbwgm0000gn/T/test_runner_₿_🏃_20190321_184436
Remaining jobs: [p2p_compactblocks.py]
1/1 - p2p_compactblocks.py failed, Duration: 3 s
stdout:
2019-03-21T10:44:36.761000Z TestFramework (INFO): Initializing test directory /var/folders/z2/cn877pxd3czdfh47mfkmbwgm0000gn/T/test_runner_₿_🏃_20190321_184436/p2p_compactblocks_0
2019-03-21T10:44:38.441000Z TestFramework (INFO): Running tests, pre-segwit activation:
2019-03-21T10:44:38.441000Z TestFramework (INFO): Testing SENDCMPCT p2p message...
2019-03-21T10:44:39.097000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/Users/michael/github/bitcoin/test/functional/test_framework/test_framework.py", line 175, in main
self.run_test()
File "/Users/michael/github/bitcoin/test/functional/p2p_compactblocks.py", line 806, in run_test
self.test_sendcmpct(self.nodes[0], self.test_node, 1)
File "/Users/michael/github/bitcoin/test/functional/p2p_compactblocks.py", line 212, in test_sendcmpct
check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" in p.last_message)
File "/Users/michael/github/bitcoin/test/functional/p2p_compactblocks.py", line 175, in check_announcement_of_new_block
block_hash, peer.last_message.get("cmpctblock", None), peer.last_message.get("inv", None)))
AssertionError: block_hash=48262114212731911042171011596261530915111028555007967809551738925302554894788, cmpctblock=None, inv=msg_inv(inv=[CInv(type=Block hash=6ab3637cd497baf974b2449251695e43f104bbdae282328302333645dd3121c4)])
2019-03-21T10:44:39.154000Z TestFramework (INFO): Stopping nodes
2019-03-21T10:44:39.517000Z TestFramework (WARNING): Not cleaning up dir /var/folders/z2/cn877pxd3czdfh47mfkmbwgm0000gn/T/test_runner_₿_🏃_20190321_184436/p2p_compactblocks_0
2019-03-21T10:44:39.517000Z TestFramework (ERROR): Test failed. Test logging available at /var/folders/z2/cn877pxd3czdfh47mfkmbwgm0000gn/T/test_runner_₿_🏃_20190321_184436/p2p_compactblocks_0/test_framework.log
2019-03-21T10:44:39.518000Z TestFramework (ERROR): Hint: Call /Users/michael/github/bitcoin/test/functional/combine_logs.py '/var/folders/z2/cn877pxd3czdfh47mfkmbwgm0000gn/T/test_runner_₿_🏃_20190321_184436/p2p_compactblocks_0' to consolidate all logs
p2p_compactblocks.py | ✖ Failed | 3 s
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
Sorry for the pain here — I’ll try to rework the test to be more maintainable (and update for this PR).
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.
2057 | @@ -2058,6 +2058,8 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr 2058 | State(pfrom->GetId())->fProvidesHeaderAndIDs = true; 2059 | State(pfrom->GetId())->fWantsCmpctWitness = nCMPCTBLOCKVersion == 2; 2060 | } 2061 | + // ignore HB requests from peers without witness support 2062 | + fAnnounceUsingCMPCTBLOCK = fAnnounceUsingCMPCTBLOCK && (pfrom->GetLocalServices() & NODE_WITNESS);
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().
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
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?
