p2p: Remove -feefilter option

amadeuszpawlik commented at 4:13 pm on May 18, 2021: contributor

net: Remove -feefilter option, as it is debug only and isn’t used in any tests. Checking this option for every peer on every iteration of the message handler is unnecessary, as described in #21545. refactor: Move feefilter logic out into a separate MaybeSendFeefilter(...) function to improve readability of the already long SendMessages(...). fixes #21545

The configuration option -feefilter has been added in 9e072a6e66efbda7d39bf61eded21d2b324323be: “Implement “feefilter” P2P message” According to the BIP133, turning the fee filter off was ment for:

[…] a node […] using prioritisetransaction to accept transactions whose actual fee rates might fall below the node’s mempool min fee [in order to] disable the fee filter to make sure it is exposed to all possible txid’s

-feefilter was subsequently set as debug only in #8150, with the motivation that the help message was too difficult to translate.

fanquake added the label P2P on May 18, 2021

amadeuszpawlik force-pushed on May 18, 2021

amadeuszpawlik marked this as ready for review on May 18, 2021

DrahtBot commented at 2:38 am on May 19, 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:

#21841 (Send fewer feefilter messages (avoid the wobbling number issue) by rebroad)

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.

in src/net_processing.cpp:4756 in b7311edce9 outdated

4782-            // until scheduled broadcast, then move the broadcast to within MAX_FEEFILTER_CHANGE_DELAY.
4783-            else if (current_time + MAX_FEEFILTER_CHANGE_DELAY < pto->m_tx_relay->m_next_send_feefilter &&
4784-                     (currentFilter < 3 * pto->m_tx_relay->lastSentFeeFilter / 4 || currentFilter > 4 * pto->m_tx_relay->lastSentFeeFilter / 3)) {
4785-                pto->m_tx_relay->m_next_send_feefilter = current_time + GetRandomDuration<std::chrono::microseconds>(MAX_FEEFILTER_CHANGE_DELAY);
4786-            }
4787+            MaybeSendFeefilter(*pto, current_time);

jnewbery commented at 8:54 am on May 19, 2021:

I’d suggest also moving the conditional logic into the MaybeSendFeeFilter() function. The compiler will inline the function anyway, so there’s no difference in the compiled code, and keeping all the feefilter logic together makes it easier to understand. Suggested diff:

 0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index cc9e38afbb..2acf4c2d62 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -4226,6 +4226,12 @@ void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, std::chrono::microseconds c
 5 {
 6     AssertLockHeld(cs_main);
 7 
 8+    if (m_ignore_incoming_txs) return;
 9+    if (!pto.m_tx_relay) return;
10+    if (pto.GetCommonVersion() < FEEFILTER_VERSION) return;
11+    // peers with the forcerelay permission should not filter txs to us
12+    if (pto.HasPermission(PF_FORCERELAY)) return;
13+
14     CAmount currentFilter = m_mempool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFeePerK();
15     static FeeFilterRounder g_filter_rounder{CFeeRate{DEFAULT_MIN_RELAY_TX_FEE}};
16 
17@@ -4745,16 +4751,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
18         if (!vGetData.empty())
19             m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData));
20 
21-        //
22-        // Message: feefilter
23-        //
24-        if (pto->m_tx_relay != nullptr &&
25-            !m_ignore_incoming_txs &&
26-            pto->GetCommonVersion() >= FEEFILTER_VERSION &&
27-            !pto->HasPermission(PF_FORCERELAY) // peers with the forcerelay permission should not filter txs to us
28-        ) {
29-            MaybeSendFeefilter(*pto, current_time);
30-        }
31+        MaybeSendFeefilter(*pto, current_time);
32     } // release cs_main
33     return true;
34 }

amadeuszpawlik commented at 2:24 pm on May 19, 2021:

Many thanks! I agree, it’s much cleaner. I applied your suggestion (after rebase, PF_FORCERELAY is now NetPermissionFlags::ForceRelay).

jnewbery commented at 8:55 am on May 19, 2021: member

Concept ACK.

Thanks for picking this up, and welcome to the project, @amadeuszpawlik!

DrahtBot added the label Needs rebase on May 19, 2021

amadeuszpawlik force-pushed on May 19, 2021

