Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: Don't ignore `-maxtxfee` when wallet is disabled #15357

Merged
merged 1 commit into from Feb 8, 2019

Conversation

Projects
None yet
7 participants
@JBaczuk
Copy link
Contributor

commented Feb 6, 2019

Resolves #15355

Show resolved Hide resolved src/init.cpp Outdated
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15340 (gui: Introduce bilingual GUI error messages by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@fanquake fanquake added the Refactoring label Feb 6, 2019

@JBaczuk

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

Ok I've tested and it does abort if invalid, and issues a warning if fee is too high.

@Empact

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

Maybe move it up to around line 1099, to sit alongside the other fee-related options handling?

@MarcoFalke MarcoFalke changed the title Move maxTxFee initialization to init.cpp rpc: Don't ignore `-maxtxfee` when wallet is disabled Feb 7, 2019

@MarcoFalke MarcoFalke added RPC/REST/ZMQ and removed Refactoring labels Feb 7, 2019

@JBaczuk

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2019

Maybe move it up to around line 1099, to sit alongside the other fee-related options handling?

done

@bitcoin bitcoin deleted a comment from weerayut3 Feb 8, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits. There is no need to keep the git history around in this pull request.

@JBaczuk JBaczuk force-pushed the JBaczuk:fix_maxtxfee branch from 5dc36d7 to dfbf117 Feb 8, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

utACK dfbf117 because it at least makes behaviour consistent. I still don't think that -maxtxfee should be shared between the node and wallet, so this doesn't fully address my concerns in #15355. Reasoning here: #15355 (comment).

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

ACK dfbf117.

Checked that it is no longer ignored when the wallet is not compiled or not enabled:

$ ./configure --disable-wallet && make
$ ./src/qt/bitcoin-qt -maxtxfee=foobar
Error: Invalid amount for -maxtxfee=<amount>: 'foobar'

$ ./configure && make
$ ./src/qt/bitcoin-qt -maxtxfee=foobar -disablewallet
Error: Invalid amount for -maxtxfee=<amount>: 'foobar'

screenshot from 2019-02-08 11-23-11

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Feb 8, 2019

@ryanofsky
Copy link
Contributor

left a comment

utACK dfbf117, confirmed move only.

@MarcoFalke MarcoFalke merged commit dfbf117 into bitcoin:master Feb 8, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Feb 8, 2019

Merge #15357: rpc: Don't ignore `-maxtxfee` when wallet is disabled
dfbf117 Move maxTxFee initialization to init.cpp (Jordan Baczuk)

Pull request description:

  Resolves #15355

Tree-SHA512: 6eafacc6a3b0589fb645b0080fd3c01598566df1bd7ee7929284853866a23493960fbd4d6f9c3417e192f8a21706d9f593197734f6189e046e4747991305a0b8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.