feefilter: Compute the absolute fee rather than stored rate #16507

pull instagibbs wants to merge 3 commits into bitcoin:master from instagibbs:feefilter_match_mempool changing 6 files +34 −21
  1. instagibbs commented at 5:24 PM on July 31, 2019: member

    This means we will use the rounding-down behavior in GetFee to match both mempool acceptance and wallet logic, with minimal changes.

    Fixes #16499

    Replacement PR for https://github.com/bitcoin/bitcoin/pull/16500

  2. instagibbs commented at 5:25 PM on July 31, 2019: member

    This can be tested by applying https://github.com/instagibbs/bitcoin/commit/4538d6bfd1bddcfb741f04dbcd61a4eb41162e8f which without this fix causes rpc_fundrawtransaction.py to stall out when feefilter starts rejecting wallet/mempool transactions.

  3. DrahtBot commented at 5:41 PM on July 31, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  4. DrahtBot added the label Mempool on Jul 31, 2019
  5. DrahtBot added the label P2P on Jul 31, 2019
  6. in src/txmempool.h:341 in 4063b84d55 outdated 
     338 | -    CFeeRate feeRate;
 339 | +    /** Fee of the transaction. */
 340 | +    CFeeRate fee;
 341 | +
 342 | +    /** Virtual size of the transaction. */
 343 | +    size_t vsize;
    naumenkogs commented at 3:11 PM on August 6, 2019:

    How is this variable supposed to be assigned?

    luke-jr commented at 12:54 AM on August 20, 2019:

    Yikes, that looks dangerous. What would happen if code without this PR got mixed in with the PR'd type?

    instagibbs commented at 1:44 PM on August 20, 2019:

    whoops! fee is supposed to be CAmount.

  7. naumenkogs commented at 8:25 PM on August 6, 2019: member

    utACK. Seems like a reasonable simple fix for the existing problem.

    Replacement PR for #165001

    The link doesn't work.

  8. instagibbs commented at 8:33 PM on August 6, 2019: member

    Oops, fixed link

  9. instagibbs force-pushed on Aug 20, 2019
  10. instagibbs force-pushed on Aug 20, 2019
  11. instagibbs commented at 2:00 PM on August 20, 2019: member

    @luke-jr fixed field type and made CFeeRate constructor explicit to avoid merge/backport errors.

  12. TheBlueMatt commented at 6:18 PM on August 20, 2019: member

    I can haz test? Looks like you managed to write a test for the issue? Should be included here, no? I'm confused if the changes to TxMempoolInfo are required - we're still converting to CFeeRate before the convserion.

  13. in src/net_processing.cpp:3823 in 051fbd877b outdated 
    3818 | @@ -3819,8 +3819,8 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
