Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consensus: Move CFeeRate out of libconsensus #9279

Merged
merged 2 commits into from May 9, 2017

Conversation

Projects
None yet
10 participants
@jtimon
Copy link
Member

commented Dec 5, 2016

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

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Dec 9, 2016

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
@dcousens

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2016

concept ACK

@sipa

This comment has been minimized.

Copy link
Member

commented Dec 9, 2016

Concept ACK

@jtimon jtimon force-pushed the jtimon:0.13-consensus-dust-out-minimal branch 2 times, most recently Dec 13, 2016

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2016

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

@jtimon jtimon force-pushed the jtimon:0.13-consensus-dust-out-minimal branch 2 times, most recently Jan 25, 2017

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2017

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.

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

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

@dcousens
Copy link
Contributor

left a comment

utACK

@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Feb 3, 2017

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 #7677

@jtimon jtimon force-pushed the jtimon:0.13-consensus-dust-out-minimal branch Feb 3, 2017

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Feb 3, 2017

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

src/qt/coincontroldialog.cpp Outdated
@@ -433,8 +433,7 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog)
{
CTxOut txout(amount, (CScript)std::vector<unsigned char>(24, 0));
txDummy.vout.push_back(txout);
if (txout.IsDust(dustRelayFee))
fDust = true;
fDust = IsDust(txout, ::dustRelayFee);

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Feb 3, 2017

Member

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

@jtimon jtimon force-pushed the jtimon:0.13-consensus-dust-out-minimal branch Feb 9, 2017

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2017

Fixed @NicolasDorier 's great catch.

src/primitives/transaction.h Outdated
@@ -7,6 +7,7 @@
#define BITCOIN_PRIMITIVES_TRANSACTION_H

#include "amount.h"
#include "policy/feerate.h"

This comment has been minimized.

Copy link
@theuni

theuni Feb 9, 2017

Member

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

@morcos

This comment has been minimized.

Copy link
Member

commented Feb 10, 2017

@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.

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2017

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.

@jtimon jtimon force-pushed the jtimon:0.13-consensus-dust-out-minimal branch Feb 11, 2017

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2017

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.

@jtimon jtimon force-pushed the jtimon:0.13-consensus-dust-out-minimal branch Apr 4, 2017

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2017

Needed rebase

@@ -0,0 +1,54 @@
// Copyright (c) 2009-2010 Satoshi Nakamoto
// Copyright (c) 2009-2015 The Bitcoin Core developers

This comment has been minimized.

Copy link
@jtimon

jtimon Apr 4, 2017

Author Member

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

@jtimon jtimon force-pushed the jtimon:0.13-consensus-dust-out-minimal branch Apr 5, 2017

@jtimon

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2017

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]
...
@sipa

This comment has been minimized.

Copy link
Member

commented Apr 21, 2017

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

jtimon added some commits Oct 14, 2016

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)

@jtimon jtimon force-pushed the jtimon:0.13-consensus-dust-out-minimal branch to 381a46e May 3, 2017

@jtimon

This comment has been minimized.

Copy link
Member Author

commented May 3, 2017

needed trivial rebase (7)

@ryanofsky
Copy link
Contributor

left a comment

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

@@ -15,6 +15,43 @@

#include <boost/foreach.hpp>

CAmount GetDustThreshold(const CTxOut& txout, const CFeeRate& dustRelayFee)

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky May 8, 2017

Contributor

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.

This comment has been minimized.

Copy link
@paveljanik

paveljanik May 9, 2017

Contributor

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.

@laanwj laanwj merged commit 381a46e into bitcoin:master May 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request May 9, 2017

Merge #9279: Consensus: Move CFeeRate out of libconsensus
381a46e Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module (Jorge Timón)
330bb5a Consensus: Minimal way to move dust out of consensus (Jorge Timón)

Tree-SHA512: 19a2ea8169afd5a9d3f940d8974e34cfaead153e3ff3068ac82fccdb8694d19d9b45938904ec9e8cd095bd5ca3a0080364da29372f6aaf56b11a6c2ccd6c7a4d

@jtimon jtimon deleted the jtimon:0.13-consensus-dust-out-minimal branch May 30, 2017

markblundeberg pushed a commit to markblundeberg/bitcoin-abc that referenced this pull request May 23, 2019

Consensus: Minimal way to move dust out of consensus
Summary:
Backport of Core PR9278
bitcoin/bitcoin#9279

Test Plan:
  make check
  test_runner.py

Reviewers: deadalnix, jasonbcox, Fabien, markblundeberg, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3039

markblundeberg pushed a commit to markblundeberg/bitcoin-abc that referenced this pull request May 23, 2019

