Remove -feefilter option #21583

Removed the -feefilter option from src/net_processing.cpp. #21545 To make sure everything works, I ran make check and then ran test_runner.py with p2p_feefilter.py

$ test/functional/test_runner.py p2p_feefilter.py
Temporary test directory at /tmp/test_runner_₿_🏃_20210403_190347
Running Unit Tests for Test Framework Modules
..........
----------------------------------------------------------------------
Ran 10 tests in 0.633s

OK
Remaining jobs: [p2p_feefilter.py]
1/1 - p2p_feefilter.py passed, Duration: 15 s

TEST             | STATUS    | DURATION

p2p_feefilter.py | ✓ Passed  | 15 s

ALL              | ✓ Passed  | 15 s (accumulated) 
Runtime: 15 s

Regarding splitting up the feefilter logic and moving it to a seperate function for better readability, the g_filter_rounder.round() function is not thread safe and is currently inside a LOCK(cs_main); called at the begining of PeerManagerImpl::SendMessages(CNode* pto). Didn't make any changes for this, how would I proceed for this?

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #21506 (p2p, refactor: make NetPermissionFlags an enum class by jonatack)

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.

To completely remove the -feefilter option you'll want to remove other uses (grep -feefilter), the surrounding unused variables, and the user option in init.cpp.

In terms of extracting the logic into a separate function, one option you can use is an assert to make sure cs_main is held before entering the main function logic e.g. AssertLockHeld(cs_main) like used here.

@ccdle12 understood the usage of AssertLockHeld(cs_main). In #21509 @jnewbery and @MarcoFalke talk about

1. removing the -feefilter option ( as @glozow mentioned I have looked through all usages of 'feefilter')
2. factoring out the feefilter logic out into its own function (will do so with AssertLockHeld(cs_main) ).

From what I understand about the goal of #21545 , was to remove the feefilter check from the message handler loop in net_processing.cpp. Wouldn't removing it from the init.cpp be problematic as it would entirely remove the option? Or is removing feefilter completely the goal? I would like some clarity on this.

Feefilter usage

Looked around and pulled up files with refrences to feefilter and understood the context of the refrence which was a good exercise that got me familiar with the codebase.

1. init.cpp - argsman.AddArg("-feefilter", ...)
2. net_processing.cpp
3. net.cpp and net.h

if (m_tx_relay != nullptr) {
    stats.minFeeFilter = m_tx_relay->minFeeFilter;
} else {
    stats.minFeeFilter = 0;
}

4. protocol.cpp and protocol.h

const char *FEEFILTER="feefilter";
...
const static std::string allNetMessageTypes[] = {
    ...
    NetMsgType::FEEFILTER,
};

5. validation.h static const bool DEFAULT_FEEFILTER = true;
6. version.h static const int FEEFILTER_VERSION = 70013;
7. fees.cpp and fees.h explicit FeeFilterRounder(const CFeeRate& minIncrementalFee); (the non-thread safe round function)
8. net.cpp

RPCResult::Type::NUM, "minfeefilter", "The minimum fee rate for transactions this peer accepts"
...
static RPCHelpMan getpeerinfo() {
    ...
    obj.pushKV("minfeefilter", ValueFromAmount(stats.minFeeFilter));
}

9. policy_fee_tests.cpp FeeFilterRounder fee_rounder{CFeeRate{1000}};
10. fees.cpp FeeFilterRounder fee_filter_rounder{minimal_incremental_fee};
11. process_message.cpp FUZZ_TARGET_MSG(feefilter);
12. Multiple python tests in test/functional a. p2p_feefilter.py b. p2p_ibd_txrelay.py c. p2p_leak.py d. rpc_net.py e. test_runner.py f. messages.py g. p2p.py

is removing feefilter completely the goal

Yes. The -feefilter config option is not used anywhere else, so it should be removed from the init code.

Looked around and pulled up files with refrences to feefilter and understood the context of the refrence which was a good exercise that got me familiar with the codebase...

All of these instances of minFeeFilter, FEEFILTER, FEEFILTER_VERSION, etc are separate from the -feefilter option, which is used to control whether we'll ever send feefilter messages to our peer.

Please also add rationale to the commit log. Why is it ok to remove -feefilter?

Closing for now due to inactivity. Let me know when you want to work on this again.