RPC: Add maxfeerate and maxburnamount args to submitpackage

instagibbs commented at 7:54 pm on November 27, 2023: member

Resolves #28949

I couldn’t manage to do it very cleanly outside of (sub)package evaluation itself, since it would change the current interface very heavily. Instead I threaded through the max fee argument and used that directly via ATMPArgs. From that perspective, this is somewhat a reversion from #19339. In a post-cluster mempool world, these checks could be consolidated to right after the given (ancestor) package is linearized/chunked, by just checking the feerate of the top chunk and rejecting the submission entirely if the top chunk is too high.

The implication here is that subpackages can be submitted to the mempool prior to hitting this new fee-based error condition.

DrahtBot commented at 8:24 pm on November 27, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	glozow, murchandamus, ismaelsadeeq
Concept ACK	kashifs
Stale ACK	achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29642 (kernel: Handle fatal errors through return values by TheCharlatan)

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.

DrahtBot added the label RPC/REST/ZMQ on Nov 27, 2023

instagibbs force-pushed on Nov 27, 2023

instagibbs force-pushed on Nov 28, 2023

instagibbs force-pushed on Dec 1, 2023

instagibbs commented at 5:46 pm on December 1, 2023: member

rebased on #28848 and ready for review

ismaelsadeeq commented at 9:56 am on December 20, 2023: member

Concept ACK

glozow commented at 11:59 am on December 20, 2023: member

Concept ACK

My ideal approach would be to not check client-side stuff inside validation, e.g. by adding a helper that loads coins (probably not extensible to chunk feerate but could maybe be a sufficient sanity check), or a test package accept + real submit (like the way BroadcastTransaction does it). But if it’s unavoidable, then this seems fine. AFAICT it’s extensible to checking chunk feerate in the future.

kashifs commented at 8:52 pm on December 25, 2023: contributor

Concept ACK

I compiled from source, read through each modified line of code, ran the automated tests, and modified test_maxfeerate_maxburn_submitpackage test to check maxfeerate and maxburnamount seperately

in src/rpc/mempool.cpp:184 in e650eed788 outdated

182@@ -183,7 +183,7 @@ static RPCHelpMan testmempoolaccept()
183             Chainstate& chainstate = chainman.ActiveChainstate();
184             const PackageMempoolAcceptResult package_result = [&] {
185                 LOCK(::cs_main);
186-                if (txns.size() > 1) return ProcessNewPackage(chainstate, mempool, txns, /*test_accept=*/true);
187+                if (txns.size() > 1) return ProcessNewPackage(chainstate, mempool, txns, /*test_accept=*/true, /*max_sane_feerate=*/{});

ismaelsadeeq commented at 10:16 am on January 8, 2024:

Should we test maxfeerate and maxburnamount for testmempoolaccept with a package also?

instagibbs commented at 3:25 pm on January 8, 2024:

testmempoolaccept is basically shoving all transactions, whether or not they’re a pacakge, into AcceptMultipleTransactions.

I’m not sure it makes sense due to this.

in src/validation.cpp:1316 in e650eed788 outdated

1311@@ -1295,6 +1312,15 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
1312             results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state));
1313             return PackageMempoolAcceptResult(package_state, std::move(results));
1314         }
1315+
1316+        // Individual modified feerate exceeded caller-deemed "sane" max; abort

ismaelsadeeq commented at 11:02 am on January 8, 2024:

My understanding of package submission is that a child with unconfirmed parents is bundled together as a package for incentive compatibility. Should this instead check the package fee rate against maxfeerate?

If I understand correctly, currently, we are checking the package transaction fee rate individually against maxfeerate. The child transactions might be greater than the maxfeerate, but as a package might not be. In this case, we might reject the child transactions and accept the subpackage, which might not be incentive compatible at the current fee estimates.

For example, suppose we want to fee bump tx A, with the arrangement below. maxfeerate is set to 50 s/vb.

 0               All txs are 10vb
 1               first value is absolute fee
 2               second value is ancestor score
 3               ┌───────────┐
 4               │  A (10s)  │
 5               │ 1 s/vb    │
 6               └───────────┘
 7                     │
 8                     ▼
 9               ┌───────────┐
