Whitelisted peers can still relay transactions for blocksonly, I suspect this would break that. (FWIW I don't like that whitelisting does stuff like that, but its what it does and people depend on it)

@gmaxwell @pstratem Thank you. I wasn't aware of this behaviour.

As far as I can tell, this can only happen to a -blocksonly node as follows:

• the node connects to a peer and sends a version message. The 'relay' field is set to FALSE (see PushNodeVersion() in net_processing.cpp - bitcoin core always sets relay to FALSE if -blocksonly is set since the global fRelayTxes = !blocksonly).
• the peer disregards the relay field. Note that bitcoin core respects the relay field that it receives (see ProcessMessage() in net_processing.cpp.
• the peer sends INVs/TXs, which the node accepts because -whitelistrelay is set.

so this can only happen if the node is connecting to a non-bitcoin-core peer which is disregarding the semantics of the relay field in the version message.

@gmaxwell @pstratem can you give some use cases? Are people relying on this behaviour?

@pstratem Maybe I'm misunderstanding, but I would expect the only way for transactions to enter the node's mempool is for transactions to be relayed from peers. That can't happen if blocksonly is set, unless that peer is violating the relay field in our version message.

Of course, we can send transactions to peers. That's true without a mempool, and is unaffected by this PR.

You seem to understand this better than me - can you explain where I'm wrong.

Somewhat unrelated to the discussion here, but I've been meaning to get rid of the fRelayTxes global for a while. I just hacked this up, and I think it makes reasoning about this much easier: theuni@f196610 .

It was done quickly and probably isn't quite right, but I'll clean it up and PR separately if the approach is agreed upon.

@theuni I agree that we should get rid of the global to make reasoning easier. I also think we need additional documentation here to make the intended behaviour more explicit (ie that blocksonly doesn't mean only blocks if you have some other setting!).

Please tag me when you have a PR for the relay/accept transactions split so I can review it. I've had a look at theuni@f196610 and the approach looks generally good.

Nodes which have whitelisted a blocks only peer will send them transactions.

I don't think this is true. When a blocksonly node connects to a peer, it sends the version header with the relay field set to false:

   connman.PushMessage(pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, PROTOCOL_VERSION, (uint64_t)nLocalNodeServices, nTime, addrYou, addrMe,
            nonce, strSubVersion, nNodeStartingHeight, ::fRelayTxes));

(

The peer receives that version message and sets fRelayTxes to false for that CNode. Any whitelist settings are not considered here:

            if (!vRecv.empty())
                vRecv >> pfrom->fRelayTxes; // set to true after we get the first filter* message

(

When the peer is sending messages to the blocksonly node, it checks the fRelayTxes field and explicitly clears all txs from its INV messages:

    if (!pto->fRelayTxes) pto->setInventoryTxToSend.clear();

(

I've run some tests locally, and they seem to confirm that bitcoin nodes will not send transactions to blocksonly peers, regardless of whitelist settings.

Furthermore, setting -blocksonly soft sets -walletbroadcast to false, so the blocksonly node won't even broadcast transactions it originates (which is sensible behaviour - nodes that broadcasts just their own transactions are terrible for transaction privacy).

It seems to me that the only ways to get transactions into a blocksonly node's mempool are to:

1. connect to it with a non-compliant node which ignores the relay field in the version message; or
2. explicitly set walletbroadcast to true and give up any transaction privacy.

Maybe it's easier to just not add the unused mempool space to the available dbcache when blocksonly is set?

@sipa - it's certainly true that what you're suggesting is possible. It's the approach that I started with, but changed tack later because it was a far smaller, cleaner code change to simply set mempool size to zero.

I'm happy to revert to my original approach, but at the very least we should understand and document the behaviour that @pstratem is talking about. From my testing and code-reading, it doesn't seem possible that blocksonly nodes would ever have anything in their mempool. I also don't understand the use-case.

Pinging @pstratem @gmaxwell . Can you help me understand what the use case here, so we can either:

• document the blocksonly/whitelist behaviour that you're talking about; or
• move forward with this PR.

Thanks!

Whitelisting was originally added so that armory user's mandatory Bitcoin nodes would still be able to reliably relay transactions when the armory software, which ignores frelaytxes, behind them wanted the transactions relayed. I have been told that other commercial custom wallet code depend on this whitelist always relays behavior (which is too bad, because I dislike it and think we never should have done it that way).

The reason for this is because otherwise if your 'wallet' is implemented as [bitcoin core] - [custom stuff] there is no way for your custom stuff to implement a rebroadcast like functionality similar to that of the integrated wallet.

Blocksonly isn't even a document option yet, because it has no servicebits significance so nodes could avoid having all their automatic outbound peers be blocks only and ending up unable to relay transactions. It will be at least one BIP and two releases from being a documented option for that reason (unless the thinking changes), so I really don't think it matters how we handle its memory usage vs dbcache for now.

Looks like this is stuck/controversial. I think it's a good point that -blocksonly is an "undocumented" developer-only option, so interaction with other options isn't very important.
Should we close?

Yes - closing for now. This should be revisited when blocksonly is made a non-debug option.

-blocksonly is no longer a hidden option (#15990)