Wallet: Add `max_tx_weight` to transaction funding options (take 2) #29523

ismaelsadeeq · 2024-03-01T09:38:37Z

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

DrahtBot · 2024-03-01T09:38:40Z

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	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.

Conflicts

No conflicts as of last run.

ismaelsadeeq · 2024-03-01T10:27:37Z

Some C.I jobs are failing, putting in draft while I try to address that.

murchandamus

Concept ACK

src/wallet/spend.cpp

src/wallet/coinselection.cpp

src/wallet/coinselection.h

src/wallet/rpc/spend.cpp

src/wallet/spend.cpp

test/functional/rpc_psbt.py

src/wallet/rpc/spend.cpp

ismaelsadeeq · 2024-04-04T10:54:33Z

Force pushed from 53ee67d to 3a64b3a compare diff

Addressed review comment
Rebased on master after ci: Temporarily disable bpfcc-tools #29788

furszy

Any chance to turn the first commit into an scripted-diff?

Eunovo · 2024-04-04T15:10:15Z

Tested ACK 3a64b3a
Verified fuzz tests

ismaelsadeeq · 2024-04-05T16:02:27Z

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 2706393

DrahtBot · 2024-04-24T23:55:58Z

🚧 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.

_{Debug: https://github.com/bitcoin/bitcoin/runs/23495200918}

ismaelsadeeq · 2024-06-10T10:32:44Z

Rebased to fix conflict after #28366 was merged.
b24b7a9a6..845d16c1b

furszy

few small comments

src/wallet/spend.cpp

furszy · 2024-06-19T22:20:42Z

test/functional/test_framework/blocktools.py

@@ -48,6 +48,7 @@

 MAX_BLOCK_SIGOPS = 20000
 MAX_BLOCK_SIGOPS_WEIGHT = MAX_BLOCK_SIGOPS * WITNESS_SCALE_FACTOR
+MAX_STANDARD_TX_WEIGHT = 400000


nit: could replace this line with this constant.

leaving this as is, as I suspect there might be other places that needs to be replaced also.
this and others can come in a separate PR ?

ismaelsadeeq · 2024-06-20T08:12:00Z

thanks for your review @furszy will force push to address this comments shortly.

rkrux

Concept ACK a120fc7, but I would look at 723a8da commit a bit more again; make, test/functional are successful though.

Following transaction funding RPCs are affected with the addition of max_tx_weight option:

fundrawtransaction
walletcreatefundedpsbt
send

src/wallet/coinselection.cpp

rkrux · 2024-06-24T11:26:08Z

src/wallet/coinselection.h

-    size_t change_output_size = 0;
+    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 (comment)

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.

test/functional/rpc_psbt.py

…x weight 72b2268 wallet: notify when preset + automatic inputs exceed max weight (furszy) Pull request description: Small change. Found it while finishing my review on #29523. This does not interfere with it. Basically, we are erroring out early when the automatic coin selection process exceeds the maximum weight, but we are not doing so when the user-preselected inputs combined with the wallet-selected inputs exceed the maximum weight. This change avoids signing all inputs before erroring out and introduces test coverage for `fundrawtransaction`. ACKs for top commit: achow101: ACK 72b2268 tdb3: re ACK for 72b2268 rkrux: tACK [72b2268](72b2268) ismaelsadeeq: utACK 72b2268 Tree-SHA512: d77be19231023383a9c79a5d66b642dcbc6ebfc31a363e0b9f063c44898720a7859ec211cdbc0914ac7a3bfdf15e52fb8fc20d97f171431f70492c0f159dbc36

furszy

Code reviewed b3ac117, getting closer. Left few topics.

src/wallet/spend.cpp

test/functional/rpc_psbt.py

furszy · 2024-06-26T20:51:44Z

src/wallet/rpc/spend.cpp

-
-            return FinishTransaction(pwallet, options, rawTx);
+            auto result = FinishTransaction(pwallet, options, rawTx);
+            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.

Interesting, I agree with this point. Returning the exact size of the transaction would make the RPC definition more robust.

This commit (along with this^ suggestion) b3ac117 can be a separate PR as well, doesn't seem necessary for this PR to be merged if I didn't miss anything.

Yes it's not needed.
I removed this commit
This can come as a separate PR, the size output can also be added to the other funding RPC's?

…ght` name - 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.

…_size` and `tx_noinputs_size` to `int` - 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>

ismaelsadeeq · 2024-06-27T14:39:32Z

Thanks for the reviews @furszy @rkrux

Force-pushed from b3ac117 to d8febf6
Compare diff b3ac1179ff..734076c6d

Changes:

Rebased after PR 30309 and updated the maximum transaction weight check to use the user passed value when given or default MAX_STANDARD_TX_WEIGHT.
Removed miscellaneous logs.
Added back the removed line in the rpc_psbt.py functional test according PEP guidelines.
Removed the last commit that returned the estimated size in the send RPC based on the review here.

furszy

utACK 734076c

furszy · 2024-06-27T16:20:57Z

src/wallet/spend.cpp

-
+    coin_selection_params.m_max_tx_weight = coin_control.m_max_tx_weight.value_or(MAX_STANDARD_TX_WEIGHT);
+    int minimum_tx_weight = MIN_STANDARD_TX_NONWITNESS_SIZE * WITNESS_SCALE_FACTOR;
+    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 734076c:

nano nit: shorter

Suggested change

if (coin_selection_params.m_max_tx_weight.value() < minimum_tx_weight || coin_selection_params.m_max_tx_weight.value() > MAX_STANDARD_TX_WEIGHT) {

if (*coin_selection_params.m_max_tx_weight < minimum_tx_weight || *coin_selection_params.m_max_tx_weight > MAX_STANDARD_TX_WEIGHT) {

rkrux

reACK 734076c

Ran make and functional tests, all successful.

@ismaelsadeeq This diff comparison b3ac117..734076c contains a lot of unrelated changes though.

ismaelsadeeq · 2024-07-07T11:28:45Z

@ismaelsadeeq This diff comparison b3ac117..734076c contains a lot of unrelated changes though.

Yes that's because of the recent rebase.

achow101 · 2024-07-17T22:19:21Z

ACK 734076c

DrahtBot added the Wallet label Mar 1, 2024

ismaelsadeeq mentioned this pull request Mar 1, 2024

Add max_tx_weight to transaction funding options #29264

Closed

ismaelsadeeq marked this pull request as draft March 1, 2024 10:26

DrahtBot added the CI failed label Mar 1, 2024

This was referenced Mar 1, 2024

wallet: Target a pre-defined utxo set composition by adjusting change outputs #29442

Closed

Fix waste calculation in SelectionResult #28366

Merged

ismaelsadeeq force-pushed the 02-2024-add-max-weight-to-tx-funding branch from babbc5c to 8e256fe Compare March 8, 2024 21:30

DrahtBot removed the CI failed label Mar 8, 2024

ismaelsadeeq marked this pull request as ready for review March 9, 2024 06:41

glozow requested review from murchandamus and furszy March 18, 2024 09:07

murchandamus reviewed Mar 18, 2024

View reviewed changes

ismaelsadeeq force-pushed the 02-2024-add-max-weight-to-tx-funding branch 2 times, most recently from cddf4e9 to 52e4812 Compare March 21, 2024 16:46

furszy reviewed Mar 22, 2024

View reviewed changes

src/wallet/spend.cpp Outdated Show resolved Hide resolved

ismaelsadeeq force-pushed the 02-2024-add-max-weight-to-tx-funding branch from 52e4812 to 53ee67d Compare March 22, 2024 12:01

Eunovo reviewed Mar 28, 2024

View reviewed changes

test/functional/rpc_psbt.py Outdated Show resolved Hide resolved

Eunovo reviewed Mar 28, 2024

View reviewed changes

src/wallet/rpc/spend.cpp Show resolved Hide resolved

ismaelsadeeq force-pushed the 02-2024-add-max-weight-to-tx-funding branch from 53ee67d to 3a64b3a Compare April 4, 2024 10:51

furszy reviewed Apr 4, 2024

View reviewed changes

DrahtBot requested a review from murchandamus April 4, 2024 15:10

ismaelsadeeq force-pushed the 02-2024-add-max-weight-to-tx-funding branch from 3a64b3a to 3d76c37 Compare April 5, 2024 15:59

DrahtBot added the CI failed label Apr 24, 2024

ismaelsadeeq force-pushed the 02-2024-add-max-weight-to-tx-funding branch from 3d76c37 to e7500ec Compare May 3, 2024 08:18

DrahtBot added the Needs rebase label Jun 5, 2024

ismaelsadeeq force-pushed the 02-2024-add-max-weight-to-tx-funding branch from b24b7a9 to 845d16c Compare June 10, 2024 10:30

DrahtBot removed the Needs rebase label Jun 10, 2024

DrahtBot mentioned this pull request Jun 12, 2024

Multiprocess bitcoin #10102

Draft

furszy mentioned this pull request Jun 19, 2024

wallet: notify when preset + automatic inputs exceed max weight #30309

Merged

furszy reviewed Jun 19, 2024

View reviewed changes

ismaelsadeeq force-pushed the 02-2024-add-max-weight-to-tx-funding branch 2 times, most recently from 1ae619f to a120fc7 Compare June 21, 2024 16:22

DrahtBot mentioned this pull request Jun 22, 2024

refactor, wallet: get serialized size of CRecipients directly #30050

Merged

rkrux approved these changes Jun 24, 2024

View reviewed changes

ismaelsadeeq force-pushed the 02-2024-add-max-weight-to-tx-funding branch from a120fc7 to b3ac117 Compare June 25, 2024 10:13

furszy reviewed Jun 26, 2024

View reviewed changes

ismaelsadeeq added 3 commits June 27, 2024 12:37

[doc]: update reason for deducting change output weight

baab0d2

`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.

[wallet]: update the data type of change_output_size, `change_spend…

b6fc504

…_size` and `tx_noinputs_size` to `int` - This change ensures consistency in transaction size and weight calculation within the wallet and prevents conversion overflow when calculating `max_selection_weight`.

ismaelsadeeq force-pushed the 02-2024-add-max-weight-to-tx-funding branch from b3ac117 to d8febf6 Compare June 27, 2024 14:20

ismaelsadeeq force-pushed the 02-2024-add-max-weight-to-tx-funding branch from d8febf6 to 734076c Compare June 27, 2024 14:31

furszy reviewed Jun 27, 2024

View reviewed changes

DrahtBot requested review from Eunovo and rkrux June 27, 2024 16:22

rkrux approved these changes Jul 7, 2024

View reviewed changes

achow101 merged commit efbf4e7 into bitcoin:master Jul 17, 2024
16 checks passed

ismaelsadeeq deleted the 02-2024-add-max-weight-to-tx-funding branch July 17, 2024 23:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wallet: Add `max_tx_weight` to transaction funding options (take 2) #29523

Wallet: Add `max_tx_weight` to transaction funding options (take 2) #29523

ismaelsadeeq commented Mar 1, 2024

DrahtBot commented Mar 1, 2024 •

edited

Loading

ismaelsadeeq commented Mar 1, 2024

murchandamus left a comment

ismaelsadeeq commented Apr 4, 2024

furszy left a comment

Eunovo commented Apr 4, 2024 •

edited

Loading

ismaelsadeeq commented Apr 5, 2024 •

edited

Loading

DrahtBot commented Apr 24, 2024

ismaelsadeeq commented Jun 10, 2024

furszy left a comment

furszy Jun 19, 2024

ismaelsadeeq Jun 21, 2024

ismaelsadeeq commented Jun 20, 2024

rkrux left a comment •

edited

Loading

rkrux Jun 24, 2024

ismaelsadeeq Jun 25, 2024 •

edited

Loading

furszy left a comment

furszy Jun 26, 2024

rkrux Jun 27, 2024

rkrux Jun 27, 2024

ismaelsadeeq Jun 27, 2024

ismaelsadeeq commented Jun 27, 2024

furszy left a comment

furszy Jun 27, 2024

rkrux left a comment

ismaelsadeeq commented Jul 7, 2024

achow101 commented Jul 17, 2024

	if (coin_selection_params.m_max_tx_weight.value() < minimum_tx_weight \|\| coin_selection_params.m_max_tx_weight.value() > MAX_STANDARD_TX_WEIGHT) {
	if (coin_selection_params.m_max_tx_weight < minimum_tx_weight \|\| coin_selection_params.m_max_tx_weight > MAX_STANDARD_TX_WEIGHT) {

Wallet: Add max_tx_weight to transaction funding options (take 2) #29523

Wallet: Add max_tx_weight to transaction funding options (take 2) #29523

Conversation

ismaelsadeeq commented Mar 1, 2024

DrahtBot commented Mar 1, 2024 • edited Loading

Code Coverage

Reviews

Conflicts

ismaelsadeeq commented Mar 1, 2024

murchandamus left a comment

Choose a reason for hiding this comment

ismaelsadeeq commented Apr 4, 2024

furszy left a comment

Choose a reason for hiding this comment

Eunovo commented Apr 4, 2024 • edited Loading

ismaelsadeeq commented Apr 5, 2024 • edited Loading

DrahtBot commented Apr 24, 2024

ismaelsadeeq commented Jun 10, 2024

furszy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismaelsadeeq commented Jun 20, 2024

rkrux left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismaelsadeeq Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismaelsadeeq commented Jun 27, 2024

furszy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkrux left a comment

Choose a reason for hiding this comment

ismaelsadeeq commented Jul 7, 2024

achow101 commented Jul 17, 2024

Wallet: Add `max_tx_weight` to transaction funding options (take 2) #29523

Wallet: Add `max_tx_weight` to transaction funding options (take 2) #29523

DrahtBot commented Mar 1, 2024 •

edited

Loading

Eunovo commented Apr 4, 2024 •

edited

Loading

ismaelsadeeq commented Apr 5, 2024 •

edited

Loading

rkrux left a comment •

edited

Loading

ismaelsadeeq Jun 25, 2024 •

edited

Loading