test: Return new_utxo from create_self_transfer in MiniWallet #25445
Conversation
There was a problem hiding this comment.
Code Review ACK fae27b8
That makes sense, it seems that there is no "easy" way to get ownership of a utxo that it is only created and not send and thus be accessible using get_utxo. This is useful in occasions where you need ownership of a utxo that is not in the mempool. Due to that limitation I use this "ugly" approach in another occasion
bitcoin/test/functional/mempool_package_limits.py
Lines 53 to 54 in fab7280
A few questions
- Under the same justification, does it make sense to implement the same behaviour for
create_self_transfer_multi? - Is there a reason to not use the
_create_utxohelper ingenerate?
| spend_2_1 = wallet.create_self_transfer(utxo_to_spend=spend_2["new_utxo"]) | ||
| spend_3_1 = wallet.create_self_transfer(utxo_to_spend=spend_3["new_utxo"]) |
There was a problem hiding this comment.
This is not an issue in this test, but is it generally a concern that with this particular usage pattern the spend_{2,3}["new_utxo"] remains in the MiniWallet's internal utxo list because of its addition through the preceding wallet.sendrawtransaction call? and hence a subsequent usage of the *_self_transfer method might spend it?
There was a problem hiding this comment.
Just for reference, the utxo will still stay in the MiniWallet in this test, as the tx is broadcast with the node's sendrawtransaction, not with the MiniWallet.sendrawtransaction method.
Also, the generate below doesn't reset it, for the same reason.
I guess it will be hard to prevent in practise, so it seems fine to leave as-is, unless there is a test failure?
There was a problem hiding this comment.
I would agree that is not such of a big deal, maybe until this kind of usage leads to unexpected behavior. I cannot see a way to prevent that, and neither I checked for existing or thought about future cases where usage of node's sendrawtransaction & generate might be required alongside MiniWallet usage.
fae27b8 to
faf199c
Compare
|
Very good feedback, thanks! I fixed all of it in the last push. |
fab9dca to
fa0b232
Compare
|
(force pushed to fix linter) |
There was a problem hiding this comment.
ACK fa0b232
Currently the only way that you could use a UTXO on a subsequent spend is by getting ownership with get_utxo which also removes it from the internal UTXO list. This PR enables ownership without that intermediary stage thus faa2750 is reasonable to make sure that the internal utxo list is in-sync.
good catch with fa95e1f!
This PR now allows the underlying utxo set changes to better simulate reality, which will probably allow for better tests.
There are already enough blocks
fa0b232 to
fa83c0c
Compare
|
re-ACK fa83c0c 🚀 |
|
Posthumous crACK; looks good! |
Summary:
```
I need this for some stuff, but it also makes sense on its own to:
unify the flow with a private _create_utxo helper
simplify the flow by giving the caller ownership of the utxo right away
```
Backport of [[bitcoin/bitcoin#25445 | core#25445]].
Depends on D12736.
Note that all the changes are not applicable to our codebase but the most important ones do.
Test Plan:
ninja check-functional
Reviewers: #bitcoin_abc, PiRK
Reviewed By: #bitcoin_abc, PiRK
Differential Revision: https://reviews.bitcoinabc.org/D12737
I need this for some stuff, but it also makes sense on its own to:
_create_utxohelper