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

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 3707
Build 5489: Bitcoin ABC Buildbot (legacy)
Build 5488: arc lint + arc unit

Event Timeline

schancel created this revision.Oct 19 2018, 00:53

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 19 2018, 00:53

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B3618: Diff 5445!Oct 19 2018, 00:59

Rebase to address feedback on D1936

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B3658: Diff 5501!Oct 22 2018, 20:30

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.

Rebase and address feedback on release-notes

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B3682: Diff 5532!Oct 25 2018, 18:10

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.