wallet/rpc: sendrawtransaction maxfeerate by kallewoof · Pull Request #13541 · bitcoin/bitcoin

kallewoof · 2018-06-27T08:24:22Z

This adds a new maxfeerate parameter to sendrawtransaction which forces the node to reject a transaction whose feerate is above the given fee rate.

This is a safety harness from shooting yourself in the foot and accidentally overpaying fees.

See also #12911.

NicolasDorier · 2018-06-27T08:49:38Z

Your PR show me actually that maxtxfee exists, which is uncorrelated with absurd fee we were talking about.

One could say that changing maxtxfee or having maxtxfeerate at config level would solve the issue without touching RPC. But, when you do a transaction manually, you know the maxfeerate to expect when you call sendrawtransaction, not when you modify the config file of bitcoind.

ACK.

(That said, I think this would be better in signrawtransaction* than sendrawtransaction)

kallewoof · 2018-06-27T08:57:55Z

Your PR show me actually that maxtxfee exists, which is uncorrelated with absurd fee we were talking about.

Yes, absurdfee can actually be specified for each call to AcceptToMempool, so it can be used after all.

One could say that changing maxtxfee or having maxtxfeerate at config level would solve the issue without touching RPC. But, when you do a transaction manually, you know the maxfeerate to expect when you call sendrawtransaction, not when you modify the config file of bitcoind.

I think having a configurable default maxtxfee would be a good idea. Perhaps this function should then ignore the default if the user does allowhighfees=true.

(That said, I think this would be better in signrawtransaction* than sendrawtransaction)

It fits better here, because it already has the functionality needed in the AcceptToMemoryPool function via the absurd fee feature. In sign, you sometimes do not know the input amounts, which means the system sometimes can't check. Here, the amounts are always known because the mempool either knows or the tx is invalid.

promag · 2018-06-27T13:36:28Z

test/functional/rpc_rawtransaction.py

Alternative (not tested)

vout = next(o for o in rawTx['vout'] if o['value'] == Decimal('1.00000000'))

Several copies of this in rpc_rawtransaction, so I made a dedicated commit to address the others.

promag · 2018-06-27T13:41:15Z

src/rpc/rawtransaction.cpp

Missing parameter.

promag · 2018-06-27T13:41:37Z

src/rpc/rawtransaction.cpp

When specifying maxfeerate does it make sense to pass allowhighfees=true?

I am sort of overriding allowhighfees right now. I could disallow it completely, i.e. require that it is null or false, if using maxfeerate, but not sure it's worth it.

To more directly answer your question, though, allowhighfees=true will simply remove the existing absurd fee cap (which is 0.1 btc I believe), so if maxfeerate was really high, it actually could result in a slightly higher cap.

Just have a single parameter... if it's a boolean, it's the (deprecated?) allowhighfees; if a number, then a specific limit.

@luke-jr does that work with named parameters?

does that work with named parameters?

Yes, it is already used by getbalance:

bitcoin/src/wallet/rpcwallet.cpp

Line 4784 in 222e627

{ "wallet", "getbalance", &getbalance, {"account|dummy","minconf","include_watchonly"} },

DrahtBot · 2018-08-12T02:13:09Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15282 (test: Replace hard-coded hex tx with class in test framework by stevenroose)
#14691 (tests: Speedup feature_pruning test and refactor big transaction logic by conscott)
#14053 (Add address-based index (attempt 4?) by marcinja)

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.

meshcollider

utACK dbd466c

meshcollider · 2018-11-10T04:34:43Z

src/rpc/rawtransaction.cpp

Shouldn't this default be whatever maxTxFee is, not ∞?

Fixed - it now outputs

2. allowhighfees|maxfeerate (boolean/numeric, optional, default=false/0.10) Allow high fees (boolean) or reject transactions whose fee rate is higher than the specified value (numeric), expressed in BTC/kB

for the default settings.

meshcollider · 2018-11-10T04:42:14Z

src/rpc/rawtransaction.cpp

Might be good to add a quick comment here to clarify the +3/4 is for rounding

Added note.

kallewoof · 2019-03-05T01:13:47Z

@MarcoFalke Squashed.

test/functional/feature_cltv.py

maflcko · 2019-03-07T18:02:12Z

re-utACK 6c17985e8b

maflcko

Just some style nits (could be ignored)

src/rpc/rawtransaction.cpp

test/functional/rpc_rawtransaction.py

kallewoof · 2019-03-14T00:51:59Z

Addressed @MarcoFalke nits.

7abd2e6 wallet/rpc: add maxfeerate parameter to testmempoolaccept (Karl-Johan Alm) 6c0a6f7 wallet/rpc: add maxfeerate parameter to sendrawtransaction (Karl-Johan Alm) e5efacb test: Refactor vout fetches in rpc_rawtransaction (Karl-Johan Alm) Pull request description: This adds a new `maxfeerate` parameter to `sendrawtransaction` which forces the node to reject a transaction whose feerate is above the given fee rate. This is a safety harness from shooting yourself in the foot and accidentally overpaying fees. See also #12911. Tree-SHA512: efa50134a7c17c9330cfdfd48ba400e095c0a419cc45e630618d8b44929c25d780d1bb2710c1fbbb6e687eca373505b0338cdaa7f2ff4ca22636d84c31557a2e

maflcko · 2019-03-18T17:25:39Z

src/validation.cpp

+        }
+    }
+    return -1;
+}


re-utACK 7abd2e6

Sorry I only noticed this unused method in this hunk after merge 😿.

fa5c511 refactor: Remove unused function (MarcoFalke) Pull request description: Oversight of kallewoof and mine in #13541 (comment) Tree-SHA512: 2fd3c4ecde5d3c58b113aa58d606976ceb4998358bde0547ead8e83df210722fa9821d2c88b717bdd190ef71593cd9c0154c3a5d3f2ccc3af8cbf6c36aaa6d45

jnewbery · 2019-03-26T20:11:15Z

post-merge ACK 7abd2e6

(except the change to validation.cpp 🙈 )

Github-Pull: bitcoin#13541 Rebased-From: 6c0a6f7 Modifications: - Includes fix from bitcoin#15618 - Retains backward compatibility - Doesn't change all the unrelated tests

Github-Pull: bitcoin#13541 Rebased-From: 7abd2e6 Modifications: - Retains backward compatibility

Summary: * test: Refactor vout fetches in rpc_rawtransaction * wallet/rpc: add maxfeerate parameter to sendrawtransaction * wallet/rpc: add maxfeerate parameter to testmempoolaccept This is a backport of Core [[bitcoin/bitcoin#13541 | PR13541]] Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D6263

fanquake added RPC/REST/ZMQ Wallet labels Jun 27, 2018

kallewoof force-pushed the sendrawtransaction-maxfeerate branch from 718aa0b to 43d2cce Compare June 27, 2018 08:45

promag reviewed Jun 27, 2018

View reviewed changes

kallewoof force-pushed the sendrawtransaction-maxfeerate branch 2 times, most recently from bbb81b6 to a43e61d Compare June 27, 2018 22:58

kallewoof force-pushed the sendrawtransaction-maxfeerate branch 6 times, most recently from f9893d4 to bec1abd Compare July 13, 2018 07:28

kallewoof force-pushed the sendrawtransaction-maxfeerate branch 7 times, most recently from d562cce to dbd466c Compare July 31, 2018 06:37

DrahtBot mentioned this pull request Aug 12, 2018

Refactoring CRPCCommand with enum category #13945

Closed

DrahtBot mentioned this pull request Oct 20, 2018

Rpc help helper class #14502

Closed

practicalswift mentioned this pull request Oct 22, 2018

Enable flake8 rule E225 which checks for missing whitespace around op… #14534

Closed

meshcollider reviewed Nov 10, 2018

View reviewed changes

kallewoof force-pushed the sendrawtransaction-maxfeerate branch 2 times, most recently from 1a81743 to da1234d Compare November 12, 2018 07:04

kallewoof mentioned this pull request Nov 12, 2018

Linter errors for unrelated files on local repo but not on bitcoin/bitcoin #14709

Closed

DrahtBot added the Needs rebase label Nov 13, 2018

maflcko reviewed Mar 5, 2019

View reviewed changes

test/functional/feature_cltv.py Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Mar 5, 2019

test: Refactor vout fetches in rpc_rawtransaction

e5efacb

kallewoof force-pushed the sendrawtransaction-maxfeerate branch from b202e25 to 6c17985 Compare March 6, 2019 00:54

DrahtBot removed the Needs rebase label Mar 6, 2019

maflcko reviewed Mar 13, 2019

View reviewed changes

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

test/functional/rpc_rawtransaction.py Outdated Show resolved Hide resolved

kallewoof added 2 commits March 14, 2019 08:48

wallet/rpc: add maxfeerate parameter to sendrawtransaction

6c0a6f7

wallet/rpc: add maxfeerate parameter to testmempoolaccept

7abd2e6

kallewoof force-pushed the sendrawtransaction-maxfeerate branch from 6c17985 to 7abd2e6 Compare March 13, 2019 23:49

maflcko merged commit 7abd2e6 into bitcoin:master Mar 18, 2019

maflcko reviewed Mar 18, 2019

View reviewed changes

src/validation.cpp

}

}

return -1;

}

Copy link

Member

maflcko Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-utACK 7abd2e6

Sorry I only noticed this unused method in this hunk after merge 😿.

maflcko mentioned this pull request Mar 18, 2019

refactor: Remove unused function #15618

Merged

kallewoof deleted the sendrawtransaction-maxfeerate branch March 18, 2019 23:58

This was referenced Apr 17, 2019

rpcclient: support numeric maxfeerate in bitcoind sendrawtransaction lightningnetwork/lnd#2961

Closed

v0.18.0 testing #15555

Closed

rpcclient: support numeric maxfeerate in bitcoind sendrawtransaction btcsuite/btcwallet#611

Open

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Apr 22, 2019

wallet/rpc: add maxfeerate parameter to testmempoolaccept

b369af8

Github-Pull: bitcoin#13541 Rebased-From: 7abd2e6 Modifications: - Retains backward compatibility

promag mentioned this pull request Jul 16, 2019

sendrawtransaction maxfeerate is interpreted as absolute fee by default #16382

Closed

This was referenced Oct 30, 2019

btcjson+rpcclient: add compatibility for bitcoind v0.19.0 btcsuite/btcd#1484

Merged

build: bump btcsuite and neutrino dependencies to latest tagged versions lightningnetwork/lnd#3649

Merged

wakiyamap mentioned this pull request Nov 25, 2019

maxfeerateの導入について monacoinproject/monacoin#63

Closed

Conversation

kallewoof commented Jun 27, 2018

Uh oh!

NicolasDorier commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Aug 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Mar 5, 2019

Uh oh!

Uh oh!

maflcko commented Mar 7, 2019

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kallewoof commented Mar 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Mar 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

NicolasDorier commented Jun 27, 2018 •

edited

Loading

kallewoof commented Jun 27, 2018 •

edited

Loading

DrahtBot commented Aug 12, 2018 •

edited

Loading