Consensus: Move CFeeRate out of libconsensus #9279

pull jtimon wants to merge 2 commits into bitcoin:master from jtimon:0.13-consensus-dust-out-minimal changing 23 files +124 −96
  1. jtimon commented at 5:45 AM on December 5, 2016: contributor

    CTxOut and CTransaction shouldn't depend on CFeeRate, which is something for policy checks and not consensus checks. This replaces #7820

  2. fanquake added the label Consensus on Dec 5, 2016
  3. jtimon commented at 6:39 PM on December 9, 2016: contributor

    Mhmm, this fails on travis without qt but strangely it seems to work locally (ubuntu) all:

    ./configure --without-gui
./configure --disable-wallet --without-gui
./configure --disable-wallet
  4. dcousens commented at 11:49 PM on December 9, 2016: contributor

    concept ACK

  5. sipa commented at 11:51 PM on December 9, 2016: member

    Concept ACK

  6. jtimon force-pushed on Dec 13, 2016
  7. jtimon force-pushed on Dec 13, 2016
  8. jtimon commented at 8:31 PM on December 13, 2016: contributor

    Rebased. Changing #include <stdlib.h> for #include <stdint.h> as suggested by ryanofsky_ on IRC solved the issue.

  9. jtimon force-pushed on Jan 25, 2017
  10. jtimon force-pushed on Jan 31, 2017
  11. jtimon commented at 7:51 PM on January 31, 2017: contributor

    Rebased, @morcos mentioned the possibility of keep passing the CFeeRate to the new function, but after separating dustRelayFee, I'm not sure how he feels about it.

  12. laanwj commented at 6:37 PM on February 2, 2017: member

    Concept ACK. This doesn't belong in transaction.h, or consensus code at all.

  13. dcousens approved
  14. dcousens commented at 1:00 AM on February 3, 2017: contributor

    utACK

  15. NicolasDorier commented at 4:47 AM on February 3, 2017: contributor

    Concept ACK. I would prefer that IsDust get an optional parameter to setup the dust FeeRate.

    A reason to not use the same global everywhere is if we decide to have a Dust FeeRate higher at the wallet level. I detailed the reason at https://github.com/bitcoin/bitcoin/issues/7677

  16. jtimon force-pushed on Feb 3, 2017
  17. jtimon commented at 5:17 AM on February 3, 2017: contributor

    Changed to maintain CFeeRate param in GetDustThreshold/IsDust. And rebased without need, why not?

  18. in src/qt/coincontroldialog.cpp:None in 9e7f477872 outdated 
     432 | @@ -433,8 +433,7 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog)
 433 |          {
 434 |              CTxOut txout(amount, (CScript)std::vector<unsigned char>(24, 0));
 435 |              txDummy.vout.push_back(txout);
 436 | -            if (txout.IsDust(dustRelayFee))
 437 | -               fDust = true;
 438 | +            fDust = IsDust(txout, ::dustRelayFee);
    NicolasDorier commented at 5:33 AM on February 3, 2017:

    should change to fDust =| IsDust(txout, ::dustRelayFee); to maintain the logic.

  19. jtimon force-pushed on Feb 9, 2017
  20. jtimon commented at 9:31 PM on February 9, 2017: contributor

    Fixed @NicolasDorier 's great catch.

  21. in src/primitives/transaction.h:None in 4e124f8ea7 outdated 
       6 | @@ -7,6 +7,7 @@
   7 |  #define BITCOIN_PRIMITIVES_TRANSACTION_H
   8 |  
   9 |  #include "amount.h"
  10 | +#include "policy/feerate.h"
    theuni commented at 11:51 PM on February 9, 2017:

    Not necessary, I assume? Kinda defeats the purpose :)

  22. morcos commented at 4:52 PM on February 10, 2017: member

    @jtimon what do you think about removing the factor of 3 in the formula, and then just increasing the dustRelayTxFee in the code to 3000 sat/KB. Seems unnecessarily complicated as is.

  23. jtimon commented at 2:37 AM on February 11, 2017: contributor

    I think your suggestion makes sense, but I would really try to avoid scope creep here. I've been wanting to move this for very long...(see #5114 ). Seems like a simple change to do afterwards.

  24. jtimon force-pushed on Feb 11, 2017
  25. jtimon commented at 2:53 AM on February 11, 2017: contributor

    Fixed @theuni 's nit.

    I was going to add @morcos ' suggestion too, but I'm not sure how to rephrase this:

    "Fee rate (in %s/kB) used to defined dust, the value of an output such that it will cost about 1/3 of its value in fees at this fee rate to spend it." (init.cpp) The comment on DUST_RELAY_TX_FEE delcaration (policy/policy.h) would probably need some modifications too.

  26. jtimon force-pushed on Apr 4, 2017
  27. jtimon commented at 4:09 PM on April 4, 2017: contributor

    Needed rebase

  28. in src/policy/feerate.h:2 in c32f28b402 outdated 
       0 | @@ -0,0 +1,54 @@
   1 | +// Copyright (c) 2009-2010 Satoshi Nakamoto
   2 | +// Copyright (c) 2009-2015 The Bitcoin Core developers
    jtimon commented at 4:18 PM on April 4, 2017:

    Auto-nit: s/2015/2017/ also maybe remove the line with satoshi?

  29. jtimon force-pushed on Apr 5, 2017
  30. jtimon commented at 11:08 PM on April 5, 2017: contributor

    Rebased to make travis run again because

    E: Failed to fetch http://us-central1.gce.archive.ubuntu.com/ubuntu/pool/universe/w/wine1.6/wine1.6_1.6.2-0ubuntu4_amd64.deb  503  Service Unavailable [IP: 23.236.60.84 80]
