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:

    0./configure --without-gui
1./configure --disable-wallet --without-gui
2./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: 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: 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

    0E: 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]
1...
  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 0: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:

     0policy/policy.cpp:18:63: warning: declaration shadows a variable in the global namespace [-Wshadow]
 1CAmount GetDustThreshold(const CTxOut& txout, const CFeeRate& dustRelayFee)
 2                                                              ^
 3./policy/policy.h:101:17: note: previous declaration is here
 4extern CFeeRate dustRelayFee;
 5                ^
 6policy/policy.cpp:50:50: warning: declaration shadows a variable in the global namespace [-Wshadow]
 7bool IsDust(const CTxOut& txout, const CFeeRate& dustRelayFee)
 8                                                 ^
 9./policy/policy.h:101:17: note: previous declaration is here
10extern CFeeRate dustRelayFee;
11                ^
122 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


jtimon dcousens sipa laanwj NicolasDorier theuni morcos ryanofsky paveljanik

Labels
Consensus

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: 2024-12-19 00:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me