Page MenuHomePhabricator

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

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

Details

Reviewers
deadalnix
jasonbcox
markblundeberg
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
SeverityLocationCodeMessage
Auto-Fixsrc/wallet/wallet.cpp:1CFMTCode style violation
Unit
No Unit Test Coverage
Build Status
Buildable 5263
Build 8589: Bitcoin ABC Buildbot (legacy)
Build 8588: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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 marked an inline comment as done.

Update per feedback from @deadalnix

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 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.

Rebase on proper defaults branch

schancel edited the summary of this revision. (Show Details)

Rebase and remove dependency on D2014

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 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?

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

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

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 added inline comments.
src/config.cpp
81

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.

Rebase and address feedback

src/config.cpp
10 ↗(On Diff #7937)

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

src/qt/walletmodel.cpp
289 ↗(On Diff #7937)

Why is this check removed?

src/config.cpp
10 ↗(On Diff #7937)

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

src/qt/walletmodel.cpp
289 ↗(On Diff #7937)

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

markblundeberg added a subscriber: markblundeberg.

I think a variety of things have touched this code lately -- can this be rebased?

This revision now requires changes to proceed.Jul 1 2019, 17:22