test: Return new_utxo from create_self_transfer in MiniWallet #25445

MarcoFalke commented at 8:57 AM on June 22, 2022: member

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

fanquake added the label Tests on Jun 22, 2022

in test/functional/mempool_reorg.py:72 in fae27b88b5 outdated

  67 | @@ -68,10 +68,8 @@ def run_test(self):
  68 |          assert_raises_rpc_error(-26, 'non-final', self.nodes[0].sendrawtransaction, timelock_tx)
  69 |  
  70 |          self.log.info("Create spend_2_1 and spend_3_1")
  71 | -        spend_2_utxo = wallet.get_utxo(txid=spend_2['txid'])
  72 | -        spend_2_1 = wallet.create_self_transfer(utxo_to_spend=spend_2_utxo)
  73 | -        spend_3_utxo = wallet.get_utxo(txid=spend_3['txid'])
  74 | -        spend_3_1 = wallet.create_self_transfer(utxo_to_spend=spend_3_utxo)
  75 | +        spend_2_1 = wallet.create_self_transfer(utxo_to_spend=spend_2["new_utxo"])
  76 | +        spend_3_1 = wallet.create_self_transfer(utxo_to_spend=spend_3["new_utxo"])

kouloumos commented at 4:40 PM on June 22, 2022:

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?

MarcoFalke commented at 1:01 PM on June 27, 2022:

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?

kouloumos commented at 1:41 PM on June 27, 2022:

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.

kouloumos commented at 5:15 PM on June 22, 2022: member

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_utxo helper in generate?

MarcoFalke force-pushed on Jun 23, 2022

MarcoFalke commented at 9:57 AM on June 23, 2022: member

Very good feedback, thanks! I fixed all of it in the last push.

MarcoFalke force-pushed on Jun 23, 2022

laanwj commented at 11:50 AM on June 23, 2022: member

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 Looks like faf3e7f94a43fbacb71b12adaa648da862d53d9d is unrelated to the PR scope but it's fine with me to leave it in.

MarcoFalke commented at 1:07 PM on June 23, 2022: member

(force pushed to fix linter)

in test/functional/test_framework/wallet.py:119 in fa0b232117 outdated

 112 | @@ -110,13 +113,22 @@ def rescan_utxos(self):
 113 |          res = self._test_node.scantxoutset(action="start", scanobjects=[self.get_descriptor()])
 114 |          assert_equal(True, res['success'])
 115 |          for utxo in res['unspents']:
 116 | -            self._utxos.append({'txid': utxo['txid'], 'vout': utxo['vout'], 'value': utxo['amount'], 'height': utxo['height']})
 117 | +            self._utxos.append(self._create_utxo(txid=utxo["txid"], vout=utxo["vout"], value=utxo["amount"], height=utxo["height"]))
 118 |  
 119 |      def scan_tx(self, tx):
 120 |          """Scan the tx for self._scriptPubKey outputs and add them to self._utxos"""

kouloumos commented at 4:32 PM on June 24, 2022:

maybe this should now mention that it also removes spent utxos? something like "...and update the internal utxo list" instead of "add"

MarcoFalke commented at 9:11 AM on June 27, 2022:

Thx, adjusted docs

kouloumos commented at 5:02 PM on June 24, 2022: member

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.

test: Return new_utxo from create_self_transfer in MiniWallet fa34e44e98

test: Return new_utxos from create_self_transfer_multi in MiniWallet fa04ff61b6

test: Drop spent utxos in MiniWallet scan_tx dddd7c4d39

test: Sync MiniWallet utxo state after each generate call fa13375aa3

test: Remove unused call to generate in rpc_mempool_info

There are already enough blocks

fa83c0c44f

MarcoFalke force-pushed on Jun 27, 2022

kouloumos commented at 12:41 PM on June 27, 2022: member

re-ACK fa83c0c44ff2072f7c3e56000edc805321bcf41a 🚀

MarcoFalke merged this on Jun 27, 2022

MarcoFalke closed this on Jun 27, 2022

MarcoFalke deleted the branch on Jun 27, 2022

jamesob commented at 2:23 PM on June 27, 2022: member

Posthumous crACK; looks good!

sidhujag referenced this in commit b4eeef501d on Jun 27, 2022

Fabcien referenced this in commit 9811dc648f on Dec 3, 2022

DrahtBot locked this on Jun 27, 2023