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

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.

Concept ACK

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

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.

ACK f72a62418c2c617ebe19e59f855f3bec1da9ee15

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.

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.

rebased

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

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

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?

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

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

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

ACK 427f5736f174de5dfd60ad3fbbcdcbd498a2f30a

Lightly tested with blockfilterindex and peerbloomfilters enabled.

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.

<!--4a62be1de6b64f3ed646cdc7932c8cf5-->

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

rebased

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

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

Code review ACK 82d0a43

rebased

review ACK b316dcb758e3dbd302fbb5941a1b5b0ef5f1f207 📒

<details><summary>Show signature and timestamp</summary>

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK b316dcb758e3dbd302fbb5941a1b5b0ef5f1f207 📒
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjFTQv+MjBurQNjNB/sPp5ynOe9ZPZqoLsSdpWQK3Mco7MIGVe0IuCiXO9EEwNK
n6olAJEkeeze+rtrCig8m55yptQLnQy/Us4U5XrAUHCDqTGy+Uja6m1zt7YZJkvK
SLoQa1SJCtqdmN9r4X3Ejeeat/nVDWRFkrKRaUHxj7E4+jq871h31mjX9eNc2CB4
qwJQP/ZDK4oA0vzJuxqoU48Fs7HCXyk4XnXEm8hPLBmRLogJSF5B6V8+wULc++NT
2b5kUKBBQydqvDrzbOUPHs6QvnkoHflYq6b00BeQsdIy85GiT0TMZwi9G925cpf4
xwIwgEDaEL2GhLpjnKS5BlHyjECC6TarPfPdVaLC9rU45hjmY/PktgK3oJiUEO0o
1TfjFNRHJYkiIz7CuUlcQOWbEwurQeXKx4Kmuy0EpFgK5Z5H1HlILKB8Uo/cSIEB
dvGUZW2kODHCLEab2znPbGEhYej73gBgGiAp1o0MIDshYAW7DdYeNVQ5GC4UDph9
SrTxzuXa
=MU7a
-----END PGP SIGNATURE-----

Timestamp of file with hash 4d415a967d56551b5e8c98d1933644ad7c3b68b61fba3333c1eab1b112bdc144 -

</details>

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

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