Outbound peers disconnect - implement #2283

[ReverseControl]

#2283 Suggested fix. However, it will disconnect outbound connection even if the invalid header comes from a compact block, which under BIP 152 there are scenarios where that is allowed.

We need a test for this. I would write it, but don't know how to. If someone can take care of that, or teach me how to do it, that would be great.

@rnicoll @patricklodder @michilumin

[patricklodder]

I think that the BIP-152 text referred to is:

As high-bandwidth mode permits relaying of CMPCTBLOCK messages prior to full validation (requiring only that the block header is valid before relay), nodes SHOULD NOT ban a peer for announcing a new block with a CMPCTBLOCK message that is invalid, but has a valid header. For avoidance of doubt, nodes SHOULD bump their peer-to-peer protocol version to 70015 or higher to signal that they will not ban or punish a peer for announcing compact blocks prior to full validation, and nodes SHOULD NOT announce a CMPCTBLOCK to a peer with a version number below 70015 before fully validating the block.

Since we do run protocol version 70015 since 1.14.0, and Dogecoin currently has 2.5x higher bandwidth and 10x lower latency requirements than Bitcoin for blocks, this at first glance looks very relevant, because a non-malicious peer will craft and broadcast CMPCTBLOCK messages received from a malicious peer of its own if the header was valid: meaning the following code SHOULD NOT cause a disconnect if this and the preceding HEADER message were combined into a single CMPCTBLOCK message rather than a BLOCK message:

dogecoin/qa/rpc-tests/p2p-acceptblock.py

Lines 337 to 342 in 59da28c

	test_node.send_message(msg_block(block_1443))
	# At this point we've sent an obviously-bogus block,
	# and we must get disconnected
	assert_equal(test_node.wait_for_disconnect(), True)
	print("Successfully got disconnected after sending an invalid block")

However, this (CMPCTBLOCK-only announcement) will never happen on mainnet by 1.14.x nodes because CMPCTBLOCK will only be triggered if NODE_WITNESS is set:

dogecoin/src/net_processing.cpp

Line 417 in e341979

uint64_t nCMPCTBLOCKVersion = (pfrom->GetLocalServices() & NODE_WITNESS) ? 2 : 1;

and that will only be set if the following condition is true:

dogecoin/src/init.cpp

Line 1581 in e341979

if (chainparams.GetConsensus(0).vDeployments[Consensus::DEPLOYMENT_SEGWIT].nTimeout != 0) {

and this condition is always false for all 1.14.x clients because of this:

dogecoin/src/chainparams.cpp

Line 110 in e341979

consensus.vDeployments[Consensus::DEPLOYMENT_SEGWIT].nTimeout = 0; // Disabled

So because the relevance right now is low, I propose to postpone this one until the next minor release, with the note that:

• 1.14/rpc tests #1431 disabled all CMPCTBLOCK tests even though the protocol is supported (and defaulted to in src/version.h !!!) - re-enabling that test (p2p-compactblocks.py) is a prerequisite for merging this PR.
• This fix is likely to be a MUST HAVE before any v1.21 soft forks come into play and adds pressure to 1.21: bring back compatibility tests #2435, will probably impact 1.21 timelines.

[ReverseControl]

@patricklodder I agree. Let's move it to the next minor release.

[patricklodder]

As mentioned above, this is blocked by not having p2p-compactblocks.py enabled. Adding do-not-merge as the test needs to work before changes from this PR, so that has to be raised separately.

The base branch was changed.

[ReverseControl]

@patricklodder What is the status of this?

[patricklodder]

This is still blocked because we still have no working p2p-compactblocks.py - I'll work on it.

[xanimo]

hey my apologies, i've been oblivious this was blocked on account of the non-functional test. perhaps i was in the know but forgot. :(

in any case i had been working on this 12/2021 and gutted most of the segwit code as modeled off an iteration of bitcoin-cash which i unfortunately can't find atm but seeing is how they didn't implement segwit and supported compact blocks i thought it a good reference. omitted the bch ref as it wasn't relevant.

the test passes, however i'm not certain it's what we need considering changing the test to make the code pass vs vice versa...anyway if anyone wants to use as a starting point, i've pushed a rebased version here for cherry-picking or whatever if worthy and if not i'll try to find some time to fix it up:
xanimo@aa2d05f

[ZachOlivier]

The new parameter was added to the end of the method, but this line makes it seem like it is located where the 'ppindex' is. I suggest swapping line 250 with line 251.