10               │  B (90s)  │
11               │ 5 s/vb    │
12               └───────────┘
13                     │
14                     ▼
15               ┌────────────┐
16               │  C (1000s) │
17               │ 36.6 s/vb  │
18               └────────────┘

If maxfeerate is 50s/vb we will accept A-B and reject C because it’s fee rate is 100s/vb. But if we are evaluating maxfeerate against package fee rate we will accept the whole package A-B-C

instagibbs commented at 3:32 pm on January 8, 2024:

Ignoring topo restrictions in AcceptPackage since C should have A as a direct ancestor,

The child transactions might be greater than the maxfeerate, but as a package might not be. In this case, we might reject the child transactions and accept the subpackage, which might not be incentive compatible at the current fee estimates.

This is prior to any relay, of course restricting feerates is incentive compatible once published on the open network!

If maxfeerate is 50s/vb we will accept A-B and reject C because it’s ancestor score is 100s/vb. But if we are evaluating maxfeerate against package fee rate we will accept the whole package A-B-C

Assuming you meant C’s feerate is 100 sat/vbyte: If mempool minfee is 5 sat/vbyte, A+B would be accepted(as they’d be evaluated in their own subpackage already), then C evaluated on its own AcceptSingleTransaction, and rejected.

In a post-cluster mempool world, this would likely be reduced to evaluating chunks only, and so you’d look at A+B+C only, and reject the whole thing.

ismaelsadeeq commented at 10:38 am on January 10, 2024:

Assuming you meant C’s feerate is 100 sat/vbyte

Yes C fee rated is 100s/vb, edited thanks.

This is prior to any relay, of course restricting feerates is incentive compatible once published on the open network

Do you mean incentive incompatible here? because in the given example if C is rejected (A,B) will be added to the mempool with mining score of 5s/vb which is lower than whats intended.

instagibbs commented at 2:32 pm on January 10, 2024:

Do you mean incentive incompatible here? because in the given example if C is rejected (A,B) will be added to the mempool with mining score of 5s/vb which is lower than whats intended.

correct, typo

ismaelsadeeq commented at 7:24 am on February 10, 2024:

I understand why we should not check the package fee rate here because, in some arrangements, e.g., a parent transaction P will be cfp’ed by some child E, and E can have another child F, which is on its own. Checking the package fee rate in this case is incorrect; we should instead evaluate using chunks fee rate later, as you said 👍🏾

in src/rpc/client.cpp:132 in e650eed788 outdated

126@@ -127,6 +127,8 @@ static const CRPCConvertParam vRPCConvertParams[] =
127     { "testmempoolaccept", 0, "rawtxs" },
128     { "testmempoolaccept", 1, "maxfeerate" },
129     { "submitpackage", 0, "package" },
130+    { "submitpackage", 1, "maxfeerate" },
131+    { "submitpackage", 2, "maxburnamount" },

ismaelsadeeq commented at 11:04 am on January 8, 2024:

Should we just enable passing maxfeerate and maxburnamount as a config variables since it is used three RPC’s now sendrawtransaction, testmempoolaccept and submitpackage, downstream might prefer this?

instagibbs commented at 3:35 pm on January 8, 2024:

startup config? Maybe? Could open an issue

ismaelsadeeq commented at 10:49 am on January 10, 2024:

Done in https://github.com/bitcoin/bitcoin/issues/29217

ismaelsadeeq commented at 7:16 am on February 10, 2024:

Note a good idea, because we have to restart whenever we want to update the threshold for an RPC call, can be resolved.

in src/rpc/mempool.cpp:884 in e650eed788 outdated

885+                max_sane_feerate = std::nullopt;
886+            }
887+
888+            // Burn sanity check is run with no context
889+            const CAmount max_burn_amount = request.params[2].isNull() ? 0 : AmountFromValue(request.params[2]);
890+

ismaelsadeeq commented at 11:09 am on January 8, 2024:

We did not document this anywhere but the default behavior if maxburnamount is not passed we are rejecting the package if there is any transactions with unspendable output greater than 0. Maybe indicate it in the RPCHelpMan of maxburnamount?

Its also not indicated for sendrawtransaction.

instagibbs commented at 4:20 pm on January 8, 2024:

added a mention of the default being 0

