Fixes #16382
This patch tries to treat maxfeerate in sendrawtransaction/testmempoolaccept RPC as a rate(BTC/kB) instead of an absolute value(BTC).
The included test case checks if the new behavior works correctly, by using the transaction with an absolute fee of ~0.02BTC, where the fee rate is ~0.2BTC/kB.
This test should be failing if the default maxfeerate is 0.1BTC, but pass if the default value is 0.1BTC/kB
43+/** High fee rate for sendrawtransaction and testmempoolaccept.
44+ * By default, transaction with a fee rate higher than this will be rejected by
45+ * the RPCs. This can be overridden with the maxfeerate argument.
46 */
47-constexpr static CAmount DEFAULT_MAX_RAW_TX_FEE{COIN / 10};
48+constexpr static CAmount DEFAULT_MAX_RAW_TX_FEE_RATE{COIN / 10};
Can the type be changed as well?
0const static CFeeRate DEFAULT_MAX_RAW_TX_FEE_RATE{COIN / 10};
CFeeRate can’t be constexpr since it doesn’t have constexpr constructor.
We can change it to const though.
static const is more than enough for our purposes
static const here.
I’ll fix this.
874@@ -873,19 +875,21 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request)
875 CTransactionRef tx(MakeTransactionRef(std::move(mtx)));
876 const uint256& tx_hash = tx->GetHash();
877
878- CAmount max_raw_tx_fee = DEFAULT_MAX_RAW_TX_FEE;
879+ CAmount max_raw_tx_fee_rate = DEFAULT_MAX_RAW_TX_FEE_RATE;
0 CFeeRate max_raw_tx_fee_rate{DEFAULT_MAX_RAW_TX_FEE_RATE};
445 assert_equal(rawTxSigned['complete'], True)
446- # 1000 sat fee, ~100 b transaction, fee rate should land around 10 sat/b = 0.00010000 BTC/kB
447+ # 2000000 sat fee, ~100 b transaction, fee rate should land around 20000 sat/b = 0.20000000 BTC/kB
448 # Thus, testmempoolaccept should reject
449- testres = self.nodes[2].testmempoolaccept([rawTxSigned['hex']], 0.00001000)[0]
450+ testres = self.nodes[2].testmempoolaccept([rawTxSigned['hex']], 0.02000000)[0]
458 # And below calls should both succeed
459- testres = self.nodes[2].testmempoolaccept(rawtxs=[rawTxSigned['hex']], maxfeerate='0.00070000')[0]
460+ testres = self.nodes[2].testmempoolaccept(rawtxs=[rawTxSigned['hex']], maxfeerate='0.20000000')[0]
461 assert_equal(testres['allowed'], True)
462- self.nodes[2].sendrawtransaction(hexstring=rawTxSigned['hex'], maxfeerate='0.00070000')
463+ self.nodes[2].sendrawtransaction(hexstring=rawTxSigned['hex'], maxfeerate='0.20000000')
maxfeerate was provided.
Since the amount of fee attached to the transaction was changed, maxfeerate should be increased too.
437@@ -438,21 +438,23 @@ def run_test(self):
438
439 self.sync_all()
440 inputs = [{ "txid" : txId, "vout" : vout['n'] }]
441- outputs = { self.nodes[0].getnewaddress() : Decimal("0.99999000") } # 1000 sat fee
442+ outputs = { self.nodes[0].getnewaddress() : Decimal("0.98000000") } # 2000000 sat fee
Let me explain it a bit in detail.
I wanted to test if a transaction with sufficiently high fee rate gets rejected if no maxfeerate was specified.
I could’ve created a new transaction with a high fee and run test with it, but I thought it would be nice if I can reuse the existing code so that the change becomes minimal.
The existing rawTx’s fee was too low for that, but there should be no difference if I change the related values altogether.
So I increased the fee from 1000 sat to 0.02 BTC, and other maxfeerate values accordingly.
I wanted to test if a transaction with sufficiently high fee rate gets rejected if no maxfeerate was specified.
This is tested in other functional tests already. (I presume you can find them by removing the failure in the source code, re-compile and re-run the tests)
I changed my code as following, but couldn’t find any failing test. Could you point the exact test you’re referring to?
0+ CFeeRate max_raw_tx_fee_rate{DEFAULT_MAX_RAW_TX_FEE_RATE * 1000000};
1- CFeeRate max_raw_tx_fee_rate{DEFAULT_MAX_RAW_TX_FEE_RATE};
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
Concept ACK.
AcceptToMemoryPool could be changed to receive absurd_fee_rate instead? Then computing the transaction fee could be moved there (reducing duplicate code) and DEFAULT_MAX_FEE_RATE could be moved to validation.h.
Should backport this?
@MarcoFalke sorry for not being clear on my comment, I wasn’t suggesting to do that change here, hence the backport question.
Thanks for referencing #15810.
817 }
818
819+ size_t weight = GetTransactionWeight(*tx);
820+ // the +3/4 part rounds the value up, and is the same formula used when
821+ // calculating the fee for a transaction
822+ // (see GetVirtualTransactionSize)
GetVirtualTransactionSize instead of copy-pasting the constants here?
894 }
895
896+ size_t weight = GetTransactionWeight(*tx);
897+ // the +3/4 part rounds the value up, and is the same formula used when
898+ // calculating the fee for a transaction
899+ // (see GetVirtualTransactionSize)
239@@ -240,7 +240,7 @@ def run_test(self):
240 tx.vin.append(CTxIn(COutPoint(int(txid2, 16), 0), b""))
241 tx.vout.append(CTxOut(int(49.95 * COIN), CScript([OP_TRUE, OP_DROP] * 15 + [OP_TRUE]))) # Huge fee
242 tx.calc_sha256()
243- txid3 = self.nodes[0].sendrawtransaction(ToHex(tx))
244+ txid3 = self.nodes[0].sendrawtransaction(ToHex(tx), 0)
sendrawtransaction doesn’t use named parameter for maxfeerate.
Do you have any reason for using different convention here?
432@@ -433,7 +433,7 @@ def run_test(self):
433 # Split into two chains
434 rawtx = self.nodes[0].createrawtransaction([{"txid": singletxid, "vout": 0}], {chain_addrs[0]: node0_balance / 2 - Decimal('0.01'), chain_addrs[1]: node0_balance / 2 - Decimal('0.01')})
435 signedtx = self.nodes[0].signrawtransactionwithwallet(rawtx)
436- singletxid = self.nodes[0].sendrawtransaction(signedtx["hex"])
437+ singletxid = self.nodes[0].sendrawtransaction(signedtx["hex"], 0)
I restarted Travis for you.
ACK 7335bd3, though I suggest having the RPC code catch absurdly-high-fee from validation and change it to absurdly-high-fee-rate.
The rename from DEFAULT_MAX_RAW_TX_FEE to DEFAULT_MAX_RAW_TX_FEE_RATE adds a much needed clarification. It’s less easy to confuse with DEFAULT_TRANSACTION_MAXFEE (-maxtxfee).
We’re still in a bit of weird situation, but it’s fine for now: fundrawtransaction has a feeRate arguments and checks absolute fee against -maxtxfee, while sendrawtransaction checks the feeRate (against maxfeerate), and doesn’t check the absolute fee.
442+ outputs = { self.nodes[0].getnewaddress() : Decimal("0.98000000") } # 2000000 sat fee
443 rawTx = self.nodes[2].createrawtransaction(inputs, outputs)
444 rawTxSigned = self.nodes[2].signrawtransactionwithwallet(rawTx)
445 assert_equal(rawTxSigned['complete'], True)
446- # 1000 sat fee, ~100 b transaction, fee rate should land around 10 sat/b = 0.00010000 BTC/kB
447+ # 2000000 sat fee, ~100 b transaction, fee rate should land around 20000 sat/b = 0.20000000 BTC/kB
ACK 49b1f4811735f523571f3e99abd58b61cd1ac3e8
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK 49b1f4811735f523571f3e99abd58b61cd1ac3e8
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUh4Hgv+KIUo919Iwy0K/MPxGM6EE+WdBPrO/cqSixufYn5kMZ5+goPcBAJ8v5O9
8OoC4SzTmMKwl6qHJPgeBp3EGjubgD1Up5KBepQq70zOHdTmukvA3tn1nepc7w/GI
9nkJ9AYoXpivFI8MTabuHDNa0/0psBv1nteKsBNcow4rKSb8RSuf2Itu0HT5a9t2H
107VcvYcsQjNizgafy7FHuahYa855UwkdjGdpRntqxDqViDr69FErQI/VurM0YtrCZ
118PiFA3i++igwMpgnsDBq0Guc+Ng2cqmyVIVVF3GlfEhHSZQPJbfopSeCfArdcgdY
12TneK5tbSL/q0rRWIMOwBUoT2iuTv297E7rIFkAz0lg0STuOuIws5WfrbtE35Guv+
13zpyRIhGS3a8YWCuSF3ZnwnqscOp/IpWt3ojzOCN/tbQO6JUi33WhYLP1BnE7Wnkl
14hZDmDlSdniqjfqK9HR0754ia4Po5bQCGxJnNO5h9D4abpkb8I9Rjzty8agfZYwwW
15kPsjLD0H
16=AiEa
17-----END PGP SIGNATURE-----
Timestamp of file with hash 9a3ed53c1fee1a38984565afc1e5e4de52ffa6dde5c6b84909b5d9645e01d6d1 -
This patch adds test for the following case
- maxfeerate is omitted, and the actual fee rate is higher than
the default value(0.1 BTC/kB)
44+/** High fee rate for sendrawtransaction and testmempoolaccept.
45+ * By default, transaction with a fee rate higher than this will be rejected by
46+ * the RPCs. This can be overridden with the maxfeerate argument.
47 */
48-constexpr static CAmount DEFAULT_MAX_RAW_TX_FEE{COIN / 10};
49+const static CFeeRate DEFAULT_MAX_RAW_TX_FEE_RATE{COIN / 10};
static const … is typically used instead of const static … :-)
post-merge re-ACK 2dfd6834ef8737e16e4b96df0c459f30a0721d6c (only change is style nit)
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3post-merge re-ACK 2dfd6834ef8737e16e4b96df0c459f30a0721d6c (only change is style nit)
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUhodQv+N39HguRkVUKt6PzE2gGZ6BR0e9Jwypn08WD0kWVsrRwPOZzdpq/HBUys
8Ispph9JBh3EXGFiBTwOPaybH1yLAEyyAvvFd0/sJifOblu3QfkKwwr8b81/+s8lE
98SV+vwi0TWMVvSrq42DEdPtXlA65EhjBfRkuF4o58D7PtXa3l5keKbQGrVfZqnTb
10OFZc0ZaWrjXd3cwhJo/W5SE4RHXSKxydq5qipkqYhCQ59y2M+F41WMHH89Q92jgO
113iBUREvJb7/Yix4aM4DPK06DGaVUx5P7NxmJqsJfI4N4VY2QQF0MejpZRbe38Ddc
12iVW69qby89m5VXIuc+boktMkfsqMI4YcUUTjLetI3tLGjWm//U2HLJRa1RHSAHhS
13eHQ47zx7f+sNpFextdDmX2rTlGK7YO3j9b8dsaw4fLPgvy9ydENhqeTzX51cMtCi
14D39F36riarcGlBRto14j0txq21+LeqBI4qdRy/pBCmgcneN5ktWnKfsqQK4hXTs9
15pmvpv7um
16=2xaT
17-----END PGP SIGNATURE-----
Timestamp of file with hash fbaf9ebc856aec97f9850a70c2b6a2435c1b4ab700db0b49752d9eb74c89975e -
438 vout = next(o for o in rawTx['vout'] if o['value'] == Decimal('1.00000000'))
439
440 self.sync_all()
441 inputs = [{ "txid" : txId, "vout" : vout['n'] }]
442- outputs = { self.nodes[0].getnewaddress() : Decimal("0.99999000") } # 1000 sat fee
443+ outputs = { self.nodes[0].getnewaddress() : Decimal("0.999990000") } # 10000 sat fee
eh, actually this is incorrect. Appending zeros after the number after the decimal point does not change the number.
0>>> 0.9 == 0.90
1True
0.99990000
Labels
Tests
RPC/REST/ZMQ
Milestone
0.19.0