3819 |                      const uint256& hash = txinfo.tx->GetHash();
3820 |                      CInv inv(MSG_TX, hash);
3821 |                      pto->setInventoryTxToSend.erase(hash);
3822 | -                    if (filterrate) {
3823 | -                        if (txinfo.feeRate.GetFeePerK() < filterrate)
3824 | +                    if (filterrate > CFeeRate(0)) {
3825 | +                        if (txinfo.fee < filterrate.GetFee(txinfo.vsize))
    instagibbs commented at 6:24 PM on August 20, 2019:

    @TheBlueMatt we're using it directly here, not converting to feerate.

  14. instagibbs commented at 6:26 PM on August 20, 2019: member

    And yes I can write a test.

  15. instagibbs commented at 8:45 PM on August 21, 2019: member

    @TheBlueMatt Pushed a test change that causes fee filter test to stall out when fix is removed.

  16. DrahtBot added the label Needs rebase on Sep 7, 2019
  17. instagibbs force-pushed on Sep 9, 2019
  18. instagibbs commented at 2:03 PM on September 9, 2019: member

    rebased

  19. DrahtBot removed the label Needs rebase on Sep 9, 2019
  20. instagibbs force-pushed on Sep 9, 2019
  21. instagibbs commented at 3:20 PM on September 9, 2019: member

    kdiff ate some tab space or something, pushed fix

  22. in src/net_processing.cpp:3887 in af5f7d2190 outdated 
    3881 | @@ -3881,10 +3882,10 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
3882 |                      for (std::set<uint256>::iterator it = pto->m_tx_relay->setInventoryTxToSend.begin(); it != pto->m_tx_relay->setInventoryTxToSend.end(); it++) {
3883 |                          vInvTx.push_back(it);
3884 |                      }
3885 | -                    CAmount filterrate = 0;
3886 | +                    CFeeRate filterrate;
    jonatack commented at 11:48 AM on September 18, 2019:

    FWIW identical code at L3847 and L3885 (also the case before the changes touching this).

                        CFeeRate filterrate;
                    {
                        LOCK(pto->m_tx_relay->cs_feeFilter);
                        filterrate = CFeeRate(pto->m_tx_relay->minFeeFilter);
                    }
    jkczyz commented at 9:15 PM on October 2, 2019:

    In addition to removing code duplication, a helper would simplify this code, reduce dependencies on CNode internals, and prevent ever having an uninitialized fee rate. Consider (or similar):

    const CFeeRate filter_rate = pto->GetMinRelayFee();

    IMHO, continually making small improvements like this would go a long way in making the codebase more approachable. Speaking from experience, as it stands the codebase is quite daunting to new contributors.

    instagibbs commented at 1:49 PM on October 3, 2019:

    The p2p code is the most daunting part. Refactoring has its own dangers. Complete ACK on working to make it easier to understand!

  23. in src/net_processing.cpp:3915 in af5f7d2190 outdated 
    3911 | @@ -3911,7 +3912,8 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
3912 |                          if (!txinfo.tx) {
3913 |                              continue;
3914 |                          }
3915 | -                        if (filterrate && txinfo.feeRate.GetFeePerK() < filterrate) {
3916 | +                        if (filterrate > CFeeRate(0) &&
    jonatack commented at 11:51 AM on September 18, 2019:

    1. Some code comments would be great, particularly before the conditional here and line 3859 just above.

    2. With these changes the 2 conditionals appear to be equivalent now. Making them the same or extracting them to a well-named bool function might be a nice touch.

                            if (filterrate > CFeeRate(0)) {
                            if (txinfo.fee < filterrate.GetFee(txinfo.vsize)) {
                                continue;
                            }
                        }

                            if (filterrate > CFeeRate(0) &&
                            txinfo.fee < filterrate.GetFee(txinfo.vsize)) {
                            continue;
                        }
    instagibbs commented at 1:30 PM on September 18, 2019:

    Can you suggest some text? I'm not sure of the confusion.

    Also I'm not sure what you mean by (2)

    instagibbs commented at 7:03 PM on September 21, 2019:

    ah I see now, I'm just mimicking current style. If I need to rebase or something I'll make them match.

    jkczyz commented at 12:41 AM on October 3, 2019:

    Agreed regarding extracting into a well-named helper and/or commenting there. To me:

    1. It seems this code is to prevent sending transactions to a peers that won't relay it. Is that correct?
    2. It's not apparent why the check isn't made when the fee rate is zero. Is it as an optimization or some other reason?

    Arguably someone scanning this code shouldn't need to parse that particular logic. Or if they do then they could dive into the helper.

    That said, it's not clear to me why filterrate > CFeeRate(0) is needed. Won't GetFee return a zero amount when the rate is zero? Can this be eliminated or is it an optimization?

  24. jonatack commented at 11:56 AM on September 18, 2019: member

    ACK af5f7d2190023b0a9e2aaaf829b72043293c6879. Appears to be a good, low-complexity fix. Code review, build/tests pass (thanks for adding tests!), new tests fail without the changes and on current master cc1d7fd57c9e777f7fb6da6a488b18317fbcd2c2. Sorry for taking so long to review this. Feel free to ignore the nits below.

  25. naumenkogs commented at 1:09 PM on September 21, 2019: member

    utACK 6a9ee897dd3dc3edd286085d47cb00df08f6d93c.

    I would slightly prefer 3859 and 3915 to share the code-style (they do the same thing but in different fashion), but feel free to ignore.

  26. in src/net_processing.cpp:3859 in 6a9ee897dd outdated 
    3855 | @@ -3856,9 +3856,10 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
3856 |                          const uint256& hash = txinfo.tx->GetHash();
3857 |                          CInv inv(MSG_TX, hash);
3858 |                          pto->m_tx_relay->setInventoryTxToSend.erase(hash);
3859 | -                        if (filterrate) {
3860 | -                            if (txinfo.feeRate.GetFeePerK() < filterrate)
3861 | +                        if (filterrate > CFeeRate(0)) {
    kallewoof commented at 8:11 AM on September 27, 2019:

    μ-Nit: I would slightly prefer a IsNull() { return nSatoshisPerK == 0; } in CFeeRate and using that instead of instantiating CFeeRate()s whenever you check if the filter rate is set.

    instagibbs commented at 8:12 PM on September 30, 2019:

    will do if forced to rebase or change commits for more substantial reasons

    laanwj commented at 1:17 PM on October 1, 2019:

    maybe isZero instead of isNull? I always associate Null with pointers somehow, not with values, this could give confusion here.

    kallewoof commented at 2:41 AM on October 2, 2019:

    Either works for me. We use IsNull elsewhere (e.g. uint256, FlatFilePos, BlockHeader, PSBTInput, CScriptWitness, etc etc.)

    jkczyz commented at 8:57 PM on October 2, 2019:

    @kallewoof Those examples aren't really values in the sense of integral values that could support arithmetic operations. Rather they support invalid or null values via IsNull. For example, FlatFilePos::IsNull is not for the zero position but rather an invalid position object (i.e. nFile == -1). So IsZero seems more appropriate here since a zero fee rate represents a valid object.

    I'm indifferent as to whether the helper is needed, though.

    kallewoof commented at 12:30 PM on October 3, 2019:

    Same goes for uint256 I'd say, but I don't really have a strong opinion on the matter. (Consistency -> IsNull; intuitive -> IsZero.)

  27. kallewoof approved
  28. kallewoof commented at 8:15 AM on September 27, 2019: member

    utACK with an ignorable nit

  29. achow101 commented at 7:21 PM on October 2, 2019: member

    ACK af5f7d2190023b0a9e2aaaf829b72043293c6879

    Reviewed code and checked that the test fails without this change.

  30. jkczyz commented at 1:19 AM on October 3, 2019: contributor

    ACK af5f7d2

    Also checked that test fails without the change.

    My preference is to include some of the suggested refactorings as they would be beneficial in terms of improving code health and approachability.

  31. instagibbs commented at 1:44 PM on October 3, 2019: member

    @jkczyz your reading is correct. I was keeping the code as close as possible to the original logic, while fixing the bug. The previous logic asks if the fee is set(non-zero), so I mimicked that. p2p code is notoriously hard to get right and even harder to get reviewers for, so I just kept things as close as possible.

    Follow-on cleanups are welcome of course.

  32. ajtowns commented at 3:17 PM on October 3, 2019: member

    I think this no longer compiles after #16727 got merged?

  33. MarcoFalke commented at 3:44 PM on October 3, 2019: member

    If you rebase this, could you please do so on top of 8afa602f308ef003bb6893718eae1fe5a830690c, so that backports are simplified?

  34. MarcoFalke closed this on Oct 3, 2019
  35. MarcoFalke reopened this on Oct 3, 2019
  36. instagibbs commented at 3:45 PM on October 3, 2019: member

    I'll rebase it sure and fix some minimal nits.

  37. feefilter: Compute the absolute fee rather than stored rate to match mempool acceptance logic 8e59af55aa
  38. Disallow implicit conversion for CFeeRate constructor 6a51f79517
  39. modify p2p_feefilter test to catch rounding error eb7b781659
  40. instagibbs force-pushed on Oct 3, 2019
  41. instagibbs commented at 6:04 PM on October 3, 2019: member

    rebased and removed the non-essential conditional that is spawing all the IsNull/IsZero debate.

  42. TheBlueMatt commented at 9:48 PM on October 3, 2019: member

    utACK af5f7d2190023b0a9e2aaaf829b72043293c6879. Code changes are simple enough and test fails if you revert the fix.

  43. in src/policy/feerate.h:28 in 6a51f79517 outdated 
      24 | @@ -25,7 +25,7 @@ class CFeeRate
  25 |      /** Fee rate of 0 satoshis per kB */
  26 |      CFeeRate() : nSatoshisPerK(0) { }
  27 |      template<typename I>
  28 | -    CFeeRate(const I _nSatoshisPerK): nSatoshisPerK(_nSatoshisPerK) {
  29 | +    explicit CFeeRate(const I _nSatoshisPerK): nSatoshisPerK(_nSatoshisPerK) {
    promag commented at 10:38 PM on October 3, 2019:

    nit, add space before :.

  44. promag commented at 10:40 PM on October 3, 2019: member

    ACK eb7b78165966f2c79da71b993c4c4d793e37297f.

    In case you push again, commit "Disallow implicit conversion for CFeeRate constructor" could be the first commit.

  45. kallewoof commented at 1:57 AM on October 4, 2019: member

    rebased and removed the non-essential conditional that is spawing all the IsNull/IsZero debate.

    Still see a bunch of >/== CFeeRate(0) in the changes.

  46. ajtowns commented at 6:09 AM on October 4, 2019: member

    ACK eb7b78165966f2c79da71b993c4c4d793e37297f code review only

  47. instagibbs commented at 1:26 PM on October 4, 2019: member

    @kallewoof I only removed the redundant calls in net_processing.cpp because it does reduce readability and fixing is a net gain imo.

  48. naumenkogs commented at 4:57 PM on October 4, 2019: member

    utACK eb7b78165966f2c79da71b993c4c4d793e37297f

  49. achow101 commented at 5:08 PM on October 4, 2019: member

    re ACK eb7b78165966f2c79da71b993c4c4d793e37297f

  50. fanquake referenced this in commit 94d6a18f23 on Oct 4, 2019
  51. fanquake merged this on Oct 4, 2019
  52. fanquake closed this on Oct 4, 2019
  53. sidhujag referenced this in commit ec1754ad1a on Oct 4, 2019
  54. MarcoFalke commented at 6:04 PM on October 7, 2019: member

    post-merge

    ACK eb7b78165966f2c79da71b993c4c4d793e37297f

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK eb7b78165966f2c79da71b993c4c4d793e37297f
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgb3AwAh1Oi4PZcMIwCloxjHiSdLUKf/fEvb6wptxlDFYwLt9S8yI5uFKTVtYSg
K16HQxDHoHP/kAt8Ym3WFREQ+ib7BBiBrFG0xGKI2GukyThCrh2lcgGQGjCDUWlC
5V+n1K2xpar3UzD+ih7v515dQp6ve7TUXxybS3J1HcyIXVjIPgD1XGbEYi+CA/fp
rV52yAzVi0xqaHgKcjY4gqNW9F7hNGm6EFa8Xu3H8eaNFDbMA2fs+UY6kQdkMeaf
5e8YDUn58NM0yNR9X4rZ+baHWXqkF/r2y9SnW396kVvO5XiKWm2HIaYj7MzuF9Ur
HGOjeBPSO/zjsrOtpTwn+kV6pJ3rwkG7koMseSMB0zkwak1rNhDHU+Ryb5E5n7MG
8hBpIqtKJ+/3iEyjCqto21e79yN38Dub15bYjjRAb+doPbJJqDp9qxgWwudV8nxS
lOteMqZGpanPXfOp19bP6Nm7WnkPJ2358R0smPalrpNuJrolfziYVgFAIoq0zHTo
UUc9jGn0
=lpFy
-----END PGP SIGNATURE-----

    Timestamp of file with hash 8169fd75811d5d99dfe89e4d9d3d55e4528f00d1db035d132d66806155ce78a3 -

    </details>

  55. jasonbcox referenced this in commit 22362253b4 on Oct 26, 2020
  56. DrahtBot locked this on Dec 16, 2021
Contributors
instagibbsDrahtBotnaumenkogsluke-jrTheBlueMattjonatackjkczyzkallewooflaanwjachow101ajtownsMarcoFalkepromag
Labels
P2PMempool
Linked (view graph)
#16499 Fee(rate) mismatch between min relay and fee filter#16500 GetFee should round up to avoid undershooting feerate
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-17 09:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me