in test/functional/rpc_packages.py:382 in e650eed788 outdated

377+        chain_hex = [chain_hex[0], tx.serialize().hex()]
378+        # burn test is run before any package evaluation; nothing makes it in and we get broader exception
379+        assert_raises_rpc_error(-25, "Unspendable output exceeds maximum configured by user", node.submitpackage, chain_hex, 0, 0.1)
380+        assert_equal(node.getrawmempool(), [])
381+
382+        # Turn off restrictions for both and send it; parent gets through as own subpackage

ismaelsadeeq commented at 11:12 am on January 8, 2024:

To turn off for maxfeerate you should pass 0 fee rate, you cant turn off the restriction of maxburnamount nit: maybe reword to relax?

0# Relax the restrictions and send it; parent gets through as own subpackage

instagibbs commented at 4:20 pm on January 8, 2024:

taken

instagibbs force-pushed on Jan 8, 2024

DrahtBot added the label CI failed on Jan 16, 2024

ajtowns commented at 10:11 am on January 30, 2024: contributor

Isn’t this check a little bit later than ideal? Wouldn’t it make more sense to check both maxfeerate and maxburnamount as part of signrawtransactionwith* and walletprocesspsbt so that you never create a tx that can lose your money, rather than creating it and hoping that it won’t find its way onto the network via some method other than rpc with default args?

glozow commented at 10:42 am on January 30, 2024: member

Isn’t this check a little bit later than ideal? Wouldn’t it make more sense to check both maxfeerate and maxburnamount as part of signrawtransactionwith* and walletprocesspsbt so that you never create a tx that can lose your money

I think it’s indeed good to have that, but isn’t it still possible that transactions submitted via submitpackage were created somewhere else?

instagibbs commented at 1:21 pm on January 30, 2024: member

This PR is essentially focusing on the max fee check during submission to local mempool feature only. Wallet features to reduce accidental overpayment are also welcome, and may already exist in some forms?

in src/validation.cpp:471 in bf6fbea979 outdated

463@@ -464,6 +464,11 @@ class MemPoolAccept
464          * policies such as mempool min fee and min relay fee.
465          */
466         const bool m_package_feerates;
467+        /** Used for local submission of transactions to catch "absurd" fees
468+         * due to fee miscalulation by wallets. std:nullopt implies unset, allowing any feerates.
469+         * Any individual transaction failing this check causes immediate failure.
470+         */
471+        const std::optional<CFeeRate> m_max_sane_feerate;

glozow commented at 4:18 pm on February 2, 2024:

nit: I think calling this m_client_maxfeerate would make more sense. Don’t think “sane” tells us very much, and it’d be good to specify that this comes from the client. Generally I don’t think “sanity check” is the right term for these, just “check,” as the user can provide a strange or no value and we don’t judge whether it makes sense or not.

instagibbs commented at 7:45 pm on February 9, 2024:

done

in src/rpc/mempool.cpp:834 in bf6fbea979 outdated

827@@ -828,6 +828,14 @@ static RPCHelpMan submitpackage()
828                     {"rawtx", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, ""},
829                 },
830             },
831+            {"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK())},
832+             "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT +
833+                 "/kvB.\nSet to 0 to accept any fee rate."},
834+            {"maxburnamount", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(0)},

glozow commented at 4:20 pm on February 2, 2024:

Probably good time to make a DEFAULT_MAX_BURN_AMOUNT{0}

instagibbs commented at 7:45 pm on February 9, 2024:

done

in src/validation.cpp:1317 in bf6fbea979 outdated

