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

Event Timeline

schancel created this revision.Oct 19 2018, 00:53
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 19 2018, 00:53
schancel updated this revision to Diff 5501.Oct 22 2018, 20:24

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 marked 2 inline comments as done.Oct 24 2018, 19:26
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.

schancel updated this revision to Diff 5532.Oct 25 2018, 18:03

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 updated this revision to Diff 5545.Oct 26 2018, 00:51
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
schancel edited the summary of this revision. (Show Details)Nov 5 2018, 19:31
schancel updated this revision to Diff 5646.Nov 5 2018, 19:49

Fix Qt Compilation error

schancel updated this revision to Diff 5661.Nov 6 2018, 02:39

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 requested review of this revision.Nov 25 2018, 17:29
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