Checking the other implementations, btcsuite/btcd#1536 tagged as "low priority" is open in btcd to implement signet. I didn't see other signet proposals in bcoin, bitcoinj, or libbitcoin.

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.

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.

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?

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.

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:

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

or just:

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

(or both, even).

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

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.

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

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

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

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

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

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

Dropped commits 764042e and f26e5eb and updated 5866a3e based on the review feedback (thanks!)

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

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

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

Here's the buried discussion in that PR to add the disconnection: #20564 (comment). 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.

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.

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

review ACK 2327cad

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.

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.

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.

ACK 346b0ec

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

Conflicts

No conflicts as of last run.

Rebased.

re-ACK 06d47d9

Thanks for the ACKs. Rebased.

re-ACK e1e6714

ACK e1e6714