Page MenuHomePhabricator

Change the maxtxfee to be per kb.
Needs RevisionPublic

Authored by schancel on Oct 19 2018, 00:53.

Details

Reviewers
jasonbcox
deadalnix
Group Reviewers
Restricted Project
Summary

Update the codebase to use a feerate for asburdly large fees. In order to simplify wallet fee handling, we removed (in D1800) the wallet-specific mintxfee in favor of only using the minimum relay fee, as the codebase used the maximum of these two amounts. Now, the wallet still uses a maximum fee which is a flat amount, rather than a feerate. This means GetMinimumFee must return an Amount, rather than a FeeRate.

Requiring an Amount from GetRequiredFee inhibits further simplification of the wallet fee handling code.

Depends on D1936

Test Plan
make check && ./test/functional/test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
maxtxfee
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3694
Build 5463: Bitcoin ABC Buildbot (legacy)
Build 5462: arc lint + arc unit

Event Timeline

Rebase to address feedback on D1936

jasonbcox requested changes to this revision.Oct 23 2018, 19:04
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
doc/release-notes.md
11 ↗(On Diff #5501)

Instead of WARNING, maybe we should start using a format like:

Backwards-incompatible or otherwise breaking changes:
...

Other features and fixes:
...

Thoughts?

src/config.cpp
84 ↗(On Diff #5501)

Should this be >= to prevent weird edge cases? Maybe something to look at for a separate diff.

src/validation.h
71 ↗(On Diff #5501)

update this comment to mention sats/kB

448 ↗(On Diff #5501)

I think nAbsurdFee should be removed since there is access to config in this function. It also makes this code less error-prone since nAbsurdFee defaults to 0, which you probably don't want in most circumstances?

This revision now requires changes to proceed.Oct 23 2018, 19:04
schancel added inline comments.
src/config.cpp
84 ↗(On Diff #5501)

Why would we want the min and max to be the empty set?

src/validation.h
448 ↗(On Diff #5501)

nAbsurdFee here should probably just not even be checked, since if a transaction is being relayed to us we shouldn't care. That's up for the wallet to figure out. However, I don't think it's a good idea to change it in this diff.

Rebase and address feedback on release-notes

jasonbcox requested changes to this revision.Oct 25 2018, 21:24
jasonbcox added inline comments.
doc/release-notes.md
6 ↗(On Diff #5532)

This needs to be moved to the backwards-incompatible section.

src/config.cpp
84 ↗(On Diff #5501)

Exactly? With the code written here, fee can be equal to minFeePerKB and still return true.

This revision now requires changes to proceed.Oct 25 2018, 21:24
schancel marked an inline comment as done.

Rebase

deadalnix requested changes to this revision.Nov 4 2018, 19:01
deadalnix added a subscriber: deadalnix.

First, it's unclear to me why we want to change this. The diff description should mention what's the motivation. To me right now, it's unclear.

Second, there is a behavioral change here, but there is no test checking for the change of behavior.

This revision now requires changes to proceed.Nov 4 2018, 19:01

Fix Qt Compilation error

Rebas eon updated changes to maxtxfee

jasonbcox requested changes to this revision.Nov 12 2018, 22:13
This revision now requires changes to proceed.Nov 12 2018, 22:13
schancel added inline comments.
doc/release-notes.md
6 ↗(On Diff #5532)

I'm not doing that in this PR.

jasonbcox requested changes to this revision.Mar 7 2019, 19:00

Moving this off my queue until it's updated.

This revision now requires changes to proceed.Mar 7 2019, 19:00