This PR:
sendall spends unconfirmed changesendall spends regular unconfirmed inputs when specified by usersendall by using calculateCombinedBumpFee and adjusting the effective value accordinglysendall
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | furszy, S3RK, achow101 |
| Concept ACK | jonatack, BrandonOdiwuor, murchandamus, tdb3 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
390 | + def sendall_spends_unconfirmed_inputs_if_specified(self): 391 | + self.log.info("Test that sendall spends specified unconfirmed inputs") 392 | + self.def_wallet.sendtoaddress(self.wallet.getnewaddress(), 17) 393 | + assert_greater_than(self.wallet.getbalances()["mine"]["untrusted_pending"], 0) 394 | + unspent = self.wallet.listunspent(minconf=0)[0] 395 | +
Suggest running ./test/lint/lint-whitespace.py locally on your changes, the lint CI needs appeasing.
Done
Concept ACK
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
Concept ACK
Concept ACK
383 | + def sendall_spends_unconfirmed_change(self): 384 | + self.log.info("Test that sendall spends unconfirmed change") 385 | + self.add_utxos([17]) 386 | + self.wallet.sendtoaddress(self.remainder_target, 10) 387 | + assert_greater_than(self.wallet.getbalances()["mine"]["trusted"], 6) 388 | + self.test_sendall_success(sendall_args = [self.remainder_target])
In "rpc, test: test sendall spends unconfirmed change and unconfirmed inputs when specified" 69408120a64316624b9ec379771dbac142dc6646, perhaps you could confirm here that the wallet is now empty:
assert_equal(self.wallet.getbalance(), 0)
Done
1485 | - const CAmount effective_value{total_input_value - fee_from_size}; 1486 | + CAmount effective_value{total_input_value - fee_from_size}; 1487 | + 1488 | + const std::optional<CAmount> total_bump_fees{pwallet->chain().calculateCombinedBumpFee(outpoints_spent, fee_rate)}; 1489 | + if (total_bump_fees) { 1490 | + effective_value -= *total_bump_fees;
This could be tested by first spending an input whose parent transaction has a higher feerate than what we aim for, then an input of the same type with the same amount whose parent transaction has a lower feerate and showing that the amount sent to the receiver is amended correspondingly.
I've added a test that does this.
Concept ACK, the ancestor aware funding could use a test
403 | + def sendall_does_ancestor_aware_funding(self): 404 | + self.log.info("Test that sendall does ancestor aware funding for unconfirmed inputs") 405 | + 406 | + # higher parent feerate 407 | + self.def_wallet.sendtoaddress(address=self.wallet.getnewaddress(), amount=17, fee_rate=20) 408 | + 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_walletis not equal toself.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.
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?
391 | + 392 | + @cleanup 393 | + def sendall_spends_unconfirmed_inputs_if_specified(self): 394 | + self.log.info("Test that sendall spends specified unconfirmed inputs") 395 | + self.def_wallet.sendtoaddress(self.wallet.getnewaddress(), 17) 396 | + assert_greater_than(self.wallet.getbalances()["mine"]["untrusted_pending"], 0)
In c9746711:
Need to flush the validation queue before checking the wallet balance. The transaction could have not been processed by the validation interface worker thread. Call self.wallet.syncwithvalidationinterfacequeue().
(saw it failing locally).
Would be better to check for the exact amount. You are sending exactly 17 btc to the wallet.
Done
1481 | + CAmount effective_value{total_input_value - fee_from_size}; 1482 | + 1483 | + const std::optional<CAmount> total_bump_fees{pwallet->chain().calculateCombinedBumpFee(outpoints_spent, fee_rate)}; 1484 | + if (total_bump_fees) { 1485 | + effective_value -= *total_bump_fees; 1486 | + }
nit: could write it:
const TxSize tx_size{CalculateMaximumSignedTxSize(CTransaction(rawTx), pwallet.get())};
const CAmount fee_from_size{fee_rate.GetFee(tx_size.vsize)};
const std::optional<CAmount> total_bump_fees{pwallet->chain().calculateCombinedBumpFee(outpoints_spent, fee_rate)};
CAmount effective_value = total_input_value - fee_from_size - total_bump_fees.value_or(0);
Done
393 | @@ -394,11 +394,48 @@ def sendall_spends_unconfirmed_inputs_if_specified(self): 394 | self.log.info("Test that sendall spends specified unconfirmed inputs") 395 | self.def_wallet.sendtoaddress(self.wallet.getnewaddress(), 17) 396 | assert_greater_than(self.wallet.getbalances()["mine"]["untrusted_pending"], 0) 397 | + self.wallet.syncwithvalidationinterfacequeue()
In e73a56f13:
This change should be in the first commit, not here. And the sync call should be done prior to calling getbalances() too (one line above the current one).
Done
420 | + child_tx = self.wallet.decoderawtransaction(child_hex) 421 | + higher_parent_feerate_amount = child_tx["vout"][0]["value"] 422 | + 423 | + # lower parent feerate 424 | + self.def_wallet.sendtoaddress(address=self.wallet.getnewaddress(), amount=17, fee_rate=10) 425 | + assert_greater_than(self.wallet.getbalances()["mine"]["untrusted_pending"], 0)
Same as before, would be better to not use assert_greater_than, the wallet is receiving exactly 17 btc.
Done, but it is 34 because the wallet also received 17 previously.
420 | + child_tx = self.wallet.decoderawtransaction(child_hex) 421 | + higher_parent_feerate_amount = child_tx["vout"][0]["value"] 422 | + 423 | + # lower parent feerate 424 | + self.def_wallet.sendtoaddress(address=self.wallet.getnewaddress(), amount=17, fee_rate=10) 425 | + assert_equal(self.wallet.getbalances()["mine"]["untrusted_pending"], 34)
Receiving a test failure here:
2024-02-28T02:12:00.025000Z TestFramework (INFO): Test that sendall does ancestor aware funding for unconfirmed inputs
2024-02-28T02:12:00.174000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/path/to/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
self.run_test()
File "/path/to/bitcoin/test/functional/wallet_sendall.py", line 527, in run_test
self.sendall_does_ancestor_aware_funding()
File "/path/to/bitcoin/test/functional/wallet_sendall.py", line 20, in wrapper
func(self)
File "/path/to/bitcoin/test/functional/wallet_sendall.py", line 425, in sendall_does_ancestor_aware_funding
assert_equal(self.wallet.getbalances()["mine"]["untrusted_pending"], 34)
File "/path/to/bitcoin/test/functional/test_framework/util.py", line 57, in assert_equal
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(17.00000000 == 34)
Need to add a syncwithvalidationinterfacequeue() after the send call. The mempool event that notifies the second wallet is consumed by a different thread (not the http one).
Should be fixed now, sorry for the delay.
Concept ACK. Nice addition (adding more ancestor awareness). Received a test failure (inline comment provided).
ACK modulo existing comments.
ACK 71aae72e1f
ACK 71aae72e1fc998b2629d68a7301d85dc1b65641e
ACK 71aae72e1fc998b2629d68a7301d85dc1b65641e