doc: refer to BIPs 339/155 in feature negotiation

jonatack commented at 10:37 pm on December 13, 2020: member

of wtxidrelay and addrv2/sendaddrv2, and add fSuccessfullyConnected doxygen documentation to clarify that it is set to true on VERACK.

jonatack commented at 10:40 pm on December 13, 2020: member

Checking the other implementations, https://github.com/btcsuite/btcd/pull/1536 tagged as “low priority” is open in btcd to implement signet. I didn’t see other signet proposals in bcoin, bitcoinj, or libbitcoin.

DrahtBot added the label P2P on Dec 14, 2020

ajtowns commented at 7:20 am on December 14, 2020: member

If we’re going to change the software, just making it ignore the message if received post-VERACK (for compatibility with earlier drafts of the BIP and hence rc2) seems simpler than adding special cases.

Deliberately disconnecting nodes for behaving in ways we know is not harmful seems very backwards to me.

MarcoFalke commented at 7:32 am on December 14, 2020: member

Tend toward NACK. There are 20.99 nodes on signet without addrv2 support and will keep a connection just fine. If your addrman is populated with those (or with rc3/final/master nodes), you won’t have any issues either. The issue #20637 is purely about a vanilla start with empty addrman and the only seed being the node that is run by @ajtowns or @kallewoof (I don’t remember). The solution is to update that node, not to introduce more branches in net processing.

Even if people want to go forward with patching the source code, the current patch is too broad. The only thing changes should be the disconnection, not the way how addrv2 messages are recognized.

ajtowns commented at 9:20 am on December 14, 2020: member

the current patch is too broad. The only thing changes should be the disconnection, not the way how addrv2 messages are recognized

This does only change the disconnection?

MarcoFalke commented at 9:27 am on December 14, 2020: member

This does only change the disconnection?

It will run pfrom.m_wants_addrv2 = true; on signet, which will not be run on mainnet. The whole goal of signet was to be exactly like mainet, except for the pow.

ajtowns commented at 9:59 am on December 14, 2020: member

Well, adding a test for the signet chain pretty much guarantees different behaviour on signet than mainnet… (And should probably be if (m_chainparams.GetConsensus().signet_blocks) anyway, if it were desirable)

I think just being deliberately compatible with (or at least tolerant of) implementations of earlier versions of the BIP would make much more sense, so either:

0if (pfrom->m_wants_addrv2) {
1    LogPrintf(BCLog::NET, "peer=%d sent SENDADDRV2 twice; disconnecting\n", pfrom->GetId());
2    pfrom->fDisconnect = 1;
3    return;
4}
5pfrom->m_wants_addrv2 = true;
6return;

or just:

0if (pfrom.fSuccessfullyConnected) return; // ignore sendaddrv2 received too late
1pfrom->m_wants_addrv2 = true;
2return;

(or both, even).

MarcoFalke commented at 10:15 am on December 14, 2020: member

Deliberately disconnecting nodes for behaving in ways we know is not harmful seems very backwards to me.

Tell that to your past self. The disconnection logic has been suggested by you for wtxid, and as such carried over to addv2 ;)

https://github.com/bitcoin/bitcoin/pull/18044/files#r446755938

ajtowns commented at 10:29 am on December 14, 2020: member

Deliberately disconnecting nodes for behaving in ways we know is not harmful seems very backwards to me.

Tell that to your past self. The disconnection logic has been suggested by you for wtxid, and as such carried over to addv2 ;)

https://github.com/bitcoin/bitcoin/pull/18044/files#r446755938

If there had already been nodes on the network sending wtxid relay mesages after verack, disconnecting them would have also been a bad idea.

jonatack commented at 10:29 am on December 14, 2020: member

should probably be if (m_chainparams.GetConsensus().signet_blocks)

Thanks! I was telling myself there was surely a better way of checking for it.

