net processing: Tolerate sendheaders and sendcmpct messages before verack

jnewbery commented at 10:44 am on December 8, 2020: member

BIP 130 (sendheaders) and BIP 152 (compact blocks) do not specify at which stage the sendheaders or sendcmpct messages should be sent. Therefore we should tolerate them being sent before the version-verack handshake is complete.

fanquake added the label P2P on Dec 8, 2020

in src/net_processing.cpp:2506 in 2a942d4e19 outdated

2534@@ -2535,6 +2535,35 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
2535         return;
2536     }
2537 
2538+    if (msg_type == NetMsgType::SENDHEADERS) {

MarcoFalke commented at 11:48 am on December 8, 2020:

if you move this hunk before the wtxidrelay msg handling, it will avoid a merge conflict with other open pulls

jnewbery commented at 12:58 pm on December 8, 2020:

Done

MarcoFalke commented at 11:49 am on December 8, 2020: member

Concept ACK

DrahtBot commented at 12:18 pm on December 8, 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:

#20592 (doc: update wtxidrelay documentation per BIP339 by jonatack)

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.

jnewbery force-pushed on Dec 8, 2020

jnewbery renamed this:
~~[net processing] Tolerate sendheaders and sendcmpct messages before verack~~
net processing: Tolerate sendheaders and sendcmpct messages before verack
on Dec 8, 2020

benthecarman commented at 4:30 pm on December 8, 2020: contributor

ACK f72a62418c2c617ebe19e59f855f3bec1da9ee15

DrahtBot added the label Needs rebase on Dec 9, 2020

naumenkogs commented at 7:58 am on December 9, 2020: member

I’m wondering whether this is a good allocation of review resources? It makes practically no difference today, and maybe it won’t make a difference ever (unless there’s a new implementation I assume)?

At the same time, it’s not like the review is super-easy. We had this code working for years now, and now we might miss some corner case of an attacker sending us something while on-VERACK structs are not even initialized yet.

Or perhaps if you’re planning to rearrange sending signal messages, then I see the point.

jnewbery force-pushed on Dec 9, 2020

jnewbery commented at 8:56 am on December 9, 2020: member

we might miss some corner case of an attacker sending us something while on-VERACK structs are not even initialized yet.

We don’t initialize any structs in verack processing. verack processing simply sets fSuccessfullyConnected, logs, and sends the send* messages.

I’m wondering whether this is a good allocation of review resources?

How you allocate your time is up to you. If you don’t think this is worth reviewing, then please don’t feel under any obligation to spend time on it.

jnewbery commented at 8:56 am on December 9, 2020: member

rebased

DrahtBot removed the label Needs rebase on Dec 9, 2020

practicalswift commented at 9:41 am on December 9, 2020: contributor

@jnewbery Just to understand the context of this change: is this purely a “follow the BIP” change, or is there any known scenario in which this change would improve interoperability with any currently existing client?

jnewbery commented at 10:24 am on December 9, 2020: member

Is this purely a “follow the BIP” change, or is there any known scenario in which this change would improve interoperability with any currently existing client?

This is purely to follow the BIP. I don’t know of any client that sends send* messages before verack. However, future clients may wish to do so (since that’s a more rational place to negotiate features).

jonatack commented at 11:29 am on December 9, 2020: member

Concept ACK, IIUC correctly from a quick glance, the point of this PR is move-only of the hunk, from after the following “unsupported message” conditional to before it, confirm?

0    if (!pfrom.fSuccessfullyConnected) {
1        LogPrint(BCLog::NET, "Unsupported message \"%s\" prior to verack from peer=%d\n", SanitizeString(msg_type), pfrom.GetId());
2        return;
3    }

Should a proposal be made to update the BIPs on this point, similarly to BIPS 155/339?

jnewbery commented at 11:46 am on December 9, 2020: member

the point of this PR is move-only of the hunk, from after the following “unsupported message” conditional to before it, confirm?

Correct, this PR moves the processing of sendheaders and sendcmpct to before the !pfrom.fSuccessfullyConnected conditional. I’m loathe to call it ‘move-only’ since it’s a behaviour change.

Should a proposal be made to update the BIPs on this point, similarly to BIPS 155/339?

No, this change is simply updating our behaviour to be consistent with those BIPs (since neither specify when the message should be sent).

jonatack commented at 11:59 am on December 9, 2020: member

ACK 427f5736f174de5dfd60ad3fbbcdcbd498a2f30a

Lightly tested with blockfilterindex and peerbloomfilters enabled.

jnewbery commented at 12:05 pm on December 9, 2020: member

the point of this PR is move-only of the hunk, from after the following “unsupported message” conditional to before it, confirm?

Correct, this PR moves the processing of sendheaders and sendcmpct to before the !pfrom.fSuccessfullyConnected conditional. I’m loathe to call it ‘move-only’ since it’s a behaviour change.

For a bit more context, I’d like to make the following change at some point:

Introduce a connection_state enum to CNode which can take the values:

NEW_CONNECTION (version message not received)
VERSION_RECEIVED (version message received, verack not received)
FULLY_CONNECTED (version and verack both received)
DISCONNECTING (node is being disconnected. I’m not sure if this is necessary/a good addition)

Then ProcessMessage() becomes a table which maps message types to:

a ProcessXXXXXMessage() function
connection_states where this message type can be processed
connection_states where the peer should be disconnected for sending this message type

That makes the state machine much more explicit.

DrahtBot commented at 4:09 pm on December 9, 2020: member

🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

DrahtBot added the label Needs rebase on Dec 10, 2020

jnewbery commented at 10:20 am on December 10, 2020: member

rebased

jnewbery force-pushed on Dec 10, 2020

DrahtBot removed the label Needs rebase on Dec 10, 2020

laanwj commented at 12:59 pm on December 10, 2020: member

Code review ACK 82d0a43a23519361d3801c9c087859e13065c535 Checked that this is move-only.

jonatack commented at 11:23 pm on December 10, 2020: member

Code review re-ACK 82d0a43a23519361d3801c9c087859e13065c535 per git range-diff da957cd 427f573 82d0a43

benthecarman commented at 5:42 pm on December 11, 2020: contributor

Code review ACK 82d0a43

DrahtBot added the label Needs rebase on Dec 14, 2020

[net processing] Tolerate sendheaders and sendcmpct messages before verack

BIP 130 (sendheaders) and BIP 152 (compact blocks) do not specify at
which stage the `sendheaders` or `sendcmpct` messages should be sent.
Therefore we should tolerate them being sent before the version-verack
handshake is complete.

b316dcb758

jnewbery commented at 1:23 pm on December 14, 2020: member

rebased

jnewbery force-pushed on Dec 14, 2020

DrahtBot removed the label Needs rebase on Dec 14, 2020

MarcoFalke commented at 3:48 pm on December 14, 2020: member

review ACK b316dcb758e3dbd302fbb5941a1b5b0ef5f1f207 📒

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK b316dcb758e3dbd302fbb5941a1b5b0ef5f1f207 📒
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjFTQv+MjBurQNjNB/sPp5ynOe9ZPZqoLsSdpWQK3Mco7MIGVe0IuCiXO9EEwNK
 8n6olAJEkeeze+rtrCig8m55yptQLnQy/Us4U5XrAUHCDqTGy+Uja6m1zt7YZJkvK
 9SLoQa1SJCtqdmN9r4X3Ejeeat/nVDWRFkrKRaUHxj7E4+jq871h31mjX9eNc2CB4
10qwJQP/ZDK4oA0vzJuxqoU48Fs7HCXyk4XnXEm8hPLBmRLogJSF5B6V8+wULc++NT
112b5kUKBBQydqvDrzbOUPHs6QvnkoHflYq6b00BeQsdIy85GiT0TMZwi9G925cpf4
12xwIwgEDaEL2GhLpjnKS5BlHyjECC6TarPfPdVaLC9rU45hjmY/PktgK3oJiUEO0o
131TfjFNRHJYkiIz7CuUlcQOWbEwurQeXKx4Kmuy0EpFgK5Z5H1HlILKB8Uo/cSIEB
14dvGUZW2kODHCLEab2znPbGEhYej73gBgGiAp1o0MIDshYAW7DdYeNVQ5GC4UDph9
15SrTxzuXa
16=MU7a
17-----END PGP SIGNATURE-----

Timestamp of file with hash 4d415a967d56551b5e8c98d1933644ad7c3b68b61fba3333c1eab1b112bdc144 -

jonatack commented at 4:05 pm on December 14, 2020: member

Code review re-ACK b316dcb758e3dbd302fbb5941a1b5b0ef5f1f207 per git range-diff b103fdcb 82d0a43 b316dcb, rebase only

luke-jr approved

luke-jr commented at 4:08 pm on December 14, 2020: member

utACK b316dcb758e3dbd302fbb5941a1b5b0ef5f1f207, only code movement from after verack check to before

MarcoFalke merged this on Dec 14, 2020

MarcoFalke closed this on Dec 14, 2020

jnewbery deleted the branch on Dec 14, 2020

DrahtBot locked this on Feb 15, 2022

net processing: Tolerate sendheaders and sendcmpct messages before verack #20599

Conflicts