1311@@ -1295,6 +1312,15 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
1312             results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state));
1313             return PackageMempoolAcceptResult(package_state, std::move(results));
1314         }
1315+
1316+        // Individual modified feerate exceeded caller-deemed "sane" max; abort
1317+        if (args.m_max_sane_feerate && CFeeRate(ws.m_modified_fees, ws.m_vsize) > args.m_max_sane_feerate.value()) {

glozow commented at 4:26 pm on February 2, 2024:

I can see why we’d check individual feerate instead of chunk feerate, since we don’t currently check whether there’s a real CPFP happening and which transactions are in the chunk containing a CPFP. Could add a TODO item explaining this?

instagibbs commented at 7:45 pm on February 9, 2024:

added note

in test/functional/rpc_packages.py:369 in bf6fbea979 outdated

364+        self.generatetoaddress(node, 1, deterministic_address)
365+
366+        self.log.info("Submitpackage maxfeerate arg testing")
367+        chained_txns = self.wallet.create_self_transfer_chain(chain_length=2)
368+        chain_hex = [t["hex"] for t in chained_txns]
369+        pkg_result = node.submitpackage(chain_hex, 0.00000001)

glozow commented at 4:30 pm on February 2, 2024:

Maybe instead of magic number, calculate/specify the parent feerate and make maxfeerate a little bit less?

instagibbs commented at 7:45 pm on February 9, 2024:

done

in test/functional/rpc_packages.py:383 in bf6fbea979 outdated

378+        # burn test is run before any package evaluation; nothing makes it in and we get broader exception
379+        assert_raises_rpc_error(-25, "Unspendable output exceeds maximum configured by user", node.submitpackage, chain_hex, 0, 0.1)
380+        assert_equal(node.getrawmempool(), [])
381+
382+        # Relax the restrictions for both and send it; parent gets through as own subpackage
383+        pkg_result = node.submitpackage(chain_hex, 50, 50)

glozow commented at 4:32 pm on February 2, 2024:

Similarly, maybe specify maxburnamount as == the output amount? Imo a test should fail if I switch the check from > to >=.

instagibbs commented at 7:45 pm on February 9, 2024:

done

in src/rpc/mempool.cpp:912 in bf6fbea979 outdated

907             }
908             if (!IsChildWithParentsTree(txns)) {
909                 throw JSONRPCTransactionError(TransactionError::INVALID_PACKAGE, "package topology disallowed. not child-with-parents or parents depend on each other.");
910             }
911 
912+

glozow commented at 4:35 pm on February 2, 2024:

random newline?

instagibbs commented at 7:45 pm on February 9, 2024:

removed

in src/rpc/mempool.cpp:835 in bf6fbea979 outdated

827@@ -828,6 +828,14 @@ static RPCHelpMan submitpackage()
828                     {"rawtx", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, ""},
829                 },
830             },
831+            {"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK())},
832+             "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT +
833+                 "/kvB.\nSet to 0 to accept any fee rate."},
834+            {"maxburnamount", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(0)},
835+             "Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value(default of 0), expressed in " + CURRENCY_UNIT + ".\n"

jonatack commented at 5:35 pm on February 2, 2024:

Default value provided twice, can omit second one.

3. maxburnamount (numeric or string, optional, default="0.00") Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value(default of 0), expressed in BTC.

0             "Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value, expressed in " + CURRENCY_UNIT + ".\n"

instagibbs commented at 7:45 pm on February 9, 2024:

done

in src/rpc/mempool.cpp:50 in bf6fbea979 outdated

46@@ -47,7 +47,7 @@ static RPCHelpMan sendrawtransaction()
47              "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT +
48                  "/kvB.\nSet to 0 to accept any fee rate."},
49             {"maxburnamount", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(0)},
50-             "Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value, expressed in " + CURRENCY_UNIT + ".\n"
51+             "Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value(default of 0), expressed in " + CURRENCY_UNIT + ".\n"

jonatack commented at 5:37 pm on February 2, 2024:

Same suggestion here as https://github.com/bitcoin/bitcoin/pull/28950/files#r1476464485.

0             "Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value, expressed in " + CURRENCY_UNIT + ".\n"

instagibbs commented at 7:45 pm on February 9, 2024:

done

instagibbs force-pushed on Feb 9, 2024

DrahtBot removed the label CI failed on Feb 9, 2024

ismaelsadeeq commented at 7:14 am on February 10, 2024: member

Code review ACK 83f34b4783565336341948affa42e3ec71242c4b

DrahtBot requested review from glozow on Feb 10, 2024

DrahtBot added the label Needs rebase on Feb 19, 2024

in src/rpc/mempool.cpp:882 in 83f34b4783 outdated

