max_tx_weight to transaction funding options (take 2)
#29523
This PR taken over from #29264
The PR added an option max_tx_weight to transaction funding RPC’s that ensures the resulting transaction weight does not exceed the specified max_tx_weight limit.
If max_tx_weight is not given MAX_STANDARD_TX_WEIGHT is used as the max threshold.
This PR addressed outstanding review comments in #29264
For more context and rationale behind this PR see https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418/11?u=instagibbs
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | furszy, rkrux, achow101 |
| Concept ACK | murchandamus |
| Stale ACK | Eunovo |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
700@@ -701,7 +701,7 @@ util::Result<SelectionResult> ChooseSelectionResult(interfaces::Chain& chain, co
701 } else append_error(bnb_result);
702 }
703
704- // As Knapsack and SRD can create change, also deduce change weight.
705+ // Deduce change weight because remaining Coin Selection algorithms can create change output.
In “[doc]: update reason for deducing change output weight” (05373e632500670ff8bd65ed6621a039849886ad):
to deduct means to subtract, to deduce means to draw a logical conclusion
0 // Deduct change weight because remaining Coin Selection algorithms can create change output.
Also in the commit message for that PR:
CoinGrinder will always produce a change output.
0-`CoinGrinder` also might produce change output, listing all the
1+`CoinGrinder` will also produce a change output, listing all the
83@@ -84,19 +84,19 @@ struct {
84 * bound of the range.
85 * @param const CAmount& cost_of_change This is the cost of creating and spending a change output.
86 * This plus selection_target is the upper bound of the range.
87- * @param int max_selection_weight The maximum allowed weight for a selection result to be valid.
88+ * @param int64_t max_selection_weight The maximum allowed weight for a selection result to be valid.
In “[wallet]: change max_selection_weight to int64_t” (eaf40cea9d31b43b4eb112e561fbadc1911e5732):
This commit surprises me. I guess being explicit is better than being implicit, so I would understand using int32_t instead of int, but I don’t understand the motivation to switch to an int64_t. int in Bitcoin Core is always a signed 32-bit integer, because Bitcoin Core only supports platforms where int is exactly 32 bits. Therefore, the maximum value for int is 231, i.e. a bit more than 2 billion. Since the blockweight is limited to 4 million bytes, it’s unclear to me why we would need more than 32 bits for max_selection_weight. What am I missing here?
You are right int is enough, I switched back to the status quo.
Thanks.
173@@ -174,10 +174,15 @@ struct CoinSelectionParams {
174 * 1) Received from other wallets, 2) replacing other txs, 3) that have been replaced.
175 */
176 bool m_include_unsafe_inputs = false;
177+ /**
178+ * The maximally sized transaction we are willing to make.
179+ */
In “[wallet, rpc]: add max_tx_weight to tx funding options” (854acd72e99dc9f0924c5f0b5b7a657b39538911):
Weight, not size:
0 /** The maximum weight for this transaction */
178+ * The maximally sized transaction we are willing to make.
179+ */
180+ std::optional<int64_t> m_max_tx_weight{std::nullopt};
181
182- CoinSelectionParams(FastRandomContext& rng_fast, size_t change_output_size, size_t change_spend_size,
183+ CoinSelectionParams(FastRandomContext& rng_fast, int change_output_size, size_t change_spend_size,
In “[wallet, rpc]: add max_tx_weight to tx funding options” (854acd72e99dc9f0924c5f0b5b7a657b39538911):
I’m a bit surprised by this sudden type change in a commit whose commit message doesn’t mention it. While I agree in principal that size_t is an inappropriate type for variables storing vsize or weight, could you please explain your motivation to change the types, and especially in this context? If you are retyping unrelated variables, I think that should perhaps be a separate commit. If you are changing the types of change_output_size and tx_noinputs_size, why not also change_spend_size?
Perhaps if we are getting big into the business of changing the types of various transaction size, vsize and weight variables, we should perhaps consider introducing explicit types for weight, size, and vsize to prevent people accidentally mistaking them for the same type. On the other hand, that feels like the sort of change that will never propagate to the entire codebase, so I’m not sure it’s a good idea to even start.
Thanks for your review @murchandamus
Perhaps if we are getting big into the business of changing the types of various transaction size, vsize and weight variables, we should perhaps consider introducing explicit types for weight, size, and vsize to prevent people accidentally mistaking them for the same type.
I agree with this suggestion and have same concern previously here #29242 (review) should make the code cleaner
Sorry for not explaining the rationale behind this in the commit messages, I will try and explain why I did the change below
On master we have
0 int max_inputs_weight = MAX_STANDARD_TX_WEIGHT - (coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR);
1
2
3 .......
4 max_inputs_weight -= (coin_selection_params.change_output_size * WITNESS_SCALE_FACTOR);
After this PR maximum transaction weight can be configured by users, so we now subtract from user provided maximum weight or use the default if not given.
0int max_selection_weight = coin_selection_params.m_max_tx_weight.value_or(MAX_STANDARD_TX
1_WEIGHT) - coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR;
m_max_tx_weight is int, tx_noinputs_size and change_output_size are both size_t.
given that Bitcoin Core only supports platforms where int is exactly 32 bits
For signed integers (int), it ranges from -2^(31) to (2^(31)) - 1.
For unsigned integers (unsigned int), it ranges from 0 to (2^(32)) - 1.
The issue arises when after compiling with “ASan + LSan + UBSan + integer, without dependencies, USDT” and running the functional test.
From 854acd72e9 we allow passing 0 weight and test for that in the functional test, results of subtracting coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR from 0 causes max_selection_weight to have some positive value instead of negative. hence the test fails see https://cirrus-ci.com/task/6341256662482944, This is reproducible locally.
Thats why I changed change_output_size, and tx_noinputs_sizefrom size_t to int64_t and also use same datatype for m_max_tx_weight which works fine.
0int64_t max_selection_weight = coin_selection_params.m_max_tx_weight.value_or(MAX_STANDARD_TX
1_WEIGHT) - static_cast<int64_t>(coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR);
But since Bitcoin Core only supports platforms where int is exactly 32 bits I think using int for change_output_size, and tx_noinputs_size should work fine?
From https://github.com/bitcoin/bitcoin/commit/854acd72e99dc9f0924c5f0b5b7a657b39538911 we allow passing 0 weight and test for that in the functional test.
After thinking about this more, it does not make sense to allow passing passing 0 maximum transaction weight, should instead be between minimum transaction weight possible and MAX_STANDARD_TX_WEIGHT.
I will update to this.
max_tx_weight below MIN_STANDARD_TX_NONWITNESS_SIZE * WITNESS_SCALE_FACTOR
But since Bitcoin Core only supports platforms where int is exactly 32 bits I think using int for
change_output_size, andtx_noinputs_sizeshould work fine?
[…]
I switched back to status quo, and prevent users from passing
max_tx_weightbelowMIN_STANDARD_TX_NONWITNESS_SIZE * WITNESS_SCALE_FACTOR
I think we may have crossed some wires here. Thanks for the explanation, that was very helpful. I still agree that size_t is the wrong type for change_output_size and tx_noinputs_size. I would support you taking the opportunity because m_max_tx_weight is int to also change those two to int, perhaps even take change_spend_size along for that. I just think that it might be better to make it a separate commit and that it should be explained in the commit message.
int :)
1373@@ -1374,6 +1374,7 @@ RPCHelpMan sendall()
1374 RPCResult::Type::OBJ, "", "",
1375 {
1376 {RPCResult::Type::BOOL, "complete", "If the transaction has a complete set of signatures"},
1377+ {RPCResult::Type::NUM, "weight", "The resulting transaction weight in bytes"},
In “[rpc]: update sendall RPC to return the resulting tx weight” (8e256fe679236710549820ec48ac7c0793fcfd96):
“bytes” is not a unit of weight. I would expect that this is returning the weight in weight units or the vsize in vbytes.
1049@@ -1050,7 +1050,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
1050 if (change_spend_size == -1) {
1051 coin_selection_params.change_spend_size = DUMMY_NESTED_P2WPKH_INPUT_SIZE;
1052 } else {
1053- coin_selection_params.change_spend_size = (size_t)change_spend_size;
1054+ coin_selection_params.change_spend_size = (int)change_spend_size;
change_spend_size is already an int.
187@@ -187,6 +188,16 @@ def run_test(self):
188 # Create and fund a raw tx for sending 10 BTC
189 psbtx1 = self.nodes[0].walletcreatefundedpsbt([], {self.nodes[2].getnewaddress():10})['psbt']
190
191+ self.log.info("Test that max_tx_weight option is sanity checked and fails when expected")
192+ dest_arg = [{self.nodes[0].getnewaddress(): 1}]
193+ min_tx_weight = MIN_STANDARD_TX_NONWITNESS_SIZE * WITNESS_SCALE_FACTOR
194+ assert_raises_rpc_error(-8, f"Invalid parameter, max tx weight must be between {min_tx_weight} and 400000", self.nodes[0].walletcreatefundedpsbt, [], dest_arg, 0, {"max_tx_weight": -1})
MAX_STANDARD_TX_WEIGHT but I think it’s better to define the MAX_STANDARD_TX_WEIGHT constant and use it here. This makes it obvious to anyone reading what “400000” is.
test_framework/blocktools.py
795@@ -787,6 +796,8 @@ RPCHelpMan fundrawtransaction()
796 },
797 },
798 },
799+ {"max_tx_weight", RPCArg::Type::NUM, RPCArg::Default{MAX_STANDARD_TX_WEIGHT}, "The maximum acceptable transaction weight.\n"
Force pushed from https://github.com/bitcoin/bitcoin/commit/53ee67de7e388703cb672e67ffaecdde53eb01de to https://github.com/bitcoin/bitcoin/pull/29523/commits/3a64b3a339fe55c2642369b00a382231871c2838 compare diff
Any chance to turn the first commit into an scripted-diff?
The commit message was misleading It’s not just renaming max_weight, it also updates the input parameter docstring to match the new name, some comment updates, and also the fuzz test renames has more info like low_max_selection_weight and high_max_selection_weight.
I’ve updated the commit message of the first commit https://github.com/bitcoin/bitcoin/pull/29523/commits/270639324a6637e0025e1458221ab488a8a09554
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
691@@ -692,7 +692,9 @@ util::Result<SelectionResult> ChooseSelectionResult(interfaces::Chain& chain, co
692 };
693
694 // Maximum allowed weight for selected coins.
695- int max_selection_weight = MAX_STANDARD_TX_WEIGHT - (coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR);
696+ int max_transaction_weight = coin_selection_params.m_max_tx_weight.value_or(MAX_STANDARD_TX_WEIGHT);
697+ int tx_weight_no_input = coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR;
698+ int max_selection_weight = std::max((max_transaction_weight - tx_weight_no_input), 0);
In d9ec6e7f28f0:
Instead of setting it to 0, could error out early if max_selection_weight <= 0 (a test for this would be nice).
703@@ -702,7 +704,8 @@ util::Result<SelectionResult> ChooseSelectionResult(interfaces::Chain& chain, co
704 }
705
706 // Deduct change weight because remaining Coin Selection algorithms can create change output
707- max_selection_weight -= (coin_selection_params.change_output_size * WITNESS_SCALE_FACTOR);
708+ int change_outputs_weight = coin_selection_params.change_output_size * WITNESS_SCALE_FACTOR;
709+ max_selection_weight = std::max((max_selection_weight - change_outputs_weight), 0);
max_selection_weight <= 0. No need to run the selection algorithms under such circumstance.
47@@ -48,6 +48,7 @@
48
49 MAX_BLOCK_SIGOPS = 20000
50 MAX_BLOCK_SIGOPS_WEIGHT = MAX_BLOCK_SIGOPS * WITNESS_SCALE_FACTOR
51+MAX_STANDARD_TX_WEIGHT = 400000
83@@ -84,14 +84,14 @@ struct {
84 * bound of the range.
85 * @param const CAmount& cost_of_change This is the cost of creating and spending a change output.
86 * This plus selection_target is the upper bound of the range.
87- * @param int max_weight The maximum weight available for the input set.
88+ * @param int max_selection_weight The maximum allowed weight for a selection result to be valid.
As per the following 2 statements in ChooseSelectionResult:
The no_input and change_output weights are accounted for, but where is the weight of the destination output accounted for?
coin_selection_params.tx_noinputs_size here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/spend.cpp#L1100
138@@ -139,9 +139,9 @@ struct CoinSelectionParams {
139 /** Randomness to use in the context of coin selection. */
140 FastRandomContext& rng_fast;
141 /** Size of a change output in bytes, determined by the output type. */
142- size_t change_output_size = 0;
143+ int change_output_size = 0;
This change ensures consistency in transaction size and weight calculation within the wallet and prevents conversion overflow when calculating
max_selection_weight.
Are the inconsistencies because size_t is platform dependent and int is not (at least for the purposes of core because of this: https://github.com/bitcoin/bitcoin/blob/v26.0/src/compat/assumptions.h#L44)? I still need to digest this comment: #29523 (review)
It’s simply just to ensure consistency in transaction size and weight calculation within the wallet and prevents conversion overflow when calculating max_selection_weight as it happened here https://cirrus-ci.com/task/6341256662482944
I think it definitely will be worth it to define a datatype for size in the entire codebase.
But I am limiting this change to affect only the wallet size variables change_output_size, change_spend_size and tx_noinputs_size, because they relate to this PR.
229+ assert_raises_rpc_error(-4, "The inputs size exceeds the maximum weight", self.nodes[0].walletcreatefundedpsbt, [], dest_arg_large, 0, {"max_tx_weight": (large_tx_vsize_with_change + 1) * WITNESS_SCALE_FACTOR})
230+
231+ max_tx_weight_sufficient = 1000 # 1k vbytes is enough
232+ psbt = self.nodes[0].walletcreatefundedpsbt(outputs=dest_arg,locktime=0, options={"max_tx_weight": max_tx_weight_sufficient})["psbt"]
233+ weight = self.nodes[0].decodepsbt(psbt)["tx"]["weight"]
234+ # ensure the transactions weight is below the transaction weight.
Nit: In case the file is changed.
0# ensure the transaction's weight is below the specified max_tx_weight.
206+ output_count = 1
207+ tx_weight_without_inputs = (base_tx_vsize + output_count + p2wpkh_output_vsize) * WITNESS_SCALE_FACTOR
208+ # min_tx_weight is greater than transaction weight without inputs
209+ assert_greater_than(min_tx_weight, tx_weight_without_inputs)
210+
211+ # In other to test for when the passed max weight is less than the transaction weight without inputs
715@@ -706,7 +716,11 @@ util::Result<SelectionResult> ChooseSelectionResult(interfaces::Chain& chain, co
716 }
717
718 // Deduct change weight because remaining Coin Selection algorithms can create change output
719- max_selection_weight -= (coin_selection_params.change_output_size * WITNESS_SCALE_FACTOR);
720+ int change_outputs_weight = coin_selection_params.change_output_size * WITNESS_SCALE_FACTOR;
721+ max_selection_weight = max_selection_weight - change_outputs_weight;
tiny nit:
0 max_selection_weight -= change_outputs_weight;
1105@@ -1091,13 +1106,16 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
1106 // Static vsize overhead + outputs vsize. 4 version, 4 nLocktime, 1 input count, 1 witness overhead (dummy, flag, stack size)
1107 coin_selection_params.tx_noinputs_size = 10 + GetSizeOfCompactSize(vecSend.size()); // bytes for output count
1108
1109+ LogInfo("input count size %s \n", GetSizeOfCompactSize(vecSend.size()));
1113 {
1114 CTxOut txout(recipient.nAmount, GetScriptForDestination(recipient.dest));
1115
1116 // Include the fee cost for outputs.
1117 coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout);
1118+ LogInfo("Output size %s \n", static_cast<int>(::GetSerializeSize(txout)));
220+
221+ # Test for max_tx_weight just enough to include inputs but not change output
222+ assert_raises_rpc_error(-4, "Maximum transaction weight is too low, can not accommodate change output", self.nodes[0].walletcreatefundedpsbt, [], dest_arg_large, 0, {"max_tx_weight": (large_tx_vsize_without_inputs + 1) * WITNESS_SCALE_FACTOR})
223+ # Test that it has to accommodate a change output
224+ large_tx_vsize_with_change = large_tx_vsize_without_inputs + p2wpkh_output_vsize
225+ assert_raises_rpc_error(-4, "Maximum transaction weight is too low, can not accommodate change output", self.nodes[0].walletcreatefundedpsbt, [], dest_arg_large, 0, {"max_tx_weight": (large_tx_vsize_with_change) * WITNESS_SCALE_FACTOR})
1032@@ -985,6 +1033,5 @@ def test_psbt_input_keys(psbt_input, keys):
1033 self.log.info("Test descriptorprocesspsbt raises if an invalid sighashtype is passed")
1034 assert_raises_rpc_error(-8, "'all' is not a valid sighash parameter.", self.nodes[2].descriptorprocesspsbt, psbt, [descriptor], sighashtype="all")
1035
1036-
PEP 8: E305 expected 2 blank lines after class or function definition, found 1. Let’s not remove this line.
1564@@ -1564,8 +1565,9 @@ RPCHelpMan sendall()
1565 pwallet->LockCoin(txin.prevout);
1566 }
1567 }
1568-
1569- return FinishTransaction(pwallet, options, rawTx);
1570+ auto result = FinishTransaction(pwallet, options, rawTx);
1571+ result.pushKV("weight", tx_size.weight);
Not sure about using tx_size, we would be returning an estimation of the final size of tx. Not the real/current tx weight.
What about getting the current tx weight by serializing the CMutableTransaction object that appears inside FinishTransaction and explain in the RPC return value description that the returned weight will not be the final one if the transaction is not complete.
- This commit renames the coin selection algorithms input parameter `max_weight`
to `max_selection_weight` for clarity.
The parameter represent the maximum weight of the UTXOs the coin selection algorithm
should select, not the transaction maximum weight.
- The commit updates the parameter docstring to provide correct description.
- Also updates coin selection unit and fuzzing test variables to match the new name.
`CoinGrinder` will also produce change output, listing all the
Coin selection algorithms that produces change output is not maintainable,
just infer that remaining algorithms all might produce change.
- This change ensures consistency in transaction size and weight calculation
within the wallet and prevents conversion overflow when calculating
`max_selection_weight`.
This allows a transaction's weight to be bound under a certain
weight if possible and desired. This can be beneficial for future
RBF attempts, or whenever a more restricted spend topology is
desired.
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
Thanks for the reviews @furszy @rkrux
Force-pushed from b3ac1179ff90fe261af4e6dc9c7af64e1518b435 to d8febf6d9ea018cf8e980ee036fb5ccd8ea8887a Compare diff b3ac1179ff..734076c6d
Changes:
MAX_STANDARD_TX_WEIGHT.rpc_psbt.py functional test according PEP guidelines.send RPC based on the review here.1010@@ -1002,7 +1011,11 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
1011 CoinSelectionParams coin_selection_params{rng_fast}; // Parameters for coin selection, init with dummy
1012 coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends;
1013 coin_selection_params.m_include_unsafe_inputs = coin_control.m_include_unsafe_inputs;
1014-
1015+ coin_selection_params.m_max_tx_weight = coin_control.m_max_tx_weight.value_or(MAX_STANDARD_TX_WEIGHT);
1016+ int minimum_tx_weight = MIN_STANDARD_TX_NONWITNESS_SIZE * WITNESS_SCALE_FACTOR;
1017+ if (coin_selection_params.m_max_tx_weight.value() < minimum_tx_weight || coin_selection_params.m_max_tx_weight.value() > MAX_STANDARD_TX_WEIGHT) {
In 734076c6de178:
nano nit: shorter
0 if (*coin_selection_params.m_max_tx_weight < minimum_tx_weight || *coin_selection_params.m_max_tx_weight > MAX_STANDARD_TX_WEIGHT) {
reACK 734076c
Ran make and functional tests, all successful. @ismaelsadeeq This diff comparison b3ac117..734076c contains a lot of unrelated changes though.
@ismaelsadeeq This diff comparison b3ac117..734076c contains a lot of unrelated changes though.
Yes that’s because of the recent rebase.