jonatack renamed this:
~~p2p: do not disconnect post-verack sendaddrv2 on signet~~
p2p: update behavior for sendaddrv2 after verack
on Dec 14, 2020

jonatack force-pushed on Dec 14, 2020

jonatack commented at 3:31 pm on December 14, 2020: member

Updated to propose each of the 3 suggestions that resolve #20637. Will remove/squash commits per reviewer preference.

in src/net_processing.cpp:2533 in f26e5ebc60 outdated

2530+            // Ignore peers that send a SENDADDRV2 message after VERACK.
2531+            return;
2532+        }
2533+        if (pfrom.m_wants_addrv2) {
2534+            // Disconnect peers that send SENDADDRV2 twice.
2535+            LogPrint(BCLog::NET, "peer=%d sent SENDADDRV2 twice; disconnecting\n", pfrom.GetId());

MarcoFalke commented at 3:38 pm on December 14, 2020:

This is not more harmful than an unknown message, so beside being dead code in practise, it is also not protecting against anything.

jonatack commented at 7:15 pm on December 14, 2020:

dropped

laanwj commented at 3:39 pm on December 14, 2020: member

~0 on this, I’d prefer to keep it as is if remotely possible, and not change mainnet logic for a temporary signet issue at the least.

Deliberately disconnecting nodes for behaving in ways we know is not harmful seems very backwards to me.

It’s not harmful but also, it’s unclear what to do in that case? Should we ignore the message? Should be back-patch addrv2 support into the existing connection?

The reason for adding the disconnection is that at verack time the protocol negotiation is finalized. This might allow for some kinds of optimizations, as well as knowing for sure whether a peer supports addrv1 or not at a clear point in time. This is useful if in the future there are plans to phase out addrv1.

DrahtBot added the label Needs rebase on Dec 14, 2020

luke-jr commented at 6:42 pm on December 14, 2020: member

PR title should describe the change (and mention Signet) please

DrahtBot commented at 7:12 pm on December 14, 2020: member

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

jonatack force-pushed on Dec 14, 2020

jonatack renamed this:
~~p2p: update behavior for sendaddrv2 after verack~~
p2p: do not disconnect post-verack sendaddrv2 on signet
on Dec 14, 2020

jonatack commented at 7:23 pm on December 14, 2020: member

Dropped commits 764042ecf74b778bd999961ba44d9648ae5bf4cb and f26e5ebc60ea2f41b959bc8b136719e250fe8761 and updated 5866a3ef2948945b15b936b572cae1a7321849a9 based on the review feedback (thanks!)

Will drop 5866a3ef2948945b15b936b572cae1a7321849a9 if #20648 is preferred. I tested #20648 and it resolves the issue as well.

Sorry for the temporary PR title, reinstated the original title.

DrahtBot removed the label Needs rebase on Dec 14, 2020

in src/net_processing.cpp:2777 in 5519482370 outdated

2556@@ -2557,8 +2557,10 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
2557     // BIP155 defines feature negotiation of addrv2 and sendaddrv2, which must happen
2558     // between VERSION and VERACK.
2559     if (msg_type == NetMsgType::SENDADDRV2) {
2560-        if (pfrom.fSuccessfullyConnected) {
2561+        if (pfrom.fSuccessfullyConnected && !m_chainparams.GetConsensus().signet_blocks) {
2562             // Disconnect peers that send a SENDADDRV2 message after VERACK.
2563+            // A temporary exception is made for bootstrapping signet support in v0.21.0.
2564+            // After 0.21.0, this can be reverted.
2565             pfrom.fDisconnect = true;

ajtowns commented at 8:01 pm on December 14, 2020:

Behaviour doesn’t match description – per the commit title, this should be:

0    if (pfrom.fSuccessfullyConnected) {
1        if (!m_chainparams.GetConsensus().signet_blocks) {
2            pfrom.fDisconnect = true;
3        }
4        return;
5    }

If you want the behaviour that’s implemented, should be described as “support sendaddrv2 after verack on signet only for compatibility with earlier bip drafts” or similar.

jonatack commented at 8:39 pm on December 14, 2020:

Thanks, good point, which @MarcoFalke also pointed out in #20646 (comment). Updated the implementation per your suggestion for signet behavior to be as close as possible to mainnet.

jonatack force-pushed on Dec 14, 2020

ajtowns commented at 8:53 pm on December 14, 2020: member

~0 on this, I’d prefer to keep it as is if remotely possible, and not change mainnet logic for a temporary signet issue at the least.

It’s not a signet issue; it’s a “nodes running #19954 (oct 11) can’t communicate with nodes running #20564 (dec 9)” issue. That happens to affect signet seriously, since signet was only merged a couple of weeks earlier (sep 21), so all existing signet nodes that haven’t updated in the past week are running code that’s incompatible with current master/rc3.

I thought the whole point of #20564 was to avoid disconnecting peers running other software unnecessarily, so I’m at a complete loss as to why we’d want to simultaneously introduce code that disconnects earlier versions of our own code, especially ones that have already been release candidates. This just makes absolutely no sense to me.

Deliberately disconnecting nodes for behaving in ways we know is not harmful seems very backwards to me.

It’s not harmful but also, it’s unclear what to do in that case? Should we ignore the message?

Yes. Ignoring messages is how we tolerate other nodes doing protocols we don’t support – in this case the unsupported protocol is the earlier version of sendaddrv2/bip155.

Should be back-patch addrv2 support into the existing connection?

That would be fine too – it’d be removing three lines of code instead of one – but it doesn’t seem particularly necessary. The idea isn’t that you have to support every protocol every other node uses, you just have to tolerate them as far as reasonably possible.

The reason for adding the disconnection is that at verack time the protocol negotiation is finalized. This might allow for some kinds of optimizations, as well as knowing for sure whether a peer supports addrv1 or not at a clear point in time. This is useful if in the future there are plans to phase out addrv1.

That’s the reason for finalising it before verack, and ignoring it afterwards; it’s not a reason for disconnecting peers who send it afterwards.

The reason for adding the disconnection is just to make it as obvious as possible for anyone developing new software and testing it against core that they’ve misimplemented the spec (ref)– they’ll test it, get an immediate disconnect, debug it, read the spec and go “oops” and fix it. Or at least that was the reasoning for disconnecting when receiving wtxidrelay late. And that’s free if you’ve got a new message for the feature that no one else has ever used before in an incompatible way. But while that was true for wtxidrelay, it’s not true for sendaddrv2. It’s obviously not true on signet, but it’s not true on mainnet either.

I mean, ultimately, it doesn’t matter – everyone running master/0.21rc’s released this past quarter will upgrade pretty soon, and if the upgrade’s a bit ugly, that’s survivable too. But keeping our p2p graph well connected is pretty important, and this just seems… way too carefree for something that important is the politest phrasing I can come up with.

jonatack commented at 9:04 pm on December 14, 2020: member

Here’s the buried discussion in that PR to add the disconnection: #20564 (review). I was in favor of aligning BIPs 155 and 339 on this, and their implementations, as it seemed clean and convenient, unless there is a good reason not to–which this issue may be.

MarcoFalke commented at 7:39 am on December 15, 2020: member

I’m at a complete loss as to why we’d want to simultaneously introduce code that disconnects earlier versions of our own code, especially ones that have already been release candidates.

master isn’t considered to be stable and shouldn’t be used by anyone except developers. rcs might be considered more stable, but we never promised to support rcs that have been followed up by other rcs or point releases. rcs should only be used to do initial testing. We shouldn’t assume that people download rc1 and run it in production for years.

I don’t think it matters much if we ignore the message or disconnect. The only reason I am NACK on this pull is that it is adding signet specific code for an issue that has nothing to do with signet fundamentally.

sipa commented at 7:51 am on December 15, 2020: member

I also dislike adding signet-specific exception code here.

As far as disconnecting for late-term sendaddrv2 goes, I think the discussion has been primarily between (1) accepting and acting upon sendaddrv2 at any point and (2) rejecting it entirely if it arrives too late. The possibility of just ignoring it if it arrives too late also exists, and doesn’t incur the risk of needing to forever support clients requesting it late either.

Conceptually, I think it makes sense to disconnect connections whenever a peer asks us something we cannot provide, and we have no less invasive way of signalling that. E.g. we disconnect a peer that asks for a block we don’t have, because it’s the polite thing to do - otherwise they’d keep waiting for us.

I don’t think that really applies here. sendaddrv2 means “I understand addrv2 and prefer it”, but it’s only a minor disservice to a peer to not obey it. That’s not the same as wtxid relay, which specifies that transaction announcements must use wtx - in that case there is no way we can still service the peer’s request.

I don’t think it’s a disaster to disconnect on late sendaddrv2, as it is a violation of the protocol spec. But there also isn’t a very strong reason for it.

jonatack force-pushed on Dec 15, 2020

jonatack commented at 10:59 am on December 15, 2020: member

Dropped 5866a3ef2948945b15b936b572cae1a7321849a9 as #20637 is now resolved (and I agree that the fixes that arrived after this one are preferable).

In light of the discussion, replaced it with 2327cadff6685ec353f33f409c0693310af2baa3 that ignores post-verack sendaddrv2 messages rather than disconnecting, and updated the PR title/description. Feel free to suggest closing this as well.

jonatack renamed this:
~~p2p: do not disconnect post-verack sendaddrv2 on signet~~
p2p: ignore post-verack sendaddrv2 instead of disconnecting
on Dec 15, 2020

sipa commented at 6:30 pm on December 15, 2020: member

utACK 2327cadff6685ec353f33f409c0693310af2baa3

ajtowns commented at 10:26 pm on December 15, 2020: member

ACK 2327cadff6685ec353f33f409c0693310af2baa3 – seems to behave reasonably for peers that send sendaddrv2 before or after verack

laanwj commented at 12:23 pm on December 17, 2020: member

Right, the solution to ignore seems most reasonable to me if we don’t disconnect for it.

review ACK 2327cadff6685ec353f33f409c0693310af2baa3

jnewbery commented at 1:51 pm on December 17, 2020: member

What’s the deployment plan? That this is included in an v0.21 rc4? If not, it seems a bit pointless. By the time it’s included in v0.21.1 then all of the problematic old nodes will presumably have upgraded to v0.21 and the disconnect won’t be a problem any more.

jonatack commented at 2:01 pm on December 17, 2020: member

Yes, I reckoned rc4 / 0.21.0 (or drop the commit).

jnewbery commented at 2:15 pm on December 17, 2020: member

ok, I’m ±0. The code seems fine and safe but I don’t see a huge benefit to making this change.

The benefit of a polite disconnect is pointed out by @ajtowns above - it’s immediately obvious to implementers that they’ve done something wrong if they send a sendaddrv2 after verack, whereas with this change it’s a bit harder to troubleshoot.

sipa commented at 6:38 pm on December 17, 2020: member

Yes, I was thinking 0.21.0, or not at all (if not, we could reassess by the time 0.21.1 is done to see if there is still a use for it, but I suspect not).

MarcoFalke commented at 8:32 pm on December 17, 2020: member

review ACK 2327cadff6685ec353f33f409c0693310af2baa3

The code looks correct. However, the only software affected by this is Bitcoin Core pre-release versions. All version that are affected by the incompatibility won’t be made compatible with this change. This simply changes incompatibility by disconnection to incompatibility by not sending addrv2. Disconnection makes it easier to spot incorrectly implemented new software, because not everyone reads the debug log or even has -debug=net enabled. So longer term, if this gets merged, it might also be good to revert it back to disconnection.

Sjors commented at 2:59 pm on December 30, 2020: member

Concept meh. A quick disconnect indeed seems more developer friendly, all things equal.

I also run signet seed nodes at v0.21.0rc3, which I’m happy to update at the next release candidate.

Code review ACK 2327cad (the first documentation commit is worth merging in any case)

This needs a 0.21 milestone.

Let’s also clarify in net.h that fSuccessfullyConnected implies VERACK has been sent or received.

jonatack force-pushed on Jan 3, 2021

jonatack renamed this:
~~p2p: ignore post-verack sendaddrv2 instead of disconnecting~~
doc: refer to BIPs 339/155 in feature negociation
on Jan 3, 2021

jonatack commented at 1:08 pm on January 3, 2021: member

The change to ignore rather than disconnect might be safer wrt p2p network connectivity for the launch of 0.21, but we seem to be mitigated here, so I’ve dropped the commit unless there is consensus to bring it back.

Let’s also clarify in net.h that fSuccessfullyConnected implies VERACK has been sent or received.

Done.

Sjors commented at 3:33 pm on January 3, 2021: member

ACK 346b0ec

jnewbery renamed this:
~~doc: refer to BIPs 339/155 in feature negociation~~
doc: refer to BIPs 339/155 in feature negotiation
on Jan 3, 2021

in src/net_processing.cpp:2771 in 346b0ecf31 outdated

2553@@ -2555,10 +2554,11 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
2554         return;
2555     }
2556 
2557+    // BIP155 defines feature negotiation of addrv2 and sendaddrv2, which must happen

theStack commented at 10:50 am on January 5, 2021:

I found the first part of this sentence a bit confusing, as it suggests (or could be interpreted as) that both “addrv2” and “sendaddrv2” are features to be negotiated. Maybe something like “… defines feature negotiation of addrv2 with a new message sendaddrv2, which …”?

jonatack commented at 6:37 pm on January 7, 2021:

Thanks for catching the French spelling slipping into the English :smiley:; fixed the commit message.

BIP155 refers to both addrv2 and sendaddrv2 as messages and doesn’t use the word “feature”, so I think the current wording is ok but happy to consider a new/better proposal.

theStack commented at 11:04 am on January 5, 2021: member

Concept ACK, referring to BIPs makes the life of potential code readers easier

nit: in the commit message, s/negociation/negotiation/

DrahtBot commented at 5:50 am on January 6, 2021: member

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

Conflicts

No conflicts as of last run.

felipsoarez approved

DrahtBot added the label Needs rebase on Jan 7, 2021

jonatack force-pushed on Jan 7, 2021

jonatack commented at 6:38 pm on January 7, 2021: member

Rebased.

DrahtBot removed the label Needs rebase on Jan 7, 2021

Sjors commented at 5:14 pm on January 8, 2021: member

re-ACK 06d47d9928b0e5e3ec93d603c106b757b2618ca5

DrahtBot added the label Needs rebase on Jan 29, 2021

doc: refer to BIPs 339/155 in feature negotiation

and add fSuccessfullyConnected doxygen documentation
to clarify that it is set to true on VERACK

e1e6714832

jonatack force-pushed on Feb 2, 2021

jonatack commented at 1:50 pm on February 2, 2021: member

Thanks for the ACKs. Rebased.

DrahtBot removed the label Needs rebase on Feb 2, 2021

jonatack closed this on Feb 5, 2021

jonatack reopened this on Feb 5, 2021

laanwj commented at 10:15 am on February 5, 2021: member

re-ACK e1e67148321cff0de9eb5e63d2604f05c12e69d1

laanwj merged this on Feb 5, 2021

laanwj closed this on Feb 5, 2021

jnewbery commented at 10:41 am on February 5, 2021: member

ACK e1e67148321cff0de9eb5e63d2604f05c12e69d1

jonatack deleted the branch on Feb 5, 2021

DrahtBot locked this on Aug 16, 2022

doc: refer to BIPs 339/155 in feature negotiation #20646

Conflicts