875@@ -867,6 +876,19 @@ static RPCHelpMan submitpackage()
876                                    "Array must contain between 1 and " + ToString(MAX_PACKAGE_COUNT) + " transactions.");
877             }
878 
879+            // Fee check needs to be run with chainstate and package context
880+            const CFeeRate max_raw_tx_fee_rate = request.params[1].isNull() ?
881+                                                     DEFAULT_MAX_RAW_TX_FEE_RATE :
882+                                                     CFeeRate(AmountFromValue(request.params[1]));

maflcko commented at 7:05 am on February 20, 2024:

0const CFeeRate max_raw_tx_fee_rate{ParseFeeRate(self.Arg<UniValue>(1))};

See #29434#event-11853176044

instagibbs commented at 1:38 pm on February 20, 2024:

edit: I am in fact misreading it

~to make sure I’m not misreading, this is a bump of the default feerate value by 10x? Making sure since I don’t see any tests that had to be changed.~

instagibbs commented at 1:53 pm on February 20, 2024:

taken, thanks

instagibbs force-pushed on Feb 20, 2024

instagibbs commented at 1:54 pm on February 20, 2024: member

rebased

DrahtBot removed the label Needs rebase on Feb 20, 2024

achow101 commented at 3:38 pm on March 12, 2024: member

ACK b6fe346c695d70b0d45d4a5e346aaa591cd2ef48

DrahtBot requested review from ismaelsadeeq on Mar 12, 2024

ismaelsadeeq approved

ismaelsadeeq commented at 7:22 pm on March 12, 2024: member

Re ACK https://github.com/bitcoin/bitcoin/commit/b6fe346c695d70b0d45d4a5e346aaa591cd2ef48 via diff

in src/rpc/mempool.cpp:829 in b6fe346c69 outdated

823@@ -823,6 +824,14 @@ static RPCHelpMan submitpackage()
824                     {"rawtx", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, ""},
825                 },
826             },
827+            {"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK())},
828+             "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT +
829+                 "/kvB.\nFee rates larger than 1BTC/kvB are rejected.\nSet to 0 to accept any fee rate."},

murchandamus commented at 8:31 pm on March 12, 2024:

Are we sure that 100,000 ṩ/vB is not a bit high especially for a default that can be overridden? It seems to me that we would probably want at least one if not two magnitudes lower, especially if we are looking at a complete package and therefore already consider children bumping parents in the appropriate context?

glozow commented at 9:55 am on March 13, 2024:

This PR isn’t changing the current default, so imo it’s best to open a separate issue to discuss it. This isn’t looking at a complete package btw

murchandamus commented at 2:29 pm on March 15, 2024:

Opened in #29661

in src/validation.cpp:476 in b6fe346c69 outdated

471@@ -472,6 +472,11 @@ class MemPoolAccept
472          * policies such as mempool min fee and min relay fee.
473          */
474         const bool m_package_feerates;
475+        /** Used for local submission of transactions to catch "absurd" fees
476+         * due to fee miscalulation by wallets. std:nullopt implies unset, allowing any feerates.

murchandamus commented at 8:59 pm on March 12, 2024:

0         * due to fee miscalculation by wallets. std:nullopt implies unset, allowing any feerates.

instagibbs commented at 1:46 pm on March 13, 2024:

done

in src/validation.cpp:477 in b6fe346c69 outdated

471@@ -472,6 +472,11 @@ class MemPoolAccept
472          * policies such as mempool min fee and min relay fee.
473          */
474         const bool m_package_feerates;
475+        /** Used for local submission of transactions to catch "absurd" fees
476+         * due to fee miscalulation by wallets. std:nullopt implies unset, allowing any feerates.
477+         * Any individual transaction failing this check causes immediate failure.

murchandamus commented at 9:01 pm on March 12, 2024:

Should the maxfeerate perhaps rather apply to a transaction’s mining score in the package? That way, a child transactions bumping a large feerate could have a much larger nominal feerate, while staying in the sane range for the effective feerate.

instagibbs commented at 1:27 pm on March 13, 2024:

I think this will become relatively trivial post-cluster mempool, reconsider it there?

murchandamus commented at 9:02 pm on March 12, 2024: contributor

Concept ACK

in src/validation.cpp:479 in b6fe346c69 outdated

471@@ -472,6 +472,11 @@ class MemPoolAccept
472          * policies such as mempool min fee and min relay fee.
473          */
474         const bool m_package_feerates;
475+        /** Used for local submission of transactions to catch "absurd" fees
476+         * due to fee miscalulation by wallets. std:nullopt implies unset, allowing any feerates.
477+         * Any individual transaction failing this check causes immediate failure.
478+         */
479+        const std::optional<CFeeRate> m_client_maxfeerate;

