test: convert feature_bip68_sequence.py to use MiniWallet #21144

danben commented at 10:46 PM on February 10, 2021: contributor

This is within the scope of issue #20078.

Changes:

Removed skip_if_no_wallet and all uses of wallet RPCs from feature_bip_sequence.py
Added new functionality to wallet.py
- generate_random_outputs, a function that will create a multi-output transaction based on a single utxo in the wallet
- get_confirmations, a function that returns the number of confirmations for a given (confirmed) utxo
- get_utxos, a function that removes and returns the last n transactions in the wallet
Changed existing functionality in wallet.py
- send_self_transfer now accepts the following parameters: send_values, nSequence and nVersion
- send_self_transfer will now create a transaction with multiple outputs, if multiple send_values are specified

danben commented at 10:48 PM on February 10, 2021: contributor

I'm still very new to the codebase so I'm sure I did lots of things suboptimally, but this should be a decent starting point.

Thanks for reviewing!

danben force-pushed on Feb 10, 2021

fanquake added the label Tests on Feb 10, 2021

danben renamed this:
~~test: convert feature_bip_sequence.py to use MiniWallet~~
test: convert feature_bip68_sequence.py to use MiniWallet
on Feb 11, 2021

DrahtBot commented at 5:58 AM on February 11, 2021: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21390 (test: Test improvements for UTXO set hash tests by fjahr)
#21178 (test: run mempool_reorg.py even with wallet disabled by DariusParvin)
#21014 (test: Run mempool_accept.py even with wallet disabled by stackman27)
#20889 (test: ensure any failing method in MiniWallet leaves no side-effects by mjdietzx)
#20874 (test: Run mempool_limit.py even with wallet disabled by stackman27)
#20808 (test: Run rpc_generateblock.py even with wallet disabled by nginocchio)

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.

in test/functional/feature_bip68_sequence.py:42 in e89ee566b7 outdated

  39 | @@ -38,13 +40,15 @@ def set_test_params(self):
  40 |          ]
  41 |  
  42 |      def skip_test_if_missing_module(self):

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

No need for this method. You can just remove it

in test/functional/test_framework/wallet.py:35 in e89ee566b7 outdated

  31 | @@ -32,14 +32,39 @@ def __init__(self, test_node):
  32 |          self._address = ADDRESS_BCRT1_P2WSH_OP_TRUE
  33 |          self._scriptPubKey = hex_str_to_bytes(self._test_node.validateaddress(self._address)['scriptPubKey'])
  34 |  
  35 | +    def num_utxos(self):

stackman27 commented at 4:26 PM on February 15, 2021:

This method hasn't been used elsewhere. Is there a need for it?

danben commented at 5:59 PM on February 15, 2021:

Nope, that's left over from an earlier version. I'll get rid of it, thanks.

in test/functional/test_framework/wallet.py:80 in e89ee566b7 outdated

  79 | @@ -55,25 +80,41 @@ def get_utxo(self, *, txid=''):
  80 |              index = self._utxos.index(utxo)
  81 |          return self._utxos.pop(index)
  82 |  
  83 | -    def send_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_spend=None):
  84 | +    def get_utxos(self, amount):

stackman27 commented at 4:42 PM on February 15, 2021:

Why not just have a getter method for get_self_utxos, and pop it while running utxos += self.wallet.get_utxos(50). Seems cleaner no?

danben commented at 5:56 PM on February 15, 2021:

Exposing self._utxos via a getter weakens the existing guarantees of the class (i.e. the list of utxos is not writeable from the outside). It didn't seem like an appropriate change to make. (At the very least, someone made a deliberate choice not to expose this field and I didn't want to undo that.)

stackman27 commented at 4:34 PM on February 16, 2021:

I see if that's the case its fine. Just out of curiosity when you said, "Someone made a delibrate choice...", is there a specific PR or dev notes that you can link where getter method has been discarded

maflcko commented at 4:44 PM on February 16, 2021:

Just for the sake of abstraction, so that outside code doesn't access the inner "organs" of MiniWallet, I didn't write a getter for the private member _utxos.

As pointed out by @danben the "list of utxos is not writeable from the outside"

stackman27 commented at 4:46 PM on February 16, 2021:

aah i see that makes sense. Thank you!

in test/functional/test_framework/wallet.py:82 in e89ee566b7 outdated

  79 | @@ -55,25 +80,41 @@ def get_utxo(self, *, txid=''):
  80 |              index = self._utxos.index(utxo)
  81 |          return self._utxos.pop(index)
  82 |  
  83 | -    def send_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node, utxo_to_spend=None):
  84 | +    def get_utxos(self, amount):
  85 | +        """Removes and returns the last {amount} utxos from the wallet"""
  86 | +        assert amount <= len(self._utxos)

