wallet, rpc: document and update `sendall` behavior around unconfirmed inputs #28979

ishaanam · 2023-11-30T22:58:55Z

This PR:

Adds a functional test that sendall spends unconfirmed change
Adds a functional test that sendall spends regular unconfirmed inputs when specified by user
Adds ancestor aware funding to sendall by using calculateCombinedBumpFee and adjusting the effective value accordingly
Adds a functional test for ancestor aware funding in sendall

DrahtBot · 2023-11-30T22:58:58Z

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
Concept ACK	jonatack, S3RK, BrandonOdiwuor, murchandamus, tdb3, 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:

#28944 (wallet, rpc: add anti-fee-sniping to send and sendall by ishaanam)

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.

jonatack

Concept ACK

test/functional/wallet_sendall.py

jonatack · 2023-11-30T23:19:08Z

The code changes cause this test failure for me locally:

$ ./test/functional/wallet_fundrawtransaction.py --legacy-wallet 
2023-11-30T23:17:02.968000Z TestFramework (INFO): PRNG seed is: 4425912909580482609
.../...
2023-11-30T23:17:20.070000Z TestFramework (INFO): Test fundrawtxn with locked wallet and hardened derivation
2023-11-30T23:17:21.869000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
  File "/Users/jon/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
    self.run_test()
  File "/Users/jon/bitcoin/bitcoin/./test/functional/wallet_fundrawtransaction.py", line 134, in run_test
    self.test_locked_wallet()
  File "/Users/jon/bitcoin/bitcoin/./test/functional/wallet_fundrawtransaction.py", line 603, in test_locked_wallet
    value = inputs[0]["amount"] - get_fee(tx_size, self.min_relay_tx_fee)
            ~~~~~~^^^
IndexError: list index out of range

and

