A minor issue of SENDCMPCT msg in handshaking process #18390

In src/net_processing.cpp line 2131 to 2143

if (pfrom->nVersion >= SHORT_IDS_BLOCKS_VERSION) {
    bool fAnnounceUsingCMPCTBLOCK = false;
    uint64_t nCMPCTBLOCKVersion = 2;
    if (pfrom->GetLocalServices() & NODE_WITNESS)
        connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion));
    nCMPCTBLOCKVersion = 1;
    connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion));
}

Can anyone explain why the node sends one more SENDCMPCT msg with nCMPCTBLOCKVersion = 1 when pfrom->GetLocalServices() & NODE_WITNESS is true?

There is a lot of related discussion in the PR introducing this behaviour along with a change to the corresponding BIP-151.

More specifically, the updated BIP says:

Upon connection establishment, a node SHOULD send a burst of sendcmpct messages containing every version of compact block encodings for which they are willing to support sending cmpctblock and blocktxn messages, and receiving getblocktxn messages. These messages SHOULD be ordered in the order of the priority which the node wishes to receive cmpctblock/blocktxn messages, with the highest-priority version sendcmpct message sent first.

I believe that this is done to enable the possible future where SegWit-supporting nodes (NODE_WITNESS) to participate in both v1 and v2 compact block relay, and this would allow current nodes to talk to them (by not implying that SegWit is always v2-cmpctblocks).

To be clear, I just spent about 30 minutes trying to figure out, and I'm still unsure that my rationale is correct. Maybe this means we should add a comment.

There is a comment already:

https://github.com/bitcoin/bitcoin/blob/5bf45fe2a9642f8ae8f8a12bcbf8f8b4770421ad/src/net_processing.cpp#L2132

But I don't understand why we still support non-witness compact blocks these days. To simplify our code, we could just remove those things?

@MarcoFalke yes, that comment is there, but it doesn't make clear why it is useful to announce cmpctblocks-v1 for segwit nodes. The confusing part is that it's definitely not useful if everybody uses current implementation, so one have to think about either past or future scenarios, where this could be useful via compatibility.

I accidentally clicked on the 'close issue' :). I understand that sending one more SENDCMPCT msg for some nodes that only support version 1.

I still have something to discuss.

Without considering the version (1 or 2), I just understand the meaning of nCMPCTBLOCKVersion determines whether this node wants to send or receive witness data. This is because some nodes (supporting version 2) may not want to receive witness data. Also, I think fWantsCmpctWitness has the same role as nCMPCTBLOCKVersion.

// ! Whether this peer wants witnesses in cmpctblocks/blocktxns
bool fWantsCmpctWitness;

But, in receiving SENDCMPCT part, https://github.com/bitcoin/bitcoin/blob/5bf45fe2a9642f8ae8f8a12bcbf8f8b4770421ad/src/net_processing.cpp#L2215 If a peer sends any SENDCMPCT msg with nCMPCTBLOCKVersion == 2, there is no logic to change fWantsCmpctWitness to False even if the peer sends SENDCMPCT msg with nCMPCTBLOCKVersion == 1.

Is there anything I misunderstand?

You are correct, this is done to follow this part of BIP-152:

These messages SHOULD be ordered in the order of the priority which the node wishes to receive cmpctblock/blocktxn messages, with the highest-priority version sendcmpct message sent first.

Notice that upgrade from v1 to v2 is also not allowed by this code (because it will stick to v1 which is received first).

What is possible for future implementations of the node is to ignore v2 for some reason, and stick to v1, even if v1 is received later.

It appears that Bitcoin Core no longer sends multiple versions of the SENDCMPT message:

$ rg NetMsgType::SENDCMPCT
src/net_processing.cpp
1220:                m_connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION));
1227:        m_connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetCommonVersion()).Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/true, /*version=*/CMPCTBLOCKS_VERSION));
3418:            m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION));
3454:    if (msg_type == NetMsgType::SENDCMPCT) {

src/protocol.cpp
77:    NetMsgType::SENDCMPCT,

...and when we do, we only specify V2 by sending CMPCTBLOCKS_VERSION:

/** The compactblocks version we support. See BIP 152. */
static constexpr uint64_t CMPCTBLOCKS_VERSION{2};

In addition to this, we now ony support V2 compact blocks:

https://github.com/bitcoin/bitcoin/blob/f088949fcfe6ba5000caa2f6adc6803e81925afb/src/net_processing.cpp#L3454-L3460

Therefore I think this issue can be closed now.