Setting -blocksonly sets -maxmempool to zero.

jnewbery commented at 6:55 pm on January 17, 2017: member

Nodes running in -blocksonly mode do not send and receive transactions outside blocks. They do not have a mempool, so -maxmempool should be set to 0.

Unused mempool memory can be used for the UTXO coincache (PR #8610) so not setting -maxmempool to 0 can cause to coincache to grow larger than expected.

If -blocksonly is set and -maxmempool is set to anything other than 0, error and exit. If -blocksonly is set and -maxmempool is not set, implicitly set -maxmempool to 0.

I’ve also added an additional comment around the nMempoolSizeMin check in AppInitParameterInteraction() since it wasn’t immediately obvious to me what that check was doing.

Setting -blocksonly sets -maxmempool to zero.

Nodes running in -blocksonly mode do not send and receive transactions
outside blocks. They do not have a mempool, so -maxmempool should be set
to 0.

Unused mempool memory can be used for the UTXO coincache (PR #8610) so
not setting -maxmempool to 0 can cause to coincache to grow larger than
expected.

If -blocksonly is set and -maxmempool is set to anything other than 0,
error and exit. If -blocksonly is set and -maxmempool is not set,
implicitly set -maxmempool to 0.

2838d28f20

jnewbery force-pushed on Jan 17, 2017

gmaxwell commented at 11:48 pm on January 17, 2017: contributor

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)

fanquake added the label Mempool on Jan 18, 2017

pstratem commented at 4:06 pm on January 18, 2017: contributor

The intended behavior is for nodes in blocks only mode to have a mempool because of whitelisted peers.

NACK

On Jan 17, 2017 4:48 PM, “Gregory Maxwell” notifications@github.com wrote:

Whitelisted peers can still relay transactions for blocksonly, I suspect this would break that.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9569#issuecomment-273338305, or mute the thread https://github.com/notifications/unsubscribe-auth/AAl4QxnJy-KrmMlFU_e6Trkgj7BTg3alks5rTVNRgaJpZM4LmBxS .

sipa commented at 4:12 pm on January 18, 2017: member

Interesting, I was not aware.

How do we reconcile these things? Someone without whitelisted peers who sets -blocksonly will not have a mempool, and can reasonably expect that there won’t ne an extra 300 MB added to their dbcache.

Even in cases where whitelisted peers exist, I doubt a full 300 MB (if -maxmempool was not changed) will be used in commom cases.

Maybe -blocksonly should just disable the mempool/dbcache share, as whatever mempool remains under these circumstances can’t be a typical always-full mempool?

jnewbery commented at 7:03 pm on January 18, 2017: member

@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 commented at 11:46 pm on January 18, 2017: contributor

Transaction relay to whitelisted peers ignores fRelay

On Jan 18, 2017 12:03 PM, “John Newbery” notifications@github.com wrote:

@gmaxwell https://github.com/gmaxwell @pstratem https://github.com/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 https://github.com/gmaxwell @pstratem https://github.com/pstratem can you give some use cases? Are people relying on this behaviour?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9569#issuecomment-273568508, or mute the thread https://github.com/notifications/unsubscribe-auth/AAl4Q4_0uz6blDzMpNtW2J0BGQrbL7XMks5rTmIEgaJpZM4LmBxS .

jnewbery commented at 0:20 am on January 19, 2017: member

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

theuni commented at 1:39 am on January 19, 2017: member

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: https://github.com/theuni/bitcoin/commit/f1966109d17035b814a03599e5f222825056457a .

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.

jnewbery commented at 3:05 pm on January 19, 2017: member

@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 https://github.com/theuni/bitcoin/commit/f1966109d17035b814a03599e5f222825056457a and the approach looks generally good.

pstratem commented at 8:38 pm on January 21, 2017: contributor

Nodes in blocks only mode will accept transactions sent to them.

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

The only way to achieve what you want is to dynamically resize the cache based on available memory.

On Jan 18, 2017 5:20 PM, “John Newbery” notifications@github.com wrote:

@pstratem https://github.com/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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9569#issuecomment-273644406, or mute the thread https://github.com/notifications/unsubscribe-auth/AAl4Q1VKp8FTHJof-6PIhoCxB5JZlROxks5rTqxagaJpZM4LmBxS .

jnewbery commented at 6:30 pm on January 23, 2017: member

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:

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

(https://github.com/bitcoin/bitcoin/blob/727a79836035b02e82e31a3c941396146eabc207/src/net_processing.cpp#L258-L259)

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

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

(https://github.com/bitcoin/bitcoin/blob/727a79836035b02e82e31a3c941396146eabc207/src/net_processing.cpp#L1242-L1243).

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

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

(https://github.com/bitcoin/bitcoin/blob/727a79836035b02e82e31a3c941396146eabc207/src/net_processing.cpp#L3017)

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:

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

sipa commented at 6:50 pm on January 23, 2017: member

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

jnewbery commented at 7:17 pm on January 23, 2017: member

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

jnewbery commented at 0:46 am on January 28, 2017: member

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!

gmaxwell commented at 2:54 am on January 29, 2017: contributor

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.

laanwj commented at 11:53 am on June 26, 2017: member

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?

jnewbery commented at 8:18 pm on June 26, 2017: member

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

jnewbery closed this on Jun 26, 2017

jnewbery commented at 3:35 pm on June 9, 2019: member

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

MarcoFalke locked this on Dec 16, 2021

Setting -blocksonly sets -maxmempool to zero. #9569