wallet/rpc/spend.cpp:1476:47: error: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy,-warnings-as-errors]
 1476 |                 for (std::shared_ptr<COutput> output : pre_selected_inputs.coins) {
      |                                               ^
      |                      const

S3RK · 2023-12-03T13:32:40Z

Concept ACK

BrandonOdiwuor

Concept ACK

murchandamus

Concept ACK, the ancestor aware funding could use a test

test/functional/wallet_sendall.py

src/wallet/rpc/spend.cpp

murchandamus · 2023-12-29T20:08:02Z

test/functional/wallet_sendall.py

+
+        # higher parent feerate
+        self.def_wallet.sendtoaddress(address=self.wallet.getnewaddress(), amount=17, fee_rate=20)
+        assert_greater_than(self.wallet.getbalances()["mine"]["untrusted_pending"], 0)


The code seems correct to me, but the test seems to have failed on line 408:
https://cirrus-ci.com/task/6362440724643840?logs=ci#L2630

I presume self.def_wallet is not equal to self.wallet, so a mempool sync is missing here, to allow for the transaction to "propagate" through the full validation interface queue.

self.def_wallet is not equal to self.wallet, but they belong to the same node. I have implemented this suggestion, but given that the wallets are in the same node, will it make a difference?

self.def_wallet is not equal to self.wallet, but they belong to the same node. I have implemented this suggestion, but given that the wallets are in the same node, will it make a difference?

Need to call syncwithvalidationinterfacequeue() to flush the validation queue before checking the wallet balance. The transaction could have not been processed by the validation interface worker thread. This is the one dispatching the tx to the second wallet.

murchandamus · 2024-01-03T22:47:38Z

I don’t know why it could be failing here, it doesn’t seem to make sense that it would. Could you try to rebase?

test/functional/wallet_sendall.py

furszy · 2024-01-05T22:38:12Z

test/functional/wallet_sendall.py

+
+        # higher parent feerate
+        self.def_wallet.sendtoaddress(address=self.wallet.getnewaddress(), amount=17, fee_rate=20)
+        assert_greater_than(self.wallet.getbalances()["mine"]["untrusted_pending"], 0)


self.def_wallet is not equal to self.wallet, but they belong to the same node. I have implemented this suggestion, but given that the wallets are in the same node, will it make a difference?

Need to call syncwithvalidationinterfacequeue() to flush the validation queue before checking the wallet balance. The transaction could have not been processed by the validation interface worker thread. This is the one dispatching the tx to the second wallet.

test/functional/wallet_sendall.py

src/wallet/rpc/spend.cpp

test/functional/wallet_sendall.py

…uts when specified

tdb3

Concept ACK. Nice addition (adding more ancestor awareness).
Received a test failure (inline comment provided).

test/functional/wallet_sendall.py

achow101 · 2024-04-24T19:12:12Z

ACK modulo existing comments.

furszy

ACK 71aae72

jonatack reviewed Nov 30, 2023

View reviewed changes

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

DrahtBot added the CI failed label Nov 30, 2023

DrahtBot mentioned this pull request Dec 1, 2023

wallet, rpc: add anti-fee-sniping to send and sendall #28944

Open

BrandonOdiwuor reviewed Dec 8, 2023

View reviewed changes

ishaanam force-pushed the sendall_ancestor_aware_funding branch 2 times, most recently from 3a91852 to 8d28da4 Compare December 10, 2023 04:50

DrahtBot removed the CI failed label Dec 10, 2023

ishaanam marked this pull request as ready for review December 10, 2023 16:44

DrahtBot added CI failed and removed CI failed labels Dec 21, 2023

murchandamus reviewed Dec 27, 2023

View reviewed changes

test/functional/wallet_sendall.py Show resolved Hide resolved

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

ishaanam force-pushed the sendall_ancestor_aware_funding branch from 8d28da4 to 5564345 Compare December 28, 2023 21:51

DrahtBot added the CI failed label Dec 28, 2023

murchandamus reviewed Dec 29, 2023

View reviewed changes

ishaanam force-pushed the sendall_ancestor_aware_funding branch from 5564345 to 551707a Compare January 3, 2024 22:49

DrahtBot removed the CI failed label Jan 3, 2024

ishaanam force-pushed the sendall_ancestor_aware_funding branch from 551707a to db592e1 Compare January 5, 2024 21:00

furszy reviewed Jan 5, 2024

View reviewed changes

DrahtBot added the CI failed label Jan 5, 2024

ishaanam force-pushed the sendall_ancestor_aware_funding branch from db592e1 to e73a56f Compare January 7, 2024 04:10

DrahtBot removed the CI failed label Jan 7, 2024

DrahtBot added the CI failed label Jan 16, 2024

furszy reviewed Feb 7, 2024

View reviewed changes

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

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

test/functional/wallet_sendall.py Show resolved Hide resolved

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

ishaanam added 2 commits February 10, 2024 16:38

rpc, test: test sendall spends unconfirmed change and unconfirmed inp…

544131f

…uts when specified

wallet, rpc: implement ancestor aware funding for sendall

3675794

ishaanam force-pushed the sendall_ancestor_aware_funding branch from e73a56f to a7986ff Compare February 10, 2024 21:55

DrahtBot removed the CI failed label Feb 10, 2024

tdb3 reviewed Feb 28, 2024

View reviewed changes

test/functional/wallet_sendall.py Show resolved Hide resolved

test: test sendall does ancestor aware funding

71aae72

ishaanam force-pushed the sendall_ancestor_aware_funding branch from a7986ff to 71aae72 Compare May 9, 2024 16:50

furszy reviewed May 17, 2024

View reviewed changes

DrahtBot requested review from BrandonOdiwuor, S3RK, achow101, tdb3, jonatack and murchandamus May 17, 2024 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet, rpc: document and update `sendall` behavior around unconfirmed inputs #28979

wallet, rpc: document and update `sendall` behavior around unconfirmed inputs #28979

ishaanam commented Nov 30, 2023 •

edited

DrahtBot commented Nov 30, 2023 •

edited

jonatack left a comment

jonatack commented Nov 30, 2023 •

edited

S3RK commented Dec 3, 2023

BrandonOdiwuor left a comment

murchandamus left a comment

murchandamus Dec 29, 2023

maflcko Jan 4, 2024

ishaanam Jan 5, 2024

furszy Jan 5, 2024 •

edited

murchandamus commented Jan 3, 2024

furszy Jan 5, 2024 •

edited

tdb3 left a comment

achow101 commented Apr 24, 2024

furszy left a comment •

edited

wallet, rpc: document and update sendall behavior around unconfirmed inputs #28979

Are you sure you want to change the base?

wallet, rpc: document and update sendall behavior around unconfirmed inputs #28979

Conversation

ishaanam commented Nov 30, 2023 • edited

DrahtBot commented Nov 30, 2023 • edited

Code Coverage

Reviews

Conflicts

jonatack left a comment

Choose a reason for hiding this comment

jonatack commented Nov 30, 2023 • edited

S3RK commented Dec 3, 2023

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

murchandamus left a comment

Choose a reason for hiding this comment

murchandamus Dec 29, 2023

Choose a reason for hiding this comment

maflcko Jan 4, 2024

Choose a reason for hiding this comment

ishaanam Jan 5, 2024

Choose a reason for hiding this comment

furszy Jan 5, 2024 • edited

Choose a reason for hiding this comment

murchandamus commented Jan 3, 2024

furszy Jan 5, 2024 • edited

Choose a reason for hiding this comment

tdb3 left a comment

Choose a reason for hiding this comment

achow101 commented Apr 24, 2024

furszy left a comment • edited

Choose a reason for hiding this comment

wallet, rpc: document and update `sendall` behavior around unconfirmed inputs #28979

wallet, rpc: document and update `sendall` behavior around unconfirmed inputs #28979

ishaanam commented Nov 30, 2023 •

edited

DrahtBot commented Nov 30, 2023 •

edited

jonatack commented Nov 30, 2023 •

edited

furszy Jan 5, 2024 •

edited

furszy Jan 5, 2024 •

edited

furszy left a comment •

edited