stackman27 commented at 4:43 PM on February 15, 2021:

Also i'm having hard time understanding this assertion. Why are you comparing amount to the length of utxos?

danben commented at 5:58 PM on February 15, 2021:

If you pass a negative value, whose absolute value is larger than the length of the list, as the first index of a slice operator you'll just get back the whole list. This assertion protects against what is most likely user error.

stackman27 commented at 5:23 PM on February 15, 2021: contributor

Left few comments, still reviewing send_self_transfer... Also, I think you should split the commits one for feature_bip68_sequence.py and another for miniwallet

in test/functional/test_framework/wallet.py:43 in e89ee566b7 outdated

  42 |              cb_tx = self._test_node.getblock(blockhash=b, verbosity=2)['tx'][0]
  43 | -            self._utxos.append({'txid': cb_tx['txid'], 'vout': 0, 'value': cb_tx['vout'][0]['value']})
  44 | +            self._utxos.append({'txid': cb_tx['txid'], 'vout': 0, 'value': cb_tx['vout'][0]['value'], 'blockhash':b})
  45 |          return blocks
  46 |  
  47 | +    def generate_random_outputs(self, utxo, num_outputs, confirmed=True):

stackman27 commented at 3:50 PM on February 16, 2021:

Is confirmed= True needed? Seems to be true all the time as of now

danben commented at 4:19 PM on February 19, 2021:

It's true that I only use this function in feature_bip68_sequence.py to create confirmed outputs, but generate_random_outputs belongs to wallet.py, not a specific functional test. It seems more broadly useful to allow the user to create unconfirmed outputs if they like (and with the amount of tests left to convert I suspect it will come up at some point).

in test/functional/test_framework/wallet.py:78 in e89ee566b7 outdated

 125 | +        for i, send_value in enumerate(send_values):
 126 | +            self._utxos.append({'txid': tx_info['txid'], 'vout': i, 'value': satoshi_round(send_value - fee_per_output)})
 127 | +
 128 | +        assert_equal(tx_info['txid'], from_node.sendrawtransaction(tx_hex))
 129 |          assert_equal(tx_info['vsize'], vsize)
 130 | -        assert_equal(tx_info['fees']['base'], fee)

stackman27 commented at 4:32 PM on February 16, 2021:

I believe for the fees assertion, you weren't taking the whole fees of the tx into consideration. If you print(tx) in line 109, you'll see tx with one input and lots of outputs. fee_per_output only calculated tx for a single set of output, but here you need to calculate fee for the whole tx. I tried doing something like this and the assertion worked! lmk if that was helpful

...
total_base_fee = 0
        for send_value in send_values:
            output_value = satoshi_round(send_value - fee_per_output)
            total_base_fee += satoshi_round(send_value - output_value)
            total_sent += output_value
            tx.vout.append(CTxOut(int(output_value * COIN), self._scriptPubKey))
...
assert_equal(tx_info['fees']['base'], total_base_fee)

danben commented at 4:17 PM on February 19, 2021:

Thanks, that was helpful - I actually had something similar before but I think my mistake was forgetting to use satoshi_round on fee_per_output, and then I just convinced myself that I didn't know what fees-base actually represented.

stackman27 commented at 4:32 PM on February 16, 2021: contributor

Reviewed send_self_transfer. Also fees_assertion should work now :)

danben force-pushed on Feb 19, 2021

test: convert feature_bip_sequence.py to use MiniWallet

This is within the scope of issue #20078.
Changes:
 - Removed skip_if_no_wallet and all uses of wallet RPCs from feature_bip_sequence.py
 - Added new functionality to wallet.py
   - generate_random_outputs, a function that will create a multi-output transaction based on a single utxo in the wallet
   - get_confirmations, a function that returns the number of confirmations for a given (confirmed) utxo
   - get_utxos, a function that removes and returns the last n transactions in the wallet
 - Changed existing functionality in wallet.py
   - send_self_transfer now accepts the following parameters: send_values, nSequence and nVersion
   - send_self_transfer will now create a transaction with multiple outputs, if multiple send_values are specified

520e7799b5

danben force-pushed on Feb 19, 2021

DrahtBot added the label Needs rebase on Mar 26, 2021

DrahtBot commented at 9:33 AM on March 26, 2021: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

maflcko added the label Up for grabs on Aug 26, 2021

maflcko commented at 4:36 PM on August 26, 2021: member

Marked up for grabs, because it still needs rebase

fanquake closed this on Aug 27, 2021

bitcoin locked this on Aug 27, 2022

maflcko removed the label Up for grabs on Dec 12, 2023