test: ensure any failing method in MiniWallet leaves no side-effects #20889
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
Concept ACK. Reviewed and tested send-self-transfer, still testing generate method. Actually, faced a related issue while implementing tx after an exception in mempool_limit.py, but just went with the intuition that we pop the invalid utxo therefore the rest stays valid 😅
So I believe the invalid utxo stays in the list even after popping and/or exception in send_self_transfer?
|
The initial design was that none of the calls would be allowed to throw, but no objection to making it more robust and extensible. |
Thanks! So before this PR the invalid utxo would stay in the list (unless you explicitly popped it out yourself when you handle the exception). After this PR, the invalid utxo would never have been appended to the list if |
c3ac93d to
140e429
Compare
| cb_tx = self._test_node.getblock(blockhash=b, verbosity=2)['tx'][0] | ||
| self._utxos.append({'txid': cb_tx['txid'], 'vout': 0, 'value': cb_tx['vout'][0]['value']}) | ||
| cb_txs = map(lambda b: self._test_node.getblock(blockhash=b, verbosity=2)['tx'][0], blocks) | ||
| cb_utxos = map(lambda cb_tx: {'txid': cb_tx['txid'], 'vout': 0, 'value': cb_tx['vout'][0]['value']}, cb_txs) |
There was a problem hiding this comment.
Maybe it's just me but I found these lambda functions a little bit harder to parse compared to the original for loop. Perhaps it would be more readable to create a temporary variable and add it to the utxo list, similar to how utxos are handled in self_send_transfer?
| cb_utxos = map(lambda cb_tx: {'txid': cb_tx['txid'], 'vout': 0, 'value': cb_tx['vout'][0]['value']}, cb_txs) | |
| new_utxos = [] | |
| for b in blocks: | |
| cb_tx = self._test_node.getblock(blockhash=b, verbosity=2)['tx'][0] | |
| new_utxos.append({'txid': cb_tx['txid'], 'vout': 0, 'value': cb_tx['vout'][0]['value']}) | |
| self._utxos.extend(new_utxos) |
There was a problem hiding this comment.
I prefer map/filter/reduce combined with lambda functions. I know this kind of diverges from most of the python in the codebase though. So I can definitely be convinced to stick with loops like this, but for now I'll leave it and see if anyone else weighs in
140e429 to
a83cccc
Compare
|
Closing in favor of #21762 |
On top of #20876 only review this commit 3b9b083
This PR gets rid of any side-effects that methods in
MiniWalletmight leave if an exception is thrown. ie beforehand, ifsendrawtransactionfailed we would have had an invalid utxo left inself._utxos. More subtle - beforehand, ifsend_self_transferfailedself._utxosis always going to be modified bc the first thingsend_self_transferdoes is sortself._utxos. I think this is confusing, especially if tests are catching exceptionsMaybe I'm being paranoid with
generate, and side-effects probably wouldn't cause as much confusion here. But originally, if anything failed in the for-loop https://github.com/bitcoin/bitcoin/compare/master...mjdietzx:test-miniwallet-no-side-effects?expand=1#diff-7932a60a9127fd22d10d367526fd7b987f9647ce017595f8b1af5c32d5db0083L38-L40 thenself._utxoswould include utxos for all items processed before the failure. Seems like we'd prefer no side-effect to me