Consensus: Move CFeeRate out of libconsensus #9279

jtimon · 2016-12-05T05:45:37Z

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

jtimon · 2016-12-09T18:39:58Z

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 · 2016-12-09T23:49:28Z

concept ACK

sipa · 2016-12-09T23:51:09Z

Concept ACK

jtimon · 2016-12-13T20:31:45Z

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

jtimon · 2017-01-31T19:51:08Z

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 · 2017-02-02T18:37:10Z

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

dcousens

utACK

NicolasDorier · 2017-02-03T04:47:17Z

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 · 2017-02-03T05:17:44Z

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

NicolasDorier · 2017-02-03T05:33:53Z

src/qt/coincontroldialog.cpp

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

jtimon · 2017-02-09T21:31:53Z

Fixed @NicolasDorier 's great catch.

theuni · 2017-02-09T23:51:22Z

src/primitives/transaction.h

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

morcos · 2017-02-10T16:52:17Z

@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 · 2017-02-11T02:37:14Z

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 · 2017-02-11T02:53:52Z

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 · 2017-04-04T16:09:46Z

Needed rebase

jtimon · 2017-04-04T16:18:46Z

src/policy/feerate.h

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

jtimon · 2017-04-05T23:08:44Z

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

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

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

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

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

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

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

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

874096e [MOVEONLY] Move CFeeRate out of the consensus module (random-zebra) Pull request description: After #1950, this PR completes the backport of bitcoin#9279 to move CFeeRate out of libconsensus. > CTxOut and CTransaction shouldn't depend on CFeeRate, which is something for policy checks and not consensus checks. ACKs for top commit: furszy: all good now, utACK 874096e Fuzzbawls: utACK 874096e Tree-SHA512: e5c33d7e26003a1c4acb5159c24be5ac0209fe9875b7b7d430cd8927c2174ea4727a8728a065dcf0bf8be4389b8f90fdfab227face458c014cf409385a106baa

jtimon mentioned this pull request Dec 5, 2016

Consensus: Policy: Move CFeeRate out of consensus module and create CPolicy interface #7820

Closed

fanquake added the Consensus label Dec 5, 2016

jtimon force-pushed the 0.13-consensus-dust-out-minimal branch 2 times, most recently from 53955e9 to b5ba7f4 Compare December 13, 2016 20:27

morcos mentioned this pull request Jan 20, 2017

Fixes for bumpfee and minimum fees (use incrementalRelayFee for BIP 125 (RBF) replacement) #9589

Merged

jtimon force-pushed the 0.13-consensus-dust-out-minimal branch 2 times, most recently from 54981f2 to 976261f Compare January 31, 2017 18:55

dcousens approved these changes Feb 3, 2017

View reviewed changes

jtimon force-pushed the 0.13-consensus-dust-out-minimal branch from 976261f to 9e7f477 Compare February 3, 2017 05:17

NicolasDorier reviewed Feb 3, 2017

View reviewed changes

jtimon force-pushed the 0.13-consensus-dust-out-minimal branch from 9e7f477 to 4e124f8 Compare February 9, 2017 21:30

theuni reviewed Feb 9, 2017

View reviewed changes

src/primitives/transaction.h Outdated

Copy link

Member

theuni Feb 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jtimon reacted with thumbs up emoji

jtimon force-pushed the 0.13-consensus-dust-out-minimal branch from 4e124f8 to 45ba355 Compare February 11, 2017 02:53

jtimon force-pushed the 0.13-consensus-dust-out-minimal branch from 45ba355 to c32f28b Compare April 4, 2017 16:09

jtimon commented Apr 4, 2017

View reviewed changes

src/policy/feerate.h Outdated

Copy link

Contributor Author

jtimon Apr 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jtimon force-pushed the 0.13-consensus-dust-out-minimal branch from c32f28b to cb9d875 Compare April 5, 2017 23:08

paveljanik mentioned this pull request May 19, 2017

Prevent shadowing the global dustRelayFee #10431

Merged

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

random-zebra mentioned this pull request Nov 3, 2020

[Refactor] Move CFeeRate out of the consensus module PIVX-Project/PIVX#1954

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Consensus: Move CFeeRate out of libconsensus #9279

Consensus: Move CFeeRate out of libconsensus #9279

Uh oh!

Conversation

jtimon commented Dec 5, 2016

Uh oh!

jtimon commented Dec 9, 2016

Uh oh!

dcousens commented Dec 9, 2016

Uh oh!

sipa commented Dec 9, 2016

Uh oh!

jtimon commented Dec 13, 2016

Uh oh!

jtimon commented Jan 31, 2017

Uh oh!

laanwj commented Feb 2, 2017

Uh oh!

dcousens left a comment

Choose a reason for hiding this comment

Uh oh!

NicolasDorier commented Feb 3, 2017

Uh oh!

jtimon commented Feb 3, 2017

Uh oh!

NicolasDorier Feb 3, 2017

Choose a reason for hiding this comment

Uh oh!

jtimon commented Feb 9, 2017

Uh oh!

theuni Feb 9, 2017

Choose a reason for hiding this comment

Uh oh!

morcos commented Feb 10, 2017

Uh oh!

jtimon commented Feb 11, 2017

Uh oh!

jtimon commented Feb 11, 2017

Uh oh!

jtimon commented Apr 4, 2017

Uh oh!

jtimon Apr 4, 2017

Choose a reason for hiding this comment

Uh oh!

jtimon commented Apr 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants