test: Return new_utxo from create_self_transfer in MiniWallet #25445
Conversation
kouloumos
left a comment
kouloumos
left a comment
There was a problem hiding this comment.
Code Review ACK fae27b88b5dee26ba51894b2e1511204a73d1022
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 https://github.com/bitcoin/bitcoin/blob/fab72801f021e707238ba5f4428f6257cf1c5251/test/functional/mempool_package_limits.py#L53-L54
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?
test/functional/mempool_reorg.py
Outdated
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.
maflcko
force-pushed
the
2206-test-miniwallet-new-utxo-
branch
from
fae27b8 to
faf199c
Compare
|
Very good feedback, thanks! I fixed all of it in the last push. |
maflcko
force-pushed
the
2206-test-miniwallet-new-utxo-
branch
2 times, most recently
from
fab9dca to
fa0b232
Compare
|
Yes, why not, the purpose of a testing API is that it's convenient for testing, don't see a drawback in returning the extra information always. Code review ACK fab9dca87d407f7045aa4b85441309d55c53cbaa |
|
(force pushed to fix linter) |
kouloumos
left a comment
kouloumos
left a comment
There was a problem hiding this comment.
ACK fa0b23211746d8c400fd7de51b1f3c5ecdaf4ccc
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 faa2750b864a56d04749a4a1ca14eecccd3583f8 is reasonable to make sure that the internal utxo list is in-sync.
good catch with fa95e1f176d3bd97c0aeb0b25f633c41659ddc62!
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
maflcko
force-pushed
the
2206-test-miniwallet-new-utxo-
branch
from
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
5 participants
I need this for some stuff, but it also makes sense on its own to:
_create_utxohelper