p2p: Don't send FEEFILTER in blocksonly mode.

mzumsande commented at 11:49 pm on March 22, 2021: member

The purpose of FEEFILTER messages (BIP 133) is to inform our peers that we do not want transactions below a specified fee rate. In blocksonly mode, we do not want our peer to send us any transactions at all (and will disconnect if a peer still sends a transaction INV or TX).

Therefore, I don’t think that it makes sense to send FEEFILTER messages every 10 minutes on average in blocksonly mode - this PR disables it.

Note that on block-relay-only connections, FEEFILTER is already disabled, just not in blocksonly mode.

fanquake added the label P2P on Mar 22, 2021

in src/net_processing.cpp:4692 in c2aa9d004c outdated

4688@@ -4689,7 +4689,8 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
4689         //
4690         // Message: feefilter
4691         //
4692-        if (pto->m_tx_relay != nullptr && pto->GetCommonVersion() >= FEEFILTER_VERSION && gArgs.GetBoolArg("-feefilter", DEFAULT_FEEFILTER) &&
4693+        if (pto->m_tx_relay != nullptr && !m_ignore_incoming_txs && pto->GetCommonVersion() >= FEEFILTER_VERSION &&

MarcoFalke commented at 6:41 am on March 23, 2021:

It might be good to split each && into its own line, to clarify that each condition needs to be fulfilled independently

mzumsande commented at 6:00 pm on March 23, 2021:

Done.

MarcoFalke commented at 6:47 am on March 23, 2021: member

Interesting. My first reaction was to suggest that they send the MAX_FILTER once, so that feefilter could slowly replace the fRelay mechanism to enable/disable tx relay. See also #21327

Though, no need to rework that in this pull request. Seems fine to adjust this for consistency with block-relay-only.

DrahtBot commented at 10:28 am on March 23, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21506 (p2p, refactor: make NetPermissionFlags an enum class by jonatack)
#21160 (Net/Net processing: Move tx inventory into net_processing by jnewbery)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ghost commented at 10:58 am on March 23, 2021: none

Concept ACK

Bitcoin Core follows BIP 133: feefilter messages

BIP 133: Add a new message, “feefilter”, which serves to instruct peers not to send “inv”’s to the node for transactions with fees below the specified fee rate.

-blocksonly: To only sync blocks with other peers, you can disable transaction relay

glozow commented at 2:40 pm on March 23, 2021: member

My first reaction was to suggest that they send the MAX_FILTER once, so that feefilter could slowly replace the fRelay mechanism to enable/disable tx relay.

Is it possible to replace exactly? Disabling tx relay using fRelay means you disconnect them for sending you a tx, but it’s ok if they ignore a fee filter, right?

MarcoFalke commented at 2:49 pm on March 23, 2021: member

Yeah, we’d have to disconnect on feefilter violations (at least MAX_FITLER ones). There might be a large overlap between peers that ignore fRelay (and get disconnected) with peers that ignore feefilter, but I haven’t collected data on this.

glozow commented at 3:34 pm on March 23, 2021: member

Yeah, we’d have to disconnect on feefilter violations (at least MAX_FITLER ones). There might be a large overlap between peers that ignore fRelay (and get disconnected) with peers that ignore feefilter, but I haven’t collected data on this.

Just curious. Have you seen #20895 (comment)?

MarcoFalke commented at 5:21 pm on March 23, 2021: member

I did and I agree. I was hypothesising if the set of peers that ignore fRelay is identical to the set of nodes that ignore MAX_FILTER, then there is practically no difference whether they are disconnected due to ignoring either.

mzumsande commented at 5:49 pm on March 23, 2021: member

This discussion drifted away from the scope of this PR a bit - but what is the motivation behind the idea of replacing the fRelay mechanism by a MAX_FILTER feefilter?

p2p: Don't send FEEFILTER in blocksonly mode

It is unnecessary to send FEEFILTER messages when we don't accept
transactions from our peers.

18a9b27dd6

mzumsande force-pushed on Mar 23, 2021

glozow commented at 9:36 pm on March 23, 2021: member

This discussion drifted away from the scope of this PR a bit - but what is the motivation behind the idea of replacing the fRelay mechanism by a MAX_FILTER feefilter?

Sorry about that 😅. AFAIK, fRelay was introduced as a VERSION field in BIP37 as a way to turn tx relay off until a bloom filter is sent. That means it’s not very flexible, since you can’t send multiple versions and we don’t use bloom filters anymore.

glozow commented at 9:44 pm on March 23, 2021: member

Separate from the fRelay convo… Concept ACK to having consistent behavior towards our block-relay-only peers as when we’re in blocksonly mode. I don’t see how there’d be any benefit of sending them fee filters right now 🤔

amitiuttarwar approved

amitiuttarwar commented at 11:13 pm on March 23, 2021: contributor

code review ACK 18a9b27dd6

I agree there is no point of sending a FEEFILTER message when we’d just disconnect the peer for sending a transaction.

I wonder if we should also skip processing the message if we are in -blocksonly mode, but the processing is pretty trivial, so doesn’t seem like it would matter much.

You could add a small test for this behavior, something like this in p2p_feefilter.py:

0def test_blocksonly(self):
1	self.log.info("Check that we don't send feefilter messages when we start in blocks only mode")
2    self.restart_node(0, ["-blocksonly"])
3	self.nodes[0].add_p2p_connection(FeefilterConn()).assert_feefilter_received(False)

I didn’t look too closely at the timing logic to ensure there wouldn’t be a race / false positive, but it failed on master & passed on your branch.

jnewbery commented at 1:12 pm on March 24, 2021: member

utACK 18a9b27dd68dc9044a82fba0802b8cb5c68d10ce

I think the whole fee filter sending logic could be cleaned up substantially:

remove the -feefilter option, which is debug only and isn’t used in any of our tests. Seems wasteful to check this option for every peer on every iteration of the message handler loop.
factor this logic out into its own function. The compiler will inline it anyway, but it helps readability to break the very long SendMessages() function into smaller units.
move the logic out of the cs_main lock scope. I don’t think there’s anything in there that requires cs_main.

Obviously out of scope for this PR.

glozow commented at 6:51 pm on March 24, 2021: member

tested ACK 18a9b27dd68dc9044a82fba0802b8cb5c68d10ce using this. Looks like we didn’t have a test for block-relay-only either.

[test] no send feefilters when txrelay is turned off beead33a21

mzumsande commented at 9:13 pm on March 25, 2021: member

I wonder if we should also skip processing the message if we are in -blocksonly mode, but the processing is pretty trivial, so doesn’t seem like it would matter much.

I had thought about that but decided against it, since sending transactions in -blocksonly mode is unusual but not impossible (sendrawtransaction as was recently discussed in IRC, and I think we’d also answer unsolicited GETDATAs). In that case, it would be nice to also respect our peers’ feefilters.

You could add a small test for this behavior, something like this in p2p_feefilter.py:

tested ACK 18a9b27 using this. Looks like we didn’t have a test for block-relay-only either.

Thanks for the suggestion - I cherry-picked @glozow’s test commit and added it to the PR.

glozow commented at 10:14 pm on March 25, 2021: member

re ACK beead33a21483c4901e76799ba5c8f7d6fdfffe9 🙂 thanks for adding the test!

amitiuttarwar commented at 5:15 am on March 26, 2021: contributor

reACK beead33a21483c4901e76799ba5c8f7d6fdfffe9

I had thought about that but decided against it, since sending transactions in -blocksonly mode is unusual but not impossible (sendrawtransaction as was recently discussed in IRC, and I think we’d also answer unsolicited GETDATAs). In that case, it would be nice to also respect our peers’ feefilters.

yeah, great point :)

MarcoFalke commented at 7:08 am on March 26, 2021: member

remove the -feefilter option, which is debug only and isn’t used in any of our tests. Seems wasteful to check this option for every peer on every iteration of the message handler loop.

ACK, seems redundant with forcerelay

move the logic out of the cs_main lock scope. I don’t think there’s anything in there that requires cs_main.

round isn’t thread-safe, so it needs a lock. See

0src/policy/fees.h-    /** Quantize a minimum fee for privacy purpose before broadcast. Not thread-safe due to use of FastRandomContext */
1src/policy/fees.h-    CAmount round(CAmount currentMinFee);

MarcoFalke commented at 7:09 am on March 26, 2021: member

review ACK beead33a21483c4901e76799ba5c8f7d6fdfffe9

jnewbery commented at 9:54 am on March 29, 2021: member

reACK beead33a21483c4901e76799ba5c8f7d6fdfffe9

MarcoFalke merged this on Mar 29, 2021

MarcoFalke closed this on Mar 29, 2021

sidhujag referenced this in commit a53ef0f158 on Mar 29, 2021

mzumsande deleted the branch on Apr 12, 2021

DrahtBot locked this on Aug 18, 2022

p2p: Don’t send FEEFILTER in blocksonly mode. #21509

Conflicts