Skip to content
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

Ignore BIP-152 HB requests from non-witness peers. #15633

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@gmaxwell
Copy link
Member

commented Mar 21, 2019

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.

Ignore BIP-152 HB requests from non-witness peers.
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.
@gmaxwell

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2019

@fanquake fanquake added the P2P label Mar 21, 2019

@fanquake

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

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
@gmaxwell

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2019

nevermind. after spending an hour trying to update the tests to support this, I give up. it's not worth the effort.

@gmaxwell gmaxwell closed this Mar 21, 2019

@sdaftuar

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Sorry for the pain here — I’ll try to rework the test to be more maintainable (and update for this PR).

@laanwj

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

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!

@gmaxwell

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2019

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);

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Mar 24, 2019

Member

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().

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Mar 24, 2019

Author Member

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

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Mar 24, 2019

Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.