p2p: Remove -feefilter option #21992
Conversation
8f66447 to
b7311ed
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
Concept ACK.
Thanks for picking this up, and welcome to the project, @amadeuszpawlik!
b7311ed to
39088aa
Compare
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. |
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 bitcoin#21545
39088aa to
b0b6213
Compare
|
It looks like this configuration option was added by @morcos in 9e072a6: "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 See also BIP133: https://github.com/bitcoin/bips/blob/master/bip-0133.mediawiki |
|
As a follow-up to my message above, I was curious why |
8741245 to
b0b6213
Compare
|
Code review ACK b0b6213 |
b0b6213 to
d739625
Compare
Break SendMessages() function into smaller units to improve readability. Adds lock assert, as `round()` isn't thread safe. closes bitcoin#21545
d739625 to
a7a43e8
Compare
|
Code review ACK a7a43e8 |
|
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? |
|
I agree with @sipa's comment (#8150 (comment)):
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. |
|
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). |
|
Maybe this was added as an emergency switch to turn off feefilter, should an unexpected integration bug arise in production? Though, now that the 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. |
There was a problem hiding this comment.
review ACK a7a43e8 🦁
Show signature and timestamp
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
review ACK a7a43e8fe85f6247c35d7ff99f36448574f3e34a 🦁
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUh9ywwAwrzAr/Ix4u2TvqUIReJZMThgVWOWl89TTvTVRMCq0w8p+4mcTpdzhIJz
Kdmisf/S1tcgEN6Gtzi82116WgJpwmwQ/nFDB5x9/+chTt45jYu47Ncz6XlUL+2p
OjKTauw8dUfygvxDDCx8xXCczzjR0AHX0xyezm0rt6eNFKs1vUhuqe5azl9MCUv/
/eO+yofKqaWQGsRYRZuOGH6ixqw/EMbSIE7WaeI1A/wDQj8ziQHrvmJ8LF+PBFMw
Htw6fFW52g2y5rZ+lz7lBdPgD2vRTIPOCtwEfH5EzGI3LMSqfqZDYn5P4nc0fe8v
fMBp57dVZ8GC9+p+G6jsjCvmeOSTRgnwpKe4PA8gmFxrtmtfR6+sz8aseVT6PmKc
v3LttgBpPfcEvSxjIVeozTr3a0WkXoZst7UOgkm8hz3tdbDBVMZV/Jw5i9HysJdP
6OJZE5BZrhG1Q8yXhuNkfmMzon2v9/D2IxXuQRMPZWnS/n8kfPaTEYlalUPFZtLM
D2F1Bz3z
=1aDA
-----END PGP SIGNATURE-----
Timestamp of file with hash 461767b35c3b59f11465e878607b812ba3d35fa9a0f66207e25d262a8da4e456 -
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 longSendMessages(...). fixes #21545The configuration option
-feefilterhas been added in 9e072a6: "Implement "feefilter" P2P message"According to the BIP133, turning the fee filter off was ment for:
-feefilterwas subsequently set as debug only in #8150, with the motivation that the help message was too difficult to translate.