glozow commented at 9:15 am on March 13, 2024:

In a followup, perhaps we can reorder the ATMPArgs members for better struct-packing.

stickies-v commented at 2:59 pm on March 25, 2024:

I think the current memory layout requires 56 bytes for an ATMPArgs object:

 0
 1    struct ATMPArgs {
 2        const CChainParams& m_chainparams;                      // 8 bytes
 3        const int64_t m_accept_time;                            // 8 bytes
 4        const bool m_bypass_limits;                             // 1 bytes
 5                                                                // 7 bytes padding 
 6        std::vector<COutPoint>& m_coins_to_uncache;             // 8 bytes
 7        const bool m_test_accept;                               // 1 bytes
 8        const bool m_allow_replacement;                         // 1 bytes
 9        const bool m_package_submission;                        // 1 bytes
10        const bool m_package_feerates;                          // 1 bytes
11                                                                // 4 bytes padding
12        const std::optional<CFeeRate> m_client_maxfeerate;      // 9 bytes
13                                                                // 7 bytes padding
14    }; // TOTAL: 7 * 8 = 56 bytes

With optimal (?) member ordering, we could get this down to 48 bytes, I think:

 0    struct ATMPArgs {
 1        const CChainParams& m_chainparams;                      // 8 bytes
 2        const int64_t m_accept_time;                            // 8 bytes
 3        std::vector<COutPoint>& m_coins_to_uncache;             // 8 bytes
 4        const std::optional<CFeeRate> m_client_maxfeerate;      // 9 bytes
 5                                                                // 7 bytes padding
 6        const bool m_bypass_limits;                             // 1 bytes
 7        const bool m_test_accept;                               // 1 bytes
 8        const bool m_allow_replacement;                         // 1 bytes
 9        const bool m_package_submission;                        // 1 bytes
10        const bool m_package_feerates;                          // 1 bytes
11                                                                // 3 bytes padding
12    }; // TOTAL: 6 * 8 bytes = 48 bytes

