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

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 3794
Build 5662: Bitcoin ABC Buildbot (legacy)
Build 5661: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

Harbormaster completed remote builds in B3706: Diff 5565.Oct 29 2018, 18:20

Harbormaster completed remote builds in B3708: Diff 5567.Oct 29 2018, 18:22

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

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

Update per feedback from @deadalnix

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

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

Harbormaster completed remote builds in B3774: Diff 5657.Nov 6 2018, 00:18

Address more feedback

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

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

Harbormaster completed remote builds in B3775: Diff 5658.Nov 6 2018, 00:27

schancel added a child revision: D2006: Ensure config variables are initalized.Nov 6 2018, 00:45

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.

Rebase on proper defaults branch

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

schancel edited the summary of this revision. (Show Details)Nov 6 2018, 23:13

schancel added a parent revision: D2014: [move only] move default global vars to defaults.h.

Harbormaster completed remote builds in B3794: Diff 5680.Nov 6 2018, 23:18

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

schancel edited the summary of this revision. (Show Details)Nov 7 2018, 02:36

schancel edited the summary of this revision. (Show Details)Nov 21 2018, 17:59

schancel removed a parent revision: D2014: [move only] move default global vars to defaults.h.

Rebase and remove dependency on D2014

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

Harbormaster completed remote builds in B4028: Diff 5988.Nov 21 2018, 18:12

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

schancel requested review of this revision.Nov 25 2018, 09:44

schancel removed a parent revision: D2014: [move only] move default global vars to defaults.h.

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

Needs rebase.

This revision now requires changes to proceed.Mar 11 2019, 22:44

Rebase on master

Owners added a reviewer: Restricted Owners Package.Mar 17 2019, 06:29

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

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

Harbormaster completed remote builds in B5230: Diff 7722.Mar 17 2019, 06:44

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

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

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

Harbormaster completed remote builds in B5263: Diff 7768.Mar 21 2019, 03:20

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.Mar 30 2019, 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.

Rebase and address feedback

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

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

Harbormaster completed remote builds in B5376: Diff 7937.Apr 4 2019, 03:45

deadalnix added inline comments.Apr 14 2019, 19:56

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?

schancel added inline comments.Apr 14 2019, 20:09

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.

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

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 5680

src/config.h

src/config.cpp

src/init.cpp

src/qt/sendcoinsdialog.cpp

src/qt/walletmodel.cpp

src/rpc/rawtransaction.cpp

src/validation.h

src/validation.cpp

src/wallet/fees.cpp

src/wallet/init.h

src/wallet/init.cpp

src/wallet/wallet.h

src/wallet/wallet.cpp

Move handling for `-maxtxfee` to the config objectNeeds RevisionPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 5680

src/config.h

src/config.cpp

src/init.cpp

src/qt/sendcoinsdialog.cpp

src/qt/walletmodel.cpp

src/rpc/rawtransaction.cpp

src/validation.h

src/validation.cpp

src/wallet/fees.cpp

src/wallet/init.h

src/wallet/init.cpp

src/wallet/wallet.h

src/wallet/wallet.cpp

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

Revision Contents
Changeset List