Disabling high-bandwidth compact block relay from pre-segwit nodes doesn't work #18460

In MaybeSetPeerAsAnnouncingHeaderAndIDs() we try to keep three high-bandwidth compact blocks peers by downgrading one from high-bandwidth to low-bandwidth, and promoting the peer that served us the most recent block to high-bandwidth:

        connman->ForNode(nodeid, [connman](CNode* pfrom){
            AssertLockHeld(cs_main);
            uint64_t nCMPCTBLOCKVersion = (pfrom->GetLocalServices() & NODE_WITNESS) ? 2 : 1;
            if (lNodesAnnouncingHeaderAndIDs.size() >= 3) {
                // As per BIP152, we only get 3 of our peers to announce
                // blocks using compact encodings.
                connman->ForNode(lNodesAnnouncingHeaderAndIDs.front(), [connman, nCMPCTBLOCKVersion](CNode* pnodeStop){
                    AssertLockHeld(cs_main);
                    connman->PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetSendVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/false, nCMPCTBLOCKVersion));
                    return true;
                });
                lNodesAnnouncingHeaderAndIDs.pop_front();
            }
            connman->PushMessage(pfrom, CNetMsgMaker(pfrom->GetSendVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/true, nCMPCTBLOCKVersion));
            lNodesAnnouncingHeaderAndIDs.push_back(pfrom->GetId());
            return true;
        });

Note that the SENDCMPCT that we send to the node we're downgrading has its nCMPCTBLOCKVersion set based on whether we're the node we're upgrading supports segwit or not.

Here's the pre-v0.14 code for handling SENDCMPCT:

    else if (strCommand == NetMsgType::SENDCMPCT)
    {
        bool fAnnounceUsingCMPCTBLOCK = false;
        uint64_t nCMPCTBLOCKVersion = 1;
        vRecv >> fAnnounceUsingCMPCTBLOCK >> nCMPCTBLOCKVersion;
        if (nCMPCTBLOCKVersion == 1) {
            LOCK(cs_main);
            State(pfrom->GetId())->fProvidesHeaderAndIDs = true;
            State(pfrom->GetId())->fPreferHeaderAndIDs = fAnnounceUsingCMPCTBLOCK;
        }
    }

so if the peer that we're upgrading supports segwit and the node we're trying to downgrade doesn't, then the SENDCMPCT we send to the downgrading node will be version 2 and will be ignored.

We can fix this by setting the version correctly in the inner connman->ForNode block, but at this point I think we should probably just not try to set high-bandwidth mode for non-segwit nodes.

Introduced in #8393.

Thoughts @TheBlueMatt @sipa ?

Making any non-segwit peers high-bandwidth seems like a mistake, as we wouldn't ever want their blocks anyway?

low priority. Closing