Page MenuHomePhabricator

Move handling for `-maxtxfee` to the config object
Needs ReviewPublic

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

Details

Reviewers
deadalnix
jasonbcox
Group Reviewers
Restricted Project
Restricted Owners Package(Owns No Changed Paths)
Summary

As per title. Depends on D1935 and D2014

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

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D1936 (branched from master)
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 5376
Build 8814: Bitcoin ABC Teamcity Staging
Build 8813: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
schancel updated this revision to Diff 5567.Oct 29 2018, 18:12

Fix greator or equal

deadalnix requested changes to this revision.Nov 5 2018, 23:09
deadalnix added inline comments.
src/config.cpp
87 ↗(On Diff #5567)

This is left uninitialized when the config is created.

src/config.h
109 ↗(On Diff #5567)

DEFAULT_TRANSACTION_MAXFEE

src/test/txvalidationcache_tests.cpp
32 ↗(On Diff #5567)

This doesn't seems to be related with the diff.

src/wallet/init.cpp
222 ↗(On Diff #5567)

Remove and initialize the config as expected.

This revision now requires changes to proceed.Nov 5 2018, 23:09
schancel updated this revision to Diff 5657.Nov 6 2018, 00:08
schancel marked an inline comment as done.

Update per feedback from @deadalnix

schancel updated this revision to Diff 5658.Nov 6 2018, 00:18

Address more feedback

deadalnix accepted this revision.Nov 6 2018, 00:54
deadalnix added inline comments.
src/config.cpp
9 ↗(On Diff #5658)

This most likely indicate that the DEFAULT_TRANSACTION_MAXFEE should be defined elsewhere :)

This revision is now accepted and ready to land.Nov 6 2018, 00:54
schancel marked an inline comment as done.Nov 6 2018, 20:32
schancel added inline comments.
src/config.cpp
9 ↗(On Diff #5658)

Agreed, I was thinking the same thing. Let me figure out where to put these guys

src/config.h
109 ↗(On Diff #5567)

This will need to be moved out of validation.h if we want to use this value here.

schancel updated this revision to Diff 5680.Nov 6 2018, 23:08

Rebase on proper defaults branch

schancel edited the summary of this revision. (Show Details)Nov 7 2018, 02:36
schancel updated this revision to Diff 5988.Nov 21 2018, 18:00
schancel edited the summary of this revision. (Show Details)

Rebase and remove dependency on D2014

schancel requested review of this revision.Nov 25 2018, 09:44
deadalnix requested changes to this revision.Nov 25 2018, 17:05
deadalnix added inline comments.
src/wallet/init.cpp
235 ↗(On Diff #5988)

This sanity check wasn't ported over.

This revision now requires changes to proceed.Nov 25 2018, 17:05
schancel marked 2 inline comments as done.Nov 25 2018, 17:29
schancel added inline comments.
src/config.cpp
89 ↗(On Diff #5988)

@deadalnix You asked me to move the sanity check here. Am I missing something?

src/wallet/init.cpp
235 ↗(On Diff #5988)

You asked previously to move it to bool GlobalConfig::SetMaxFee(Amount fee)? Is there something wrong with what I put there?

schancel requested review of this revision.Nov 25 2018, 17:29
jasonbcox requested changes to this revision.Mar 11 2019, 22:44

Needs rebase.

This revision now requires changes to proceed.Mar 11 2019, 22:44
Owners added a reviewer: Restricted Owners Package.Mar 17 2019, 06:29
jasonbcox requested changes to this revision.Mar 20 2019, 01:43
jasonbcox added inline comments.
src/config.h
78 ↗(On Diff #7722)

Is this still being used?

src/wallet/wallet.cpp
2781 ↗(On Diff #7722)

Undo. This change doesn't appear related to this diff.

This revision now requires changes to proceed.Mar 20 2019, 01:43
schancel updated this revision to Diff 7768.Mar 21 2019, 03:05

Remove clang-format issue

This looks good on the surface, but I need to do another pass of review to make sure.

jasonbcox requested changes to this revision.Mar 25 2019, 17:25
jasonbcox added inline comments.
src/config.cpp
81 ↗(On Diff #7768)

This looks like a simple rename from feePerKB -> minFeePerKB. If that's the case, please remove feePerKB from the config. Otherwise, explain why both are needed and why they're separate.

This revision now requires changes to proceed.Mar 25 2019, 17:25
schancel requested review of this revision.Sat, Mar 30, 17:04
schancel added inline comments.
src/config.cpp
81 ↗(On Diff #7768)

Please elaborate? This is the minimum fee that will be relayed per KB. The MaxFee is not currently enforced Per KB -- the next commit on the stack fixes that.

schancel updated this revision to Diff 7937.Thu, Apr 4, 03:30

Rebase and address feedback

deadalnix added inline comments.Sun, Apr 14, 19:56
src/config.cpp
10

That looks like DEFAULT_TRANSACTION_MAXFEE is not defined at the right place.

src/qt/walletmodel.cpp
289

Why is this check removed?

schancel added inline comments.Sun, Apr 14, 20:09
src/config.cpp
10

Probably, but this should likely be moved in a different diff.

src/qt/walletmodel.cpp
289

It's the wallet's responsibility not to be creating transactions with absurdly high fees.