laanwj commented at 2:46 pm on May 19, 2021: member

net: Remove -feefilter option, as it is debug only and isn’t used in any tests

Not a NACK by any means (I don’t know the entire history here) but fwiw, “debug only” doesn’t necessarily mean test-only. Sometimes obscure options that only make sense in relatively rare conditions, or during development, are labeled as such as well.

Remove -feefilter option

Feefilter option is debug only and it isn't used in any tests, it's wasteful
to check this option for every peer on every iteration of the message handler
loop. refs #21545

c0385f10a1

amadeuszpawlik force-pushed on May 19, 2021

jonatack commented at 2:59 pm on May 19, 2021: member

It looks like this configuration option was added by @morcos in 9e072a6e66ef: “Implement “feefilter” P2P message. The “feefilter” p2p message is used to inform other nodes of your mempool min fee which is the feerate that any new transaction must meet to be accepted to your mempool. This will allow them to filter invs to you according to this feerate.”

Per https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2016-February/012449.html, its goal was to help reduce unnecessary network traffic.

in src/net_processing.cpp:4243 in b0b62138b7 outdated

4238+    if (m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
4239+        // Received tx-inv messages are discarded when the active
4240+        // chainstate is in IBD, so tell the peer to not send them.
4241+        currentFilter = MAX_MONEY;
4242+    } else {
4243+        static const CAmount MAX_FILTER{g_filter_rounder.round(MAX_MONEY)};

jonatack commented at 3:06 pm on May 19, 2021:

static const MAX_FILTER seems to only be used in the following line; maybe use it directly in the conditional.

0-        static const CAmount MAX_FILTER{g_filter_rounder.round(MAX_MONEY)};
1-        if (pto.m_tx_relay->lastSentFeeFilter == MAX_FILTER) {
2-            // Send the current filter if we sent MAX_FILTER previously
3+        if (pto.m_tx_relay->lastSentFeeFilter == g_filter_rounder.round(MAX_MONEY)) {
4+            // Send the current filter if we previously sent the maximum filter of MAX_MONEY

amadeuszpawlik commented at 4:12 pm on May 19, 2021:

Thanks for the input - this further improves the readability 8741245857b588d73a3f21489470f04bae31e67b

jnewbery commented at 8:37 am on May 20, 2021:

FeeFilterRounder::round() is not a pure function - it calls to FastRandomContext::rand32() and so will return different values even when called with the same input. Therefore this is a change in behaviour.

In general, we shouldn’t mix behaviour changes with refactors in the same PR. It may be the right change, but it should be left for a separate PR, leaving this PR as a pure refactor.

jonatack commented at 9:27 am on May 20, 2021:

Interesting – thanks @jnewbery (I don’t know how I missed that it was being memoized but thanks for catching my oversight!)

amadeuszpawlik commented at 11:34 am on May 20, 2021:

Thanks @jnewbery for catching that. Removed that commit.

DrahtBot removed the label Needs rebase on May 19, 2021

jonatack commented at 9:22 am on May 20, 2021: member

As a follow-up to my message above, I was curious why -feefilter was turned into a debug message and found #8150. IIUC, a translator found the help message too difficult to translate.

amadeuszpawlik force-pushed on May 20, 2021

jnewbery commented at 3:14 pm on May 20, 2021: member

Code review ACK b0b62138b7b9e27ced562fe4859184f4d9bb94c5

MarcoFalke renamed this:
~~Feefilter cleanups~~
Remove -feefilter option
on May 20, 2021

MarcoFalke renamed this:
~~Remove -feefilter option~~
p2p: Remove -feefilter option
on May 20, 2021

in src/net_processing.cpp:335 in b0b62138b7 outdated

330@@ -331,6 +331,9 @@ class PeerManagerImpl final : public PeerManager
331     /** Send `addr` messages on a regular schedule. */
332     void MaybeSendAddr(CNode& node, std::chrono::microseconds current_time);
333 
334+    /** Send `feefilter` message. */
335+    void MaybeSendFeefilter(CNode& node, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main);;

ccdle12 commented at 11:43 am on May 24, 2021:

minor nit: extra semi-colon at the end of method declaration

amadeuszpawlik commented at 1:00 pm on May 24, 2021:

fixed; thanks for catching that

amadeuszpawlik force-pushed on May 24, 2021

Factor feefilter logic out

Break SendMessages() function into smaller units to improve readability.
Adds lock assert, as `round()` isn't thread safe.
closes #21545

a7a43e8fe8

amadeuszpawlik force-pushed on May 24, 2021

jnewbery commented at 1:05 pm on May 24, 2021: member

Code review ACK a7a43e8fe85f6247c35d7ff99f36448574f3e34a

jonatack commented at 1:17 pm on May 24, 2021: member

Given #21992 (comment) and #21992 (comment) (@amadeuszpawlik, I think that information should have been in the PR description and could still be added), are we certain that this option is unused, unneeded, and should be removed?

jnewbery commented at 1:25 pm on May 24, 2021: member

I agree with @sipa’s comment (https://github.com/bitcoin/bitcoin/issues/8150#issuecomment-223967868):

I don’t think there are good reasons for disabling this in production

This debug only option was added with the original implementation for feefilter, and was used for debugging and testing. It’s not used in any of our functional tests, so I’m pretty sure that it’s safe to remove.

jonatack commented at 2:18 pm on May 24, 2021: member

Yes, grepping for DEBUG_ONLY there seem to be some that aren’t used in tests but are intended for users, so I’m not sure about “as it is debug only and isn’t used in any tests” as a rationale, but it’s more of a question than an assertion and I was hoping to hear discussion about it. My informal understanding was more along the lines of #21992 (comment).

promag commented at 11:43 pm on May 24, 2021: member

Code review ACK a7a43e8fe85f6247c35d7ff99f36448574f3e34a.

This breaks existing configs but I guess a release note isn’t worth it.

MarcoFalke commented at 6:35 am on May 25, 2021: member

Maybe this was added as an emergency switch to turn off feefilter, should an unexpected integration bug arise in production? Though, now that the feefilter message is being used for years already without such an observed issue, it seems fine to remove.

Generally I am not a fan of undocumented non-self-documenting code. And absent anyone speaking up with a use case (or documentation or test of that use case), this seems fine to remove.

MarcoFalke commented at 6:41 am on May 25, 2021: member

review ACK a7a43e8fe85f6247c35d7ff99f36448574f3e34a 🦁

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK a7a43e8fe85f6247c35d7ff99f36448574f3e34a 🦁
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUh9ywwAwrzAr/Ix4u2TvqUIReJZMThgVWOWl89TTvTVRMCq0w8p+4mcTpdzhIJz
 8Kdmisf/S1tcgEN6Gtzi82116WgJpwmwQ/nFDB5x9/+chTt45jYu47Ncz6XlUL+2p
 9OjKTauw8dUfygvxDDCx8xXCczzjR0AHX0xyezm0rt6eNFKs1vUhuqe5azl9MCUv/
10/eO+yofKqaWQGsRYRZuOGH6ixqw/EMbSIE7WaeI1A/wDQj8ziQHrvmJ8LF+PBFMw
11Htw6fFW52g2y5rZ+lz7lBdPgD2vRTIPOCtwEfH5EzGI3LMSqfqZDYn5P4nc0fe8v
12fMBp57dVZ8GC9+p+G6jsjCvmeOSTRgnwpKe4PA8gmFxrtmtfR6+sz8aseVT6PmKc
13v3LttgBpPfcEvSxjIVeozTr3a0WkXoZst7UOgkm8hz3tdbDBVMZV/Jw5i9HysJdP
146OJZE5BZrhG1Q8yXhuNkfmMzon2v9/D2IxXuQRMPZWnS/n8kfPaTEYlalUPFZtLM
15D2F1Bz3z
16=1aDA
17-----END PGP SIGNATURE-----

Timestamp of file with hash 461767b35c3b59f11465e878607b812ba3d35fa9a0f66207e25d262a8da4e456 -

MarcoFalke merged this on May 25, 2021

MarcoFalke closed this on May 25, 2021

amadeuszpawlik deleted the branch on May 25, 2021

sidhujag referenced this in commit 071064777d on May 25, 2021

gwillen referenced this in commit 916944ed6e on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

p2p: Remove -feefilter option #21992

Conflicts