rpc: Uncouple non-wallet rpcs from maxTxFee global #15620

maflcko · 2019-03-18T18:03:02Z

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 -maxtxfee should not be used by both node and wallet #15355
[RFC] Long term plan for wallet command-line args [RFC] Long term plan for wallet command-line args #13044

jnewbery · 2019-03-18T18:54:57Z

Concept ACK!

ryanofsky

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 · 2019-03-20T16:02:10Z

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 · 2019-03-20T16:38:38Z

How about bumpfee?

bitcoin/src/wallet/feebumper.cpp

Lines 161 to 166 in 93623ee

    
           // 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 · 2019-03-20T16:47:48Z

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

promag

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

Agree with adding release notes.

src/rpc/rawtransaction.cpp

maflcko · 2019-03-20T17:08:47Z

Renamed title and added release notes as requested by @promag

ryanofsky

utACK faadaae4795038d77936f36483f1410cd92b3238, only change is release notes

doc/release-notes-15620.md

jnewbery

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

One comment inline, otherwise utACK fa4230b8a708ad9b848c97a5142da804afc43a4b.

src/rpc/rawtransaction.cpp

maflcko · 2019-03-20T21:07:35Z

Changed my mind as requested by @jnewbery

jnewbery · 2019-03-20T22:15:13Z

utACK fa1ad20

Empact · 2019-03-21T08:24:39Z

utACK fa1ad20

promag

The above reference was a mistake.

utACK fa1ad20.

src/rpc/rawtransaction.cpp

sipa · 2019-03-25T23:40:45Z

Concept ACK

jnewbery · 2019-03-26T16:34:49Z

utACK fa1ad20

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

5c759c7 [wallet] Move maxTxFee to wallet (John Newbery) Pull request description: Closes bitcoin#15355 Moves the `-maxtxfee` from the node to the wallet. See discussion in issue for details. This is a cleanup. There is no change in behaviour. Completes bitcoin#15620 ACKs for commit 5c759c: MarcoFalke: utACK 5c759c7 ryanofsky: utACK 5c759c7. Changes since last review: updated commit message and an error message and method name. meshcollider: utACK bitcoin@5c759c7 Tree-SHA512: 2f9b2729da3940a5cda994d3f3bc11ee1a52fcc1c5e9842ea0ea63e4eb0300e8416853046776311298bc449ba07554aa46f0f245ce28598a5b0bd7347c12e752

Ref: bitcoin/bitcoin#15620

Summary: * rpc: Use IsValidNumArgs over hardcoded size checks * rpc: Uncouple rpcs from maxTxFee global * doc: Add release notes for 15620 This is a backport of Core [[bitcoin/bitcoin#15620 | PR15620]] Test Plan: ninja all check-all Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6266

MarcoFalke added 2 commits March 18, 2019 13:55

rpc: Use IsValidNumArgs over hardcoded size checks

fa965e0

rpc: Uncouple rpcs from maxTxFee global

fa96d76

maflcko added the RPC/REST/ZMQ label Mar 18, 2019

maflcko removed the RPC/REST/ZMQ label Mar 18, 2019

DrahtBot added the RPC/REST/ZMQ label Mar 19, 2019

doc: Add release notes for 15620

fa1ad20

ryanofsky approved these changes Mar 20, 2019

View reviewed changes

fanquake added the Needs release note label Mar 20, 2019

promag reviewed Mar 20, 2019

View reviewed changes

src/rpc/rawtransaction.cpp Show resolved Hide resolved

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

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

maflcko removed the Needs release note label Mar 20, 2019

ryanofsky approved these changes Mar 20, 2019

View reviewed changes

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

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

maflcko force-pushed the 1903-rpcNoMaxTxFee branch 2 times, most recently from fa55499 to fa4230b Compare March 20, 2019 20:00

jnewbery reviewed Mar 20, 2019

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 1903-rpcNoMaxTxFee branch from fa4230b to fa1ad20 Compare March 20, 2019 21:06

promag mentioned this pull request Mar 23, 2019

wallet: Keep all outputs in bumpfee #15656

Closed

promag reviewed Mar 24, 2019

View reviewed changes

src/rpc/rawtransaction.cpp Show resolved Hide resolved

maflcko merged commit fa1ad20 into bitcoin:master Mar 27, 2019

maflcko deleted the 1903-rpcNoMaxTxFee branch March 27, 2019 13:24

jnewbery mentioned this pull request Apr 9, 2019

[wallet] Move maxtxfee from node to wallet #15778

Merged

SomberNight mentioned this pull request Aug 2, 2019

wallet: lower -txmaxfee default from 0.1 to 0.01 BTC #16539

Closed

vansergen added a commit to vansergen/rpc-bitcoin that referenced this pull request Mar 26, 2020

fix: remove allowhighfees option

761f0d9

Ref: bitcoin/bitcoin#15620

ZmnSCPxj mentioned this pull request Jul 20, 2020

onchaind should scorch-earth our penatly txes ElementsProject/lightning#3832

Closed

vijaydasmp mentioned this pull request Oct 30, 2021

Merge 15919, 15897, 15763, 16912, 17001, 16656, 16804 dashpay/dash#4555

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021

merge bitcoin#15620: Uncouple non-wallet rpcs from maxTxFee global

3284d82

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

rpc: Uncouple non-wallet rpcs from maxTxFee global #15620

rpc: Uncouple non-wallet rpcs from maxTxFee global #15620

Uh oh!

Conversation

maflcko commented Mar 18, 2019

Uh oh!

jnewbery commented Mar 18, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jamesob commented Mar 20, 2019

Uh oh!

promag commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Mar 20, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Mar 20, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Mar 20, 2019

Uh oh!

jnewbery commented Mar 20, 2019

Uh oh!

Empact commented Mar 21, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sipa commented Mar 25, 2019

Uh oh!

jnewbery commented Mar 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

promag commented Mar 20, 2019 •

edited

Loading