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.
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)
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
maxtxfeeor havingmaxtxfeerateat config level would solve the issue without touching RPC. But, when you do a transaction manually, you know themaxfeerateto expect when you callsendrawtransaction, 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*thansendrawtransaction)
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.
385+ self.log.info('sendrawtransaction with maxfeerate')
386+
387+ addr = self.nodes[2].getnewaddress()
388+ txId = self.nodes[0].sendtoaddress(addr, 1.0)
389+ rawTx = self.nodes[0].getrawtransaction(txId, True)
390+ vout = False
= None?
Alternative (not tested)
0vout = next(o for o in rawTx['vout'] if o['value'] == Decimal('1.00000000'))
rpc_rawtransaction, so I made a dedicated commit to address the others.
1086@@ -1087,14 +1087,15 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
1087
1088 static UniValue sendrawtransaction(const JSONRPCRequest& request)
1089 {
1090- if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
1091+ if (request.fHelp || request.params.size() < 1 || request.params.size() > 3)
1092 throw std::runtime_error(
1093 "sendrawtransaction \"hexstring\" ( allowhighfees )\n"
1121@@ -1123,6 +1122,13 @@ static UniValue sendrawtransaction(const JSONRPCRequest& request)
1122 if (!request.params[1].isNull() && request.params[1].get_bool())
1123 nMaxRawTxFee = 0;
1124
1125+ if (!request.params[2].isNull()) {
1126+ size_t weight = GetTransactionWeight(*tx);
1127+ CFeeRate fr(AmountFromValue(request.params[2]));
1128+ CAmount max_fee = fr.GetFee((weight+3)/4);
1129+ nMaxRawTxFee = nMaxRawTxFee > 0 ? std::min(nMaxRawTxFee, max_fee) : max_fee;
maxfeerate does it make sense to pass allowhighfees=true?
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.
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.
does that work with named parameters?
Yes, it is already used by getbalance:
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
1089 "\nSubmits raw transaction (serialized, hex-encoded) to local node and network.\n"
1090 "\nAlso see createrawtransaction and signrawtransaction calls.\n"
1091 "\nArguments:\n"
1092 "1. \"hexstring\" (string, required) The hex string of the raw transaction)\n"
1093- "2. allowhighfees (boolean, optional, default=false) Allow high fees\n"
1094+ "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"
maxTxFee is, not ∞?
Fixed - it now outputs
02. allowhighfees|maxfeerate (boolean/numeric, optional, default=false/0.10) Allow
1high fees (boolean) or reject transactions whose fee rate is higher than the
2specified value (numeric), expressed in BTC/kB
for the default settings.
1118+ if (request.params[1].isBool()) {
1119+ if (request.params[1].get_bool()) nMaxRawTxFee = 0;
1120+ } else if (request.params[1].isNum()) {
1121+ size_t weight = GetTransactionWeight(*tx);
1122+ CFeeRate fr(AmountFromValue(request.params[1]));
1123+ nMaxRawTxFee = fr.GetFee((weight+3)/4);
+3/4 is for rounding
1021@@ -1021,7 +1022,7 @@ static UniValue sendrawtransaction(const JSONRPCRequest& request)
1022 "\nAlso see createrawtransaction and signrawtransactionwithkey calls.\n",
1023 {
1024 {"hexstring", RPCArg::Type::STR_HEX, /* opt */ false, /* default_val */ "", "The hex string of the raw transaction"},
1025- {"allowhighfees", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "false", "Allow high fees"},
1026+ {"maxfeerate", RPCArg::Type::AMOUNT, /* opt */ true, /* default_val */ FormatMoney(maxTxFee), "Allow high fees (boolean) or reject transactions whose fee rate is higher than the specified value (numeric), expressed in " + CURRENCY_UNIT + "/kB\n"},
0 {"maxfeerate", RPCArg::Type::AMOUNT, /* default */ FormatMoney(maxTxFee), "Allow high fees (boolean) or reject transactions whose fee rate is higher than the specified value (numeric), expressed in " + CURRENCY_UNIT + "/kB\n"},
testmempoolaccept (since that is just a mirror of sendraw)
maxTxFee from validation into wallet in fc29a5a – I don’t know if this should be a separate PR or not, but it seems this move is desired?1035@@ -1035,7 +1036,7 @@ static UniValue sendrawtransaction(const JSONRPCRequest& request)
1036 "\nAlso see createrawtransaction and signrawtransactionwithkey calls.\n",
1037 {
1038 {"hexstring", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The hex string of the raw transaction"},
1039- {"allowhighfees", RPCArg::Type::BOOL, /* default */ "false", "Allow high fees"},
1040+ {"maxfeerate", RPCArg::Type::AMOUNT, /* default */ FormatMoney(maxTxFee), "Allow high fees (boolean) or reject transactions whose fee rate is higher than the specified value (numeric), expressed in " + CURRENCY_UNIT + "/kB\n"},
My personal preference would be to not overload a parameter with two possible types and just remove the boolean option here. I think it’s ok to leave the boolean accepting logic below as a courtesy to existing users, but that should eventually be removed and it shouldn’t be advertised in the help text.
Same for testmempoolaccept below.
I’m hesitant to allow some hidden underlying default max fee when using a boolean. If we are removing the option, we should remove it completely, not just from the documentation.
I do realize that the way things are currently, someone who uses ‘1’ as boolean true will accept any fees up to 1 BTC/kB, which is rather high. Maybe removing the boolean style completely is actually the way to go.
Maybe removing the boolean style completely is actually the way to go.
I tend to agree with your idea. An option could be to have a -deprecatedrpc=sendrawtransaction -deprecatedrpc=testmempoolaccept to toggle back to the bool behavior.
An option could be to have a
-deprecatedrpc=sendrawtransaction
That’s one way, although I think it may be overkill. How about catching if a bool is passed, and then returning an error message telling the user to set ‘maxreerate’ to 0, which maintains exactly the same behaviour. Code:
0 // TODO: temporary migration code for old clients. Remove in v0.21
1 if (request.params[1].isBool()) {
2 throw JSONRPCError(RPC_INVALID_PARAMETER, "Second argument must be numeric (maxfeerate) and no longer supports a boolean. To allow a transaction with high fees, set maxfeerate to 0.");
3 } else if (request.params[1].isNum()) {
4 [...]
Or similar.
285@@ -286,11 +286,7 @@ def run_test(self):
286
287 txDetails = self.nodes[0].gettransaction(txId, True)
288 rawTx = self.nodes[0].decoderawtransaction(txDetails['hex'])
289- vout = False
290- for outpoint in rawTx['vout']:
291- if outpoint['value'] == Decimal('2.20000000'):
292- vout = outpoint
293- break
294+ vout = next(o for o in rawTx['vout'] if o['value'] == Decimal('2.20000000'))
StopIteration exception if there is no matching output, rather than fail later because vout is False.
426@@ -435,5 +427,35 @@ def run_test(self):
427 decrawtx = self.nodes[0].decoderawtransaction(rawtx)
428 assert_equal(decrawtx['version'], 0x7fffffff)
429
430+ #########################################################
log.info() calls are more useful for documenting the test sections than these block comments. Consider removing this.
436+ addr = self.nodes[2].getnewaddress()
437+ txId = self.nodes[0].sendtoaddress(addr, 1.0)
438+ rawTx = self.nodes[0].getrawtransaction(txId, True)
439+ vout = next(o for o in rawTx['vout'] if o['value'] == Decimal('1.00000000'))
440+
441+ self.sync_all()
sync_all() introduces a random delay (since transaction propagation is trickled). This is done in a bunch of test cases, so I’m not saying this is a blocker for this PR, but it’s something worth knowing about.
1068+ if (request.params[1].get_bool()) max_raw_tx_fee = 0;
1069+ } else if (request.params[1].isNum()) {
1070+ size_t weight = GetTransactionWeight(*tx);
1071+ CFeeRate fr(AmountFromValue(request.params[1]));
1072+ // the +3/4 part rounds the value up, and is the same formula used when
1073+ // calculating the fee for a transaction
GetVirtualTransactionSize() in this comment? (It took me some time to find and confirm this formula)
These changes seem reasonable, although I think the commits could be re-arranged and squashed. For example, the first two commits don’t cause any behaviour change and the third commit test: test maxfeerate parameter in sendrawtransaction tests behaviour that has not been implemented, meaning the test fails. I’d suggest the following:
BroadcastTransaction()sendrawtransaction together with test updates.testmempoolaccept together with test updates.
27@@ -28,6 +28,7 @@
28 #include <script/standard.h>
29 #include <uint256.h>
30 #include <util/bip32.h>
31+#include <util/moneystr.h>
in commit 9c6a8b52394c9fdbb8c2296a77614b1c0114db49
Why the include? I can’t see what it is for
FormatMoney in RPC helps.
I kind of prefer to have tests separate from code changes
I have no strong preference in the case of new tests, but obviously changes that need to be made to existing tests when behaviour changes should be atomic with the behaviour change commit to not break git bisects.
Sorry I was a bit overzelous in squashing. Obviously it’s your choice and I won’t be at all offended if you split out the new tests as part of your next rebase :)
REJECT_HIGHFEE and REJECT_INSUFFICIENTFEE, specifically). This also included updating AcceptToMempool and friends to allow an outgoing fee.
1055@@ -1055,20 +1056,30 @@ static UniValue sendrawtransaction(const JSONRPCRequest& request)
1056 },
1057 }.ToString());
1058
1059- RPCTypeCheck(request.params, {UniValue::VSTR, UniValue::VBOOL});
in commit 9e6f7875f27e632fe38518cff59ff61ce48dded6
Why is this removed, it should stay here with the comment “checked later”. See also git grep 'checked later'
1077+ // the +3/4 part rounds the value up, and is the same formula used when
1078+ // calculating the fee for a transaction
1079+ // (see GetVirtualTransactionSize)
1080+ max_raw_tx_fee = fr.GetFee((weight+3)/4);
1081+ } else if (!request.params[1].isNull()) {
1082+ throw JSONRPCError(RPC_INVALID_PARAMETER, "second argument must be a bool (allowhighfees) or numeric (maxfeerate)");
In commit 9e6f7875f27e632fe38518cff59ff61ce48dded6
Wrong error message. Could remove the isBool check above and use that error message?
rpc_deprecated or anywhere else?
isBool check above, so it can be removed cleanly in a future version.
64@@ -65,7 +65,7 @@ def small_txpuzzle_randfee(from_node, conflist, unconflist, amount, min_fee, fee
65 # the ScriptSig that will satisfy the ScriptPubKey.
66 for inp in tx.vin:
67 inp.scriptSig = SCRIPT_SIG[inp.prevout.n]
68- txid = from_node.sendrawtransaction(ToHex(tx), True)
69+ txid = from_node.sendrawtransaction(ToHex(tx), 1.0)
in commit 9e6f7875f27e632fe38518cff59ff61ce48dded6
nit: Should be 0 and use named args?
94@@ -95,7 +95,7 @@ def split_inputs(from_node, txins, txouts, initial_split=False):
95 else:
96 tx.vin[0].scriptSig = SCRIPT_SIG[prevtxout["vout"]]
97 completetx = ToHex(tx)
98- txid = from_node.sendrawtransaction(completetx, True)
99+ txid = from_node.sendrawtransaction(completetx, 1.0)
1126@@ -1127,7 +1127,6 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request)
1127 }.ToString());
1128 }
1129
1130- RPCTypeCheck(request.params, {UniValue::VARR, UniValue::VBOOL});
In commit 685f2f54866e14f79d56c1220abbd37e23b709d8
Same as above
1151+ // the +3/4 part rounds the value up, and is the same formula used when
1152+ // calculating the fee for a transaction
1153+ // (see GetVirtualTransactionSize)
1154+ max_raw_tx_fee = fr.GetFee((weight+3)/4);
1155+ } else if (!request.params[1].isNull()) {
1156+ throw JSONRPCError(RPC_INVALID_PARAMETER, "second argument must be a bool (allowhighfees) or numeric (maxfeerate)");
in commit 685f2f54866e14f79d56c1220abbd37e23b709d8
Same as above
1186+ } else {
1187+ CFeeRate fr_shortage(max_raw_tx_fee - fee_out, (weight+3)/4);
1188+ fees.pushKV("fee-abs-shortage", ValueFromAmount(max_raw_tx_fee - fee_out));
1189+ fees.pushKV("fee-rate-shortage", ValueFromAmount(fr_shortage.GetFeePerK()));
1190+ }
1191+ result_0.pushKV("fees", fees);
in commit 349860df8695ff0ad657a3d6d1502edb46571174
This is undocumented, but I am also not sure if it is a desired feature. Might want to split out as a separate pull request after the first part is merged?
94@@ -95,8 +95,10 @@ static const CRPCConvertParam vRPCConvertParams[] =
95 { "signrawtransactionwithkey", 2, "prevtxs" },
96 { "signrawtransactionwithwallet", 1, "prevtxs" },
97 { "sendrawtransaction", 1, "allowhighfees" },
testmempoolaccept below)
2088@@ -2047,11 +2089,11 @@ static const CRPCCommand commands[] =
2089 { "rawtransactions", "createrawtransaction", &createrawtransaction, {"inputs","outputs","locktime","replaceable"} },
2090 { "rawtransactions", "decoderawtransaction", &decoderawtransaction, {"hexstring","iswitness"} },
2091 { "rawtransactions", "decodescript", &decodescript, {"hexstring"} },
2092- { "rawtransactions", "sendrawtransaction", &sendrawtransaction, {"hexstring","allowhighfees"} },
2093+ { "rawtransactions", "sendrawtransaction", &sendrawtransaction, {"hexstring","allowhighfees|maxfeerate"} },
allowhighfees (and same for testmempoolaccept below)
112@@ -113,7 +113,7 @@ def run_test(self):
113 # rejected from the mempool for exactly that reason.
114 assert_equal(
115 [{'txid': spendtx.hash, 'allowed': False, 'reject-reason': '64: non-mandatory-script-verify-flag (Negative locktime)'}],
116- self.nodes[0].testmempoolaccept(rawtxs=[bytes_to_hex_str(spendtx.serialize())], allowhighfees=True)
117+ self.nodes[0].testmempoolaccept(rawtxs=[bytes_to_hex_str(spendtx.serialize())], maxfeerate=0)
bytes_to_hex_str has been removed. Sorry, needs rebase again :(
1071
1072- bool allowhighfees = false;
1073- if (!request.params[1].isNull()) allowhighfees = request.params[1].get_bool();
1074- const CAmount highfee{allowhighfees ? 0 : ::maxTxFee};
1075+ CAmount max_raw_tx_fee = maxTxFee;
1076+ // TODO: temporary migration code for old clients. Remove in v0.21
0 // TODO: temporary migration code for old clients. Remove in v0.20
Released in 0.19.0, so can be removed in 0.20.0?
425@@ -434,5 +426,31 @@ def run_test(self):
426 decrawtx = self.nodes[0].decoderawtransaction(rawtx)
427 assert_equal(decrawtx['version'], 0x7fffffff)
428
429+ self.log.info('sendrawtransaction/testmempoolaccept with maxfeerate')
430+
431+ addr = self.nodes[2].getnewaddress()
432+ txId = self.nodes[0].sendtoaddress(addr, 1.0)
0 txId = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 1.0)
Could inline addr
3146+ return (int)o;
3147+ }
3148+ }
3149+ }
3150+ return -1;
3151+}
re-utACK 7abd2e697c0f8e93245e09ac853bae05d0b48bee
Sorry I only noticed this unused method in this hunk after merge :crying_cat_face:.
post-merge ACK 7abd2e697c0f8e93245e09ac853bae05d0b48bee
(except the change to validation.cpp :see_no_evil: )
Labels
Wallet
RPC/REST/ZMQ
Milestone
0.19.0