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: Uncouple non-wallet rpcs from maxTxFee global #15620

Merged
merged 3 commits into from Mar 27, 2019

Conversation

Projects
None yet
9 participants
@MarcoFalke
Copy link
Member

MarcoFalke commented Mar 18, 2019

This makes the rpcs a bit more stateless by falling back to their own default max fee instead of the global maxTxFee.

A follow up pull request will move -maxtxfee to the wallet.

See also related discussions:

  • -maxtxfee should not be used by both node and wallet #15355
  • [RFC] Long term plan for wallet command-line args #13044

MarcoFalke added some commits Mar 18, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Mar 18, 2019

Concept ACK!

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

utACK fa96d76. This looks great and is nicely done. But it definitely should have release notes noting change in behavior for sendrawtransaction and testmempoolaccept RPCs, which now ignore the -maxtxfee setting.

@jamesob

This comment has been minimized.

Copy link
Member

jamesob commented Mar 20, 2019

utACK fa96d76. Verified this replaces the only usages of maxTxFee global in rpc code.

$ git grep maxTxFee | grep rpc

src/rpc/rawtransaction.cpp:    const CAmount highfee{allowhighfees ? 0 : ::maxTxFee};
src/rpc/rawtransaction.cpp:    CAmount max_raw_tx_fee = ::maxTxFee;
@promag

This comment has been minimized.

Copy link
Member

promag commented Mar 20, 2019

How about bumpfee?

// Check that in all cases the new fee doesn't violate maxTxFee
if (new_fee > maxTxFee) {
errors.push_back(strprintf("Specified or calculated fee %s is too high (cannot be higher than maxTxFee %s)",
FormatMoney(new_fee), FormatMoney(maxTxFee)));
return Result::WALLET_ERROR;
}

😮 space in L162

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Mar 20, 2019

Bumpfee is a wallet function, so it's reasonable if it continues to use maxtxfee if maxtxfee becomes a wallet option.

@promag
Copy link
Member

promag left a comment

Misinterpreted the goal sorry, IMO could have the title Uncouple non-wallet rpcs from maxTxFee global.

Agree with adding release notes.

Show resolved Hide resolved src/rpc/rawtransaction.cpp

@MarcoFalke MarcoFalke changed the title rpc: Uncouple rpcs from maxTxFee global rpc: Uncouple non-wallt rpcs from maxTxFee global Mar 20, 2019

@MarcoFalke MarcoFalke changed the title rpc: Uncouple non-wallt rpcs from maxTxFee global rpc: Uncouple non-wallet rpcs from maxTxFee global Mar 20, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Mar 20, 2019

Renamed title and added release notes as requested by @promag

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

utACK faadaae, only change is release notes

Show resolved Hide resolved doc/release-notes-15620.md Outdated
Show resolved Hide resolved doc/release-notes-15620.md Outdated

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1903-rpcNoMaxTxFee branch 2 times, most recently from fa55499 to fa4230b Mar 20, 2019

@jnewbery
Copy link
Member

jnewbery left a comment

Oops. I'd already written a branch for this too.

One comment inline, otherwise utACK fa4230b.

Show resolved Hide resolved src/rpc/rawtransaction.cpp

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1903-rpcNoMaxTxFee branch from fa4230b to fa1ad20 Mar 20, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Mar 20, 2019

Changed my mind as requested by @jnewbery

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Mar 20, 2019

utACK fa1ad20

@Empact

This comment has been minimized.

Copy link
Member

Empact commented Mar 21, 2019

utACK fa1ad20

@promag
Copy link
Member

promag left a comment

The above reference was a mistake.

utACK fa1ad20.

Show resolved Hide resolved src/rpc/rawtransaction.cpp
@sipa

This comment has been minimized.

Copy link
Member

sipa commented Mar 25, 2019

Concept ACK

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Mar 26, 2019

utACK fa1ad20

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Mar 27, 2019

Merge bitcoin#15620: rpc: Uncouple non-wallet rpcs from maxTxFee global
fa1ad20 doc: Add release notes for 15620 (MarcoFalke)
fa96d76 rpc: Uncouple rpcs from maxTxFee global (MarcoFalke)
fa965e0 rpc: Use IsValidNumArgs over hardcoded size checks (MarcoFalke)

Pull request description:

  This makes the rpcs a bit more stateless by falling back to their own default max fee instead of the global maxTxFee.

  A follow up pull request will move `-maxtxfee` to the wallet.

  See also related discussions:

  * `-maxtxfee` should not be used by both node and wallet bitcoin#15355
  *  [RFC] Long term plan for wallet command-line args bitcoin#13044

ACKs for commit fa1ad2:
  jnewbery:
    utACK fa1ad20
  Empact:
    utACK bitcoin@fa1ad20
  jnewbery:
    utACK fa1ad20
  promag:
    utACK fa1ad20.

Tree-SHA512: c9cf0b54cd30ff3ab0d090b072cc38fcbb2840bc6ad9a9711995333bc927d2500aece6b5a60e061666eca5ed72b70aa318d21e51eb15ee0106b41f5b6e4e1adf

@MarcoFalke MarcoFalke merged commit fa1ad20 into bitcoin:master Mar 27, 2019

2 checks passed

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

@MarcoFalke MarcoFalke deleted the MarcoFalke:1903-rpcNoMaxTxFee branch Mar 27, 2019

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.