...
  31. jtimon force-pushed on Apr 18, 2017
  32. jtimon commented at 7:33 PM on April 18, 2017: contributor

    Needed rebase

  33. NicolasDorier commented at 3:03 AM on April 19, 2017: contributor

    utACK 11ba99fcb6f5bcf64342f61c9c580d70873933e2

  34. jtimon force-pushed on Apr 20, 2017
  35. jtimon commented at 10:43 PM on April 20, 2017: contributor

    needed trivial rebase (6)

    EDIT: I was also removing includes to amount.h that I didn't need to

  36. jtimon force-pushed on Apr 20, 2017
  37. sipa commented at 12:18 AM on April 21, 2017: member

    utACK 57c109a2f92decfc407e1205e247668c84b411e2. I'm very happy to see those dust functions mode out of CTransaction.

  38. Consensus: Minimal way to move dust out of consensus 330bb5a456
  39. Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module
    ...from amount.o to policy/feerate.o

Policy, because it moves policy code to the policy directory (common module)
    381a46e38f
  40. jtimon force-pushed on May 3, 2017
  41. jtimon commented at 4:01 PM on May 3, 2017: contributor

    needed trivial rebase (7)

  42. in src/policy/policy.cpp:18 in 330bb5a456 outdated 
      14 | @@ -15,6 +15,43 @@
  15 |  
  16 |  #include <boost/foreach.hpp>
  17 |  
  18 | +CAmount GetDustThreshold(const CTxOut& txout, const CFeeRate& dustRelayFee)
    ryanofsky commented at 9:48 PM on May 8, 2017:

    In commit "Consensus: Minimal way to move dust out of consensus"

    In IsDust and GetDustThreshold, it might be good to give the dustRelayFee arguments a name that doesn't shadow the global ::dustRelayFee variable.

    paveljanik commented at 9:13 AM on May 9, 2017:

    Yes, this nit was not handled before merge so current master:

    policy/policy.cpp:18:63: warning: declaration shadows a variable in the global namespace [-Wshadow]
CAmount GetDustThreshold(const CTxOut& txout, const CFeeRate& dustRelayFee)
                                                              ^
./policy/policy.h:101:17: note: previous declaration is here
extern CFeeRate dustRelayFee;
                ^
policy/policy.cpp:50:50: warning: declaration shadows a variable in the global namespace [-Wshadow]
bool IsDust(const CTxOut& txout, const CFeeRate& dustRelayFee)
                                                 ^
./policy/policy.h:101:17: note: previous declaration is here
extern CFeeRate dustRelayFee;
                ^
2 warnings generated.
  43. ryanofsky commented at 10:11 PM on May 8, 2017: member

    utACK 381a46e38fa9262c868e5fee9ed0d1f71af4059e. The two commits seem like they could be separate PRs. But they are both good cleanups.

  44. laanwj merged this on May 9, 2017
  45. laanwj closed this on May 9, 2017
  46. laanwj referenced this in commit 08a7316c14 on May 9, 2017
  47. jtimon deleted the branch on May 30, 2017
  48. markblundeberg referenced this in commit 7cb2307fdf on May 23, 2019
  49. markblundeberg referenced this in commit a83e457a8f on May 23, 2019
  50. PastaPastaPasta referenced this in commit 386883e7cb on Jun 10, 2019
  51. PastaPastaPasta referenced this in commit 68014b3ca3 on Jun 10, 2019
  52. PastaPastaPasta referenced this in commit 47a96b6aa7 on Jun 10, 2019
  53. jonspock referenced this in commit cfb4dfce57 on Jun 13, 2019
  54. PastaPastaPasta referenced this in commit 46f96686e0 on Jun 19, 2019
  55. PastaPastaPasta referenced this in commit 47cf476cf7 on Jun 19, 2019
  56. PastaPastaPasta referenced this in commit fd60ab0cb2 on Jun 19, 2019
  57. PastaPastaPasta referenced this in commit 9e36fcb278 on Jun 19, 2019
  58. PastaPastaPasta referenced this in commit f2971473f9 on Jun 19, 2019
  59. PastaPastaPasta referenced this in commit 413ba9b806 on Jun 19, 2019
  60. PastaPastaPasta referenced this in commit dba6bf57d7 on Jun 19, 2019
  61. PastaPastaPasta referenced this in commit 93436270f4 on Jun 20, 2019
  62. PastaPastaPasta referenced this in commit 5003445e24 on Jun 22, 2019
  63. PastaPastaPasta referenced this in commit fae2af39e4 on Jun 22, 2019
  64. PastaPastaPasta referenced this in commit 0685795c42 on Jun 22, 2019
  65. PastaPastaPasta referenced this in commit a6b4ebca59 on Jun 22, 2019
  66. PastaPastaPasta referenced this in commit f3fb007451 on Jun 24, 2019
  67. PastaPastaPasta referenced this in commit dffade9edf on Jun 26, 2019
  68. PastaPastaPasta referenced this in commit 39cfd61c07 on Jun 26, 2019
  69. jtoomim referenced this in commit a9b3a1e660 on Jun 29, 2019
  70. jtoomim referenced this in commit 1aea2acb9c on Jun 29, 2019
  71. barrystyle referenced this in commit 2cb78e63de on Jan 22, 2020
  72. random-zebra referenced this in commit 676f6a79bf on Nov 6, 2020
  73. DrahtBot locked this on Sep 8, 2021
Contributors
jtimondcousenssipalaanwjNicolasDoriertheunimorcosryanofskypaveljanik
Labels
Consensus
Linked (view graph)
#7820 Consensus: Policy: Move CFeeRate out of consensus module and create CPolicy interface#10431 Prevent shadowing the global dustRelayFee
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-05-03 06:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me