Consensus: Minimal way to move dust out of consensus
Summary:
Backport of Core PR9279
bitcoin/bitcoin#9279

Test Plan:
  make check
  test_runner.py

Reviewers: deadalnix, jasonbcox, Fabien, markblundeberg, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3039

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 10, 2019

Merge bitcoin#9279: Consensus: Move CFeeRate out of libconsensus
381a46e Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module (Jorge Timón)
330bb5a Consensus: Minimal way to move dust out of consensus (Jorge Timón)

Tree-SHA512: 19a2ea8169afd5a9d3f940d8974e34cfaead153e3ff3068ac82fccdb8694d19d9b45938904ec9e8cd095bd5ca3a0080364da29372f6aaf56b11a6c2ccd6c7a4d

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 10, 2019

Merge bitcoin#9279: Consensus: Move CFeeRate out of libconsensus
381a46e Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module (Jorge Timón)
330bb5a Consensus: Minimal way to move dust out of consensus (Jorge Timón)

Tree-SHA512: 19a2ea8169afd5a9d3f940d8974e34cfaead153e3ff3068ac82fccdb8694d19d9b45938904ec9e8cd095bd5ca3a0080364da29372f6aaf56b11a6c2ccd6c7a4d

update dust back to pre segwit

Signed-off-by: Pasta <pasta@dashboost.org>

adjust

Signed-off-by: Pasta <pasta@dashboost.org>

remove const

Signed-off-by: Pasta <pasta@dashboost.org>

add param

Signed-off-by: Pasta <pasta@dashboost.org>

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 10, 2019

Merge bitcoin#9279: Consensus: Move CFeeRate out of libconsensus
381a46e Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module (Jorge Timón)
330bb5a Consensus: Minimal way to move dust out of consensus (Jorge Timón)

Tree-SHA512: 19a2ea8169afd5a9d3f940d8974e34cfaead153e3ff3068ac82fccdb8694d19d9b45938904ec9e8cd095bd5ca3a0080364da29372f6aaf56b11a6c2ccd6c7a4d

update dust back to pre segwit

Signed-off-by: Pasta <pasta@dashboost.org>

adjust

Signed-off-by: Pasta <pasta@dashboost.org>

remove const

Signed-off-by: Pasta <pasta@dashboost.org>

add param

Signed-off-by: Pasta <pasta@dashboost.org>

jonspock added a commit to jonspock/devault that referenced this pull request Jun 13, 2019

Consensus: Minimal way to move dust out of consensus
Summary:
Backport of Core PR9278
bitcoin/bitcoin#9279

Test Plan:
  make check
  test_runner.py

Reviewers: deadalnix, jasonbcox, Fabien, markblundeberg, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3039

jonspock added a commit to devaultcrypto/devault that referenced this pull request Jun 13, 2019

Consensus: Minimal way to move dust out of consensus
Summary:
Backport of Core PR9278
bitcoin/bitcoin#9279

Test Plan:
  make check
  test_runner.py

Reviewers: deadalnix, jasonbcox, Fabien, markblundeberg, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3039

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 19, 2019

Merge bitcoin#9279: Consensus: Move CFeeRate out of libconsensus
381a46e Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module (Jorge Timón)
330bb5a Consensus: Minimal way to move dust out of consensus (Jorge Timón)

Tree-SHA512: 19a2ea8169afd5a9d3f940d8974e34cfaead153e3ff3068ac82fccdb8694d19d9b45938904ec9e8cd095bd5ca3a0080364da29372f6aaf56b11a6c2ccd6c7a4d

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 19, 2019

Merge bitcoin#9279: Consensus: Move CFeeRate out of libconsensus
381a46e Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module (Jorge Timón)
330bb5a Consensus: Minimal way to move dust out of consensus (Jorge Timón)

Tree-SHA512: 19a2ea8169afd5a9d3f940d8974e34cfaead153e3ff3068ac82fccdb8694d19d9b45938904ec9e8cd095bd5ca3a0080364da29372f6aaf56b11a6c2ccd6c7a4d

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 19, 2019

Merge bitcoin#9279: Consensus: Move CFeeRate out of libconsensus
381a46e Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module (Jorge Timón)
330bb5a Consensus: Minimal way to move dust out of consensus (Jorge Timón)

Tree-SHA512: 19a2ea8169afd5a9d3f940d8974e34cfaead153e3ff3068ac82fccdb8694d19d9b45938904ec9e8cd095bd5ca3a0080364da29372f6aaf56b11a6c2ccd6c7a4d

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 19, 2019

Merge bitcoin#9279: Consensus: Move CFeeRate out of libconsensus
381a46e Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module (Jorge Timón)
330bb5a Consensus: Minimal way to move dust out of consensus (Jorge Timón)