However, given that afaik we generally only spawn one ATMPArgs at the same time, I don’t think we should change the member ordering just for memory footprint here, and it’s fine to leave as is? Likewise, adding actual struct packing (e.g. with #pragma) almost certainly doesn’t seem worth the performance trade-off (didn’t run any benchmarks).

glozow commented at 8:56 am on March 26, 2024:

Nice, thanks @stickies-v! Agree it’s not really worth it when we only ever have 1 at a time.

glozow assigned glozow on Mar 13, 2024

in test/functional/rpc_packages.py:370 in b6fe346c69 outdated

365+
366+        self.log.info("Submitpackage maxfeerate arg testing")
367+        chained_txns = self.wallet.create_self_transfer_chain(chain_length=2)
368+        minrate = min([chained_txn["fee"] / chained_txn["tx"].get_vsize() for chained_txn in chained_txns])
369+        chain_hex = [t["hex"] for t in chained_txns]
370+        pkg_result = node.submitpackage(chain_hex, maxfeerate=minrate-Decimal("0.00000001"))

glozow commented at 9:58 am on March 13, 2024:

This test isn’t using the arg properly. minrate is in BTC per vB, but the arg is interpreted as BTC per KvB, so it’s 1000x lower than it should be (0.3sat/vB instead of 300sat/vB). Of course the transactions are still rejected, but doesn’t seem like this was the intention.

instagibbs commented at 1:46 pm on March 13, 2024:

good catch, renamed variable to make it clearer, hopefully

in test/functional/rpc_packages.py:384 in b6fe346c69 outdated

379+        # burn test is run before any package evaluation; nothing makes it in and we get broader exception
380+        assert_raises_rpc_error(-25, "Unspendable output exceeds maximum configured by user", node.submitpackage, chain_hex, 0, chained_txns[1]["new_utxo"]["value"] - Decimal("0.00000001"))
381+        assert_equal(node.getrawmempool(), [])
382+
383+        # Relax the restrictions for both and send it; parent gets through as own subpackage
384+        pkg_result = node.submitpackage(chain_hex, maxfeerate=0, maxburnamount=chained_txns[1]["new_utxo"]["value"])

glozow commented at 9:59 am on March 13, 2024:

Imo best to set maxfeerate to the parent feerate (in BTC per KvB) so that changing the > to a >= in validation.cpp causes this test to fail.

instagibbs commented at 1:45 pm on March 13, 2024:

set it to minrate_btc_kvb which does the trick I think

in src/validation.cpp:1336 in b6fe346c69 outdated

1329@@ -1313,6 +1330,16 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
1330             results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state));
1331             return PackageMempoolAcceptResult(package_state, std::move(results));
1332         }
1333+
1334+        // Individual modified feerate exceeded caller-defined max; abort
1335+        // N.B. this doesn't take into account CPFPs. Chunk-aware validation may be more robust.
1336+        if (args.m_client_maxfeerate && CFeeRate(ws.m_modified_fees, ws.m_vsize) > args.m_client_maxfeerate.value()) {

glozow commented at 9:59 am on March 13, 2024:

I flipped to >= and no tests failed (see other comment)

instagibbs commented at 1:46 pm on March 13, 2024:

now fails, thanks

DrahtBot requested review from glozow on Mar 13, 2024

glozow unassigned glozow on Mar 13, 2024

RPC: Add maxfeerate and maxburnamount args to submitpackage

And thread the feerate value through ProcessNewPackage to
reject individual transactions that exceed the given
feerate. This allows subpackage processing, and is
compatible with future package RBF work.

38f70ba6ac

instagibbs force-pushed on Mar 13, 2024

in src/validation.h:285 in 38f70ba6ac

282 * If a transaction fails, validation will exit early and some results may be missing. It is also
283 * possible for the package to be partially submitted.
284 */
285 PackageMempoolAcceptResult ProcessNewPackage(Chainstate& active_chainstate, CTxMemPool& pool,
286-                                                   const Package& txns, bool test_accept)
287+                                                   const Package& txns, bool test_accept, std::optional<CFeeRate> max_sane_feerate)

glozow commented at 10:48 am on March 14, 2024:

this one didn’t get changed to client_max_feerate (nit)

glozow commented at 11:09 am on March 14, 2024:

nit: Seems like you are passing by reference in most places, which makes sense to me

0                                                   const Package& txns, bool test_accept, const std::optional<CFeeRate>& max_sane_feerate)

in src/validation.cpp:514 in 38f70ba6ac

510         }
511 
512         /** Parameters for child-with-unconfirmed-parents package validation. */
513         static ATMPArgs PackageChildWithParents(const CChainParams& chainparams, int64_t accept_time,
514-                                                std::vector<COutPoint>& coins_to_uncache) {
515+                                                std::vector<COutPoint>& coins_to_uncache, std::optional<CFeeRate>& client_maxfeerate) {

glozow commented at 10:51 am on March 14, 2024:

nit: should use const reference

glozow commented at 11:09 am on March 14, 2024: member

ACK 38f70ba6ac with some non-blocking nits

DrahtBot requested review from murchandamus on Mar 14, 2024

DrahtBot requested review from ismaelsadeeq on Mar 14, 2024

DrahtBot requested review from achow101 on Mar 14, 2024

instagibbs commented at 6:47 pm on March 14, 2024: member

@glozow I’ll address these if I touch the commits again

murchandamus commented at 3:03 pm on March 15, 2024: contributor

LGTM, code review ACK 38f70ba6ac86fb96c60571d2e1f316315c1c73cc

ismaelsadeeq commented at 1:49 pm on March 18, 2024: member

Re-ACK https://github.com/bitcoin/bitcoin/commit/38f70ba6ac86fb96c60571d2e1f316315c1c73cc 👍🏾

glozow merged this on Mar 18, 2024

glozow closed this on Mar 18, 2024

instagibbs commented at 12:35 pm on March 25, 2024: member

follow-ups for this here #29722 @glozow

glozow referenced this in commit 19b968f743 on Mar 26, 2024

RPC: Add maxfeerate and maxburnamount args to submitpackage #28950

Code Coverage

Reviews

Conflicts