Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
jtimon wants to merge 9 commits into from
Closed

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

jtimon wants to merge 9 commits into from

Conversation

@jtimon
Copy link
Contributor

@jtimon jtimon commented Apr 5, 2016

This moves the CFeeRate and dust code out of libconsensus and the consensus module.
Before that, it prepares an empty CPolicy interface and implementation where the dust methods are moved to. It also encapsulates the global minRelayTxFee behind the CPolicy interface.
Since this may be too disruptive for one PR, I was very careful to separate things in smaller commits.
The first commit, for example, may be useful for @jonasschnelli 's work encapsulating the wallet.

This is also intended to replace @morcos' #7557, #6068 and #5114.

@dcousens
Copy link
Contributor

dcousens commented Apr 6, 2016

concept ACK

@jtimon jtimon force-pushed the 0.12.99-consensus-dust-out branch from 71ffeaa to 1e614c8 Compare April 14, 2016 15:57
@jtimon
Copy link
Contributor Author

jtimon commented Apr 14, 2016

Rebased(1).
Also, fixed the error that was making python ./qa/pull-tester/rpc-tests.py mempool_limit fail.

@jtimon jtimon mentioned this pull request Jul 11, 2016
Closed
@jtimon jtimon force-pushed the 0.12.99-consensus-dust-out branch from 1e614c8 to eac77cd Compare August 30, 2016 21:59
@jtimon jtimon force-pushed the 0.12.99-consensus-dust-out branch 3 times, most recently from e167f6f to 9dd9bc0 Compare October 14, 2016 16:50
@jtimon
Copy link
Contributor Author

jtimon commented Oct 14, 2016

Rebased and fixed one error, but it is still failing.
Still not sure what to do with AmountErrMsg and ParseAmountFromArgs. For the latter to use the former, both need to be put together in either util.o or ui_interface.o.
I pushed an alternative simpler option without creating the policy interface nor removing the global minRelayTxFee in master...jtimon:0.13-consensus-dust-out-minimal.

@NicolasDorier
Copy link
Contributor

NicolasDorier commented Oct 15, 2016

Concept ACK will review, can you check the failing travis ?

@jtimon
Copy link
Contributor Author

jtimon commented Oct 15, 2016

I'm sorry, after sewgit's rebase this did never passed travis (but this time I removed the known past mistakes, so it's weird). That;s why I suggested master...jtimon:0.13-consensus-dust-out-minimal which seems to work (locally).
At the same I promised @btcdrak I wouldn't open parallel mutually exclusive PRs to see which one fails on travis or for people to compare and choose...

The travis error seemed related to CAmount becoming undefined for windows, but on the last push it also failed in another build.
I will keep investigating, but travis is what I meant by "it is still failing".

@jtimon
Copy link
Contributor Author

jtimon commented Nov 4, 2016 •
edited
Loading

EDIT: needed rebased and rebased master...jtimon:0.13-consensus-dust-out-minimal
ping @btcdrak since the two branches are kind of competing and he advised me against doing that with 2 open PRs because people tend to get confused.

@jtimon jtimon force-pushed the 0.12.99-consensus-dust-out branch from 9dd9bc0 to e3acf59 Compare November 4, 2016 00:58
@jtimon jtimon force-pushed the 0.12.99-consensus-dust-out branch 2 times, most recently from d796cab to c65eff1 Compare November 23, 2016 04:32
@jtimon jtimon force-pushed the 0.12.99-consensus-dust-out branch from c65eff1 to a543bce Compare December 2, 2016 10:46
jtimon added 8 commits December 2, 2016 11:47
@jtimon
Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module
25c6b82 
...from amount.o to policy/feerate.o

Policy, because it moves policy code to the policy directory (common module)
@jtimon
Globals: Allow to parse arguments without implicitly using the argMap…
7e2e6fe 
… global
@jtimon
Policy: Introduce empty CPolicy interface and CDefaultPolicy implemen…
04f546d 
…tation
@jtimon
Globals: Policy: mempool and filterRounder are initialized before min…
7cf8970 
…RelayTxFee

Therefore the default value before init::AppInit2() is being used always.
@jtimon
Policy: Encapsulate ::minRelayTxFee initialization behind CDefaultPol…
9185a86 
…icy::InitFromArgs()
@jtimon
ERROR-unittest: Consensus: Policy: MOVEONLY-ish: Rename dust methods …
93f8f01 
…from CTxOut to CPolicy

Consensus, because it moves non-consensus code out of the consensus module
Policy, because it moves policy code to the policy directory (server module)

Method Renames:
- s/CTxOut::GetDustThreshold(const CFeeRate&)/CDefaultPolicy::GetDustThreshold(const CTxOut&)/
- s/CTxOut::IsDust(const CFeeRate&)/CDefaultPolicy::AcceptDust(const CTxOut&)/
@jtimon
ERROR-unit-test_IsStandard: Policy: Introduce CPolicy::GetMinRelayFee…
a56cbad 
…() and use it everywhere except for dust
@jtimon
Globals: Cleanly remove ::minRelayTxFee
daabe73
@jtimon jtimon force-pushed the 0.12.99-consensus-dust-out branch from a543bce to daabe73 Compare December 2, 2016 10:48
This was referenced Dec 3, 2016
Merged
Closed
Merged
@jtimon
Copy link
Contributor Author

jtimon commented Dec 5, 2016

Apart from being failing tests and needing rebase, this is going to conflict with #9243. Replaced with the simpler branch I was suggesting that only moves CFeeRate without creating any policy class or interface: #9279

@jtimon jtimon closed this Dec 5, 2016
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jtimon @dcousens @NicolasDorier @laanwj