Tree-SHA512: 19a2ea8169afd5a9d3f940d8974e34cfaead153e3ff3068ac82fccdb8694d19d9b45938904ec9e8cd095bd5ca3a0080364da29372f6aaf56b11a6c2ccd6c7a4d

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 19, 2019

Merge bitcoin#9279: Consensus: Move CFeeRate out of libconsensus
381a46e Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module (Jorge Timón)
330bb5a Consensus: Minimal way to move dust out of consensus (Jorge Timón)

Tree-SHA512: 19a2ea8169afd5a9d3f940d8974e34cfaead153e3ff3068ac82fccdb8694d19d9b45938904ec9e8cd095bd5ca3a0080364da29372f6aaf56b11a6c2ccd6c7a4d

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 19, 2019

Merge bitcoin#9279: Consensus: Move CFeeRate out of libconsensus
381a46e Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module (Jorge Timón)
330bb5a Consensus: Minimal way to move dust out of consensus (Jorge Timón)

Tree-SHA512: 19a2ea8169afd5a9d3f940d8974e34cfaead153e3ff3068ac82fccdb8694d19d9b45938904ec9e8cd095bd5ca3a0080364da29372f6aaf56b11a6c2ccd6c7a4d

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 19, 2019

Merge bitcoin#9279: Consensus: Move CFeeRate out of libconsensus
381a46e Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module (Jorge Timón)
330bb5a Consensus: Minimal way to move dust out of consensus (Jorge Timón)

Tree-SHA512: 19a2ea8169afd5a9d3f940d8974e34cfaead153e3ff3068ac82fccdb8694d19d9b45938904ec9e8cd095bd5ca3a0080364da29372f6aaf56b11a6c2ccd6c7a4d

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 20, 2019

Merge bitcoin#9279: Consensus: Move CFeeRate out of libconsensus
381a46e Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module (Jorge Timón)
330bb5a Consensus: Minimal way to move dust out of consensus (Jorge Timón)

Tree-SHA512: 19a2ea8169afd5a9d3f940d8974e34cfaead153e3ff3068ac82fccdb8694d19d9b45938904ec9e8cd095bd5ca3a0080364da29372f6aaf56b11a6c2ccd6c7a4d

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 22, 2019

Merge bitcoin#9279: Consensus: Move CFeeRate out of libconsensus
381a46e Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module (Jorge Timón)
330bb5a Consensus: Minimal way to move dust out of consensus (Jorge Timón)

Tree-SHA512: 19a2ea8169afd5a9d3f940d8974e34cfaead153e3ff3068ac82fccdb8694d19d9b45938904ec9e8cd095bd5ca3a0080364da29372f6aaf56b11a6c2ccd6c7a4d

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 22, 2019

Merge bitcoin#9279: Consensus: Move CFeeRate out of libconsensus
381a46e Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module (Jorge Timón)
330bb5a Consensus: Minimal way to move dust out of consensus (Jorge Timón)

Tree-SHA512: 19a2ea8169afd5a9d3f940d8974e34cfaead153e3ff3068ac82fccdb8694d19d9b45938904ec9e8cd095bd5ca3a0080364da29372f6aaf56b11a6c2ccd6c7a4d

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 22, 2019

Merge bitcoin#9279: Consensus: Move CFeeRate out of libconsensus
381a46e Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module (Jorge Timón)
330bb5a Consensus: Minimal way to move dust out of consensus (Jorge Timón)

Tree-SHA512: 19a2ea8169afd5a9d3f940d8974e34cfaead153e3ff3068ac82fccdb8694d19d9b45938904ec9e8cd095bd5ca3a0080364da29372f6aaf56b11a6c2ccd6c7a4d

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 22, 2019

Merge bitcoin#9279: Consensus: Move CFeeRate out of libconsensus
381a46e Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module (Jorge Timón)
330bb5a Consensus: Minimal way to move dust out of consensus (Jorge Timón)

Tree-SHA512: 19a2ea8169afd5a9d3f940d8974e34cfaead153e3ff3068ac82fccdb8694d19d9b45938904ec9e8cd095bd5ca3a0080364da29372f6aaf56b11a6c2ccd6c7a4d

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jun 24, 2019

Merge bitcoin#9279: Consensus: Move CFeeRate out of libconsensus
381a46e Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module (Jorge Timón)
330bb5a Consensus: Minimal way to move dust out of consensus (Jorge Timón)

Tree-SHA512: 19a2ea8169afd5a9d3f940d8974e34cfaead153e3ff3068ac82fccdb8694d19d9b45938904ec9e8cd095bd5ca3a0080364da29372f6aaf56b11a6c2ccd6c7a4d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.