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

wallet/rpc: sendrawtransaction maxfeerate #13541

Merged
merged 3 commits into from Mar 18, 2019

Conversation

Projects
None yet
9 participants
@kallewoof
Copy link
Member

kallewoof commented Jun 27, 2018

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.

@kallewoof kallewoof force-pushed the kallewoof:sendrawtransaction-maxfeerate branch Jun 27, 2018

@NicolasDorier

This comment has been minimized.

Copy link
Member

NicolasDorier commented Jun 27, 2018

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

This comment has been minimized.

Copy link
Member Author

kallewoof commented Jun 27, 2018

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.

test/functional/rpc_rawtransaction.py Outdated
addr = self.nodes[2].getnewaddress()
txId = self.nodes[0].sendtoaddress(addr, 1.0)
rawTx = self.nodes[0].getrawtransaction(txId, True)
vout = False

This comment has been minimized.

Copy link
@promag

promag Jun 27, 2018

Member

= None?

This comment has been minimized.

Copy link
@promag

promag Jun 27, 2018

Member

Alternative (not tested)

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

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jun 27, 2018

Author Member

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

src/rpc/rawtransaction.cpp Outdated
@@ -1087,14 +1087,15 @@ UniValue signrawtransaction(const JSONRPCRequest& request)

static UniValue sendrawtransaction(const JSONRPCRequest& request)
{
if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
if (request.fHelp || request.params.size() < 1 || request.params.size() > 3)
throw std::runtime_error(
"sendrawtransaction \"hexstring\" ( allowhighfees )\n"

This comment has been minimized.

Copy link
@promag

promag Jun 27, 2018

Member

Missing parameter.

src/rpc/rawtransaction.cpp Outdated
size_t weight = GetTransactionWeight(*tx);
CFeeRate fr(AmountFromValue(request.params[2]));
CAmount max_fee = fr.GetFee((weight+3)/4);
nMaxRawTxFee = nMaxRawTxFee > 0 ? std::min(nMaxRawTxFee, max_fee) : max_fee;

This comment has been minimized.

Copy link
@promag

promag Jun 27, 2018

Member

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

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jun 27, 2018

Author Member

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.

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jun 27, 2018

Author Member

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.

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jul 29, 2018

Member

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

This comment has been minimized.

Copy link
@promag

promag Jul 29, 2018

Member

@luke-jr does that work with named parameters?

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jul 30, 2018

Author Member

does that work with named parameters?

Yes, it is already used by getbalance:

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

@kallewoof kallewoof force-pushed the kallewoof:sendrawtransaction-maxfeerate branch 2 times, most recently Jun 27, 2018

@kallewoof kallewoof force-pushed the kallewoof:sendrawtransaction-maxfeerate branch 6 times, most recently Jul 12, 2018

@kallewoof kallewoof force-pushed the kallewoof:sendrawtransaction-maxfeerate branch 7 times, most recently Jul 30, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Aug 12, 2018

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
Copy link
Member

meshcollider left a comment

utACK dbd466c

src/rpc/rawtransaction.cpp Outdated
"\nSubmits raw transaction (serialized, hex-encoded) to local node and network.\n"
"\nAlso see createrawtransaction and signrawtransaction calls.\n"
"\nArguments:\n"
"1. \"hexstring\" (string, required) The hex string of the raw transaction)\n"
"2. allowhighfees (boolean, optional, default=false) Allow high fees\n"
"2. allowhighfees|maxfeerate (boolean/numeric, optional, default=false/∞) Allow high fees (boolean) or reject transactions whose fee rate is higher than the specified value (numeric), expressed in " + CURRENCY_UNIT + "/kB\n"

This comment has been minimized.

Copy link
@meshcollider

meshcollider Nov 10, 2018

Member

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

This comment has been minimized.

Copy link
@kallewoof

kallewoof Nov 12, 2018

Author Member

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.

src/rpc/rawtransaction.cpp Outdated
} else if (request.params[1].isNum()) {
size_t weight = GetTransactionWeight(*tx);
CFeeRate fr(AmountFromValue(request.params[1]));
nMaxRawTxFee = fr.GetFee((weight+3)/4);

This comment has been minimized.

Copy link
@meshcollider

meshcollider Nov 10, 2018

Member

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

This comment has been minimized.

Copy link
@kallewoof

kallewoof Nov 12, 2018

Author Member

Added note.

@kallewoof kallewoof force-pushed the kallewoof:sendrawtransaction-maxfeerate branch 2 times, most recently Nov 12, 2018

@MarcoFalke
Copy link
Member

MarcoFalke left a comment

utACK 685f2f5

Show resolved Hide resolved src/rpc/rawtransaction.cpp
Show resolved Hide resolved src/rpc/rawtransaction.cpp Outdated
Show resolved Hide resolved test/functional/feature_fee_estimation.py Outdated
Show resolved Hide resolved test/functional/feature_fee_estimation.py Outdated
Show resolved Hide resolved src/rpc/rawtransaction.cpp
Show resolved Hide resolved src/rpc/rawtransaction.cpp Outdated
Show resolved Hide resolved src/rpc/rawtransaction.cpp Outdated
@@ -95,8 +95,10 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "signrawtransactionwithkey", 2, "prevtxs" },
{ "signrawtransactionwithwallet", 1, "prevtxs" },
{ "sendrawtransaction", 1, "allowhighfees" },

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 27, 2019

Member

remove this (and same for testmempoolaccept below)

This comment has been minimized.

Copy link
@kallewoof

kallewoof Mar 4, 2019

Author Member

I think it makes sense to keep until we remove the bool check, in case somebody is using named style, or is that unnecessary?

Show resolved Hide resolved src/rpc/rawtransaction.cpp Outdated
@@ -2047,11 +2089,11 @@ static const CRPCCommand commands[] =
{ "rawtransactions", "createrawtransaction", &createrawtransaction, {"inputs","outputs","locktime","replaceable"} },
{ "rawtransactions", "decoderawtransaction", &decoderawtransaction, {"hexstring","iswitness"} },
{ "rawtransactions", "decodescript", &decodescript, {"hexstring"} },
{ "rawtransactions", "sendrawtransaction", &sendrawtransaction, {"hexstring","allowhighfees"} },
{ "rawtransactions", "sendrawtransaction", &sendrawtransaction, {"hexstring","allowhighfees|maxfeerate"} },

This comment has been minimized.

Copy link
@jnewbery

jnewbery Feb 27, 2019

Member

remove allowhighfees (and same for testmempoolaccept below)

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

@kallewoof kallewoof force-pushed the kallewoof:sendrawtransaction-maxfeerate branch from 349860d to e57fcaf Mar 4, 2019

@kallewoof

This comment has been minimized.

Copy link
Member Author

kallewoof commented Mar 4, 2019

Addressed review comments and removed fee info related commits
121afc4
349860d

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Mar 4, 2019

Could you fixup the last commit, please?

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Mar 4, 2019

re-utACK 6c98fd0 otherwise

@kallewoof kallewoof force-pushed the kallewoof:sendrawtransaction-maxfeerate branch from 6c98fd0 to b202e25 Mar 5, 2019

@kallewoof

This comment has been minimized.

Copy link
Member Author

kallewoof commented Mar 5, 2019

@MarcoFalke Squashed.

@kallewoof kallewoof force-pushed the kallewoof:sendrawtransaction-maxfeerate branch from b202e25 to 6c17985 Mar 6, 2019

@DrahtBot DrahtBot removed the Needs rebase label Mar 6, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Mar 7, 2019

re-utACK 6c17985

@MarcoFalke
Copy link
Member

MarcoFalke left a comment

Just some style nits (could be ignored)

Show resolved Hide resolved src/rpc/rawtransaction.cpp Outdated
Show resolved Hide resolved test/functional/rpc_rawtransaction.py Outdated

kallewoof added some commits Jun 27, 2018

@kallewoof kallewoof force-pushed the kallewoof:sendrawtransaction-maxfeerate branch from 6c17985 to 7abd2e6 Mar 13, 2019

@kallewoof

This comment has been minimized.

Copy link
Member Author

kallewoof commented Mar 14, 2019

Addressed @MarcoFalke nits.

@MarcoFalke MarcoFalke merged commit 7abd2e6 into bitcoin:master Mar 18, 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 Mar 18, 2019

Merge #13541: wallet/rpc: sendrawtransaction maxfeerate
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
}
}
return -1;
}

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Mar 18, 2019

Member

re-utACK 7abd2e6

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

laanwj added a commit that referenced this pull request Mar 18, 2019

Merge #15618: refactor: Remove unused function
fa5c511 refactor: Remove unused function (MarcoFalke)

Pull request description:

  Oversight of kallewoof and mine in #13541 (comment)

Tree-SHA512: 2fd3c4ecde5d3c58b113aa58d606976ceb4998358bde0547ead8e83df210722fa9821d2c88b717bdd190ef71593cd9c0154c3a5d3f2ccc3af8cbf6c36aaa6d45

@kallewoof kallewoof deleted the kallewoof:sendrawtransaction-maxfeerate branch Mar 18, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Mar 26, 2019

post-merge ACK 7abd2e6

(except the change to validation.cpp 🙈 )

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.