test: ensure any failing method in MiniWallet leaves no side-effects #20889

mjdietzx commented at 9:29 PM on January 8, 2021: contributor

On top of #20876 only review this commit 3b9b0831d4ce152791e7953d175aa90b78008155

This PR gets rid of any side-effects that methods in MiniWallet might leave if an exception is thrown. ie beforehand, if sendrawtransaction failed we would have had an invalid utxo left in self._utxos. More subtle - beforehand, if send_self_transfer failed self._utxos is always going to be modified bc the first thing send_self_transfer does is sort self._utxos. I think this is confusing, especially if tests are catching exceptions

Maybe 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 then self._utxos would include utxos for all items processed before the failure. Seems like we'd prefer no side-effect to me

DrahtBot added the label Tests on Jan 8, 2021

DrahtBot commented at 3:48 AM on January 9, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21762 (test: Speed up mempool_spend_coinbase.py by MarcoFalke)
#21754 (test: Run feature_cltv with MiniWallet by MarcoFalke)
#21178 (test: run mempool_reorg.py even with wallet disabled by DariusParvin)
#21014 (test: Run mempool_accept.py even with wallet disabled by stackman27)
#20874 (test: Run mempool_limit.py even with wallet disabled by stackman27)

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.

stackman27 commented at 11:30 PM on January 9, 2021: contributor

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?

MarcoFalke commented at 9:35 AM on January 10, 2021: member

The initial design was that none of the calls would be allowed to throw, but no objection to making it more robust and extensible.

mjdietzx commented at 6:44 PM on January 10, 2021: contributor

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?

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 send_self_transfer failed. So after this PR it would be more robust, and I think more intuitive

DrahtBot added the label Needs rebase on Jan 15, 2021

mjdietzx force-pushed on Feb 1, 2021

DrahtBot removed the label Needs rebase on Feb 1, 2021

in test/functional/test_framework/wallet.py:79 in 140e429c16 outdated

  71 | @@ -72,8 +72,9 @@ def send_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_sp
  72 |          tx_hex = tx.serialize().hex()
  73 |  
  74 |          tx_info = from_node.testmempoolaccept([tx_hex])[0]
  75 | -        self._utxos.append({'txid': tx_info['txid'], 'vout': 0, 'value': send_value})
  76 | +        utxos.append({'txid': tx_info['txid'], 'vout': 0, 'value': send_value})
  77 |          from_node.sendrawtransaction(tx_hex)
  78 |          assert_equal(tx_info['vsize'], vsize)
  79 |          assert_equal(tx_info['fees']['base'], fee)
  80 | +        self._utxos = utxos  # update our internal utxo set after we are sure send_self_transfer will succeed

stackman27 commented at 3:28 PM on February 15, 2021:

nit: Aren't we assigning utxos when sendrawtransaction succeeds? So, shouldn't it be #update... after we are sure that sendrawtransaction will succeed because in the case if we add sign_dont_send options, the send_self_transfer succeeds but dont spend tx?

mjdietzx commented at 3:13 PM on April 28, 2021:

Good point - I improved the comment. Pushed and rebased

stackman27 commented at 3:33 PM on February 15, 2021: contributor

Everything looks good to go, left a comment before giving an ACK :)

in test/functional/test_framework/wallet.py:48 in 140e429c16 outdated

  34 | @@ -35,9 +35,9 @@ def __init__(self, test_node):
  35 |      def generate(self, num_blocks):
  36 |          """Generate blocks with coinbase outputs to the internal address, and append the outputs to the internal list"""
  37 |          blocks = self._test_node.generatetoaddress(num_blocks, self._address)
  38 | -        for b in blocks:
  39 | -            cb_tx = self._test_node.getblock(blockhash=b, verbosity=2)['tx'][0]
  40 | -            self._utxos.append({'txid': cb_tx['txid'], 'vout': 0, 'value': cb_tx['vout'][0]['value']})
  41 | +        cb_txs = map(lambda b: self._test_node.getblock(blockhash=b, verbosity=2)['tx'][0], blocks)
  42 | +        cb_utxos = map(lambda cb_tx: {'txid': cb_tx['txid'], 'vout': 0, 'value': cb_tx['vout'][0]['value']}, cb_txs)

DariusParvin commented at 6:00 AM on April 8, 2021:

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?

        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)

mjdietzx commented at 3:17 PM on April 28, 2021:

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

DariusParvin approved

DariusParvin commented at 6:04 AM on April 8, 2021: contributor

Concept ACK! The changes make the utxo handling more robust. Just left one suggestion for readability.

test: ensure any failing method in miniwallet leaves no side-effects a83cccc432

mjdietzx force-pushed on Apr 28, 2021

in test/functional/test_framework/wallet.py:91 in a83cccc432

  83 | @@ -84,8 +84,11 @@ def send_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_sp
  84 |          tx_hex = tx.serialize().hex()
  85 |  
  86 |          tx_info = from_node.testmempoolaccept([tx_hex])[0]
  87 | -        self._utxos.append({'txid': tx_info['txid'], 'vout': 0, 'value': send_value})
  88 | +        utxos.append({'txid': tx_info['txid'], 'vout': 0, 'value': send_value})
  89 |          from_node.sendrawtransaction(tx_hex)
  90 |          assert_equal(tx_info['vsize'], vsize)
  91 |          assert_equal(tx_info['fees']['base'], fee)
  92 | +        # update our internal utxo set after `sendrawtransaction` and the above assertions succeed to ensure a thrown exception

MarcoFalke commented at 4:19 PM on April 28, 2021:

I fixed this in #21762 by moving the self._utxos.append to the line after sendrawtransaction. Wdyt?

mjdietzx commented at 5:24 PM on April 28, 2021:

Looks great, prefer you're implementation

mjdietzx commented at 5:24 PM on April 28, 2021: contributor

Closing in favor of https://github.com/bitcoin/bitcoin/pull/21762

mjdietzx closed this on Apr 28, 2021

DrahtBot locked this on Aug 18, 2022