test: Run feature_bip68_sequence.py with MiniWallet

miles170 commented at 7:28 am on December 8, 2022: contributor

This PR enables one more of the non-wallet functional tests (feature_bip68_sequence.py) to be run even when no wallet is compiled in by using the MiniWallet instead, as proposed in #20078.

miles170 marked this as a draft on Dec 8, 2022

miles170 force-pushed on Dec 8, 2022

DrahtBot commented at 7:35 am on December 8, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, MarcoFalke

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26886 (test: add rescan utxos inside MiniWallet’s initialization by kouloumos)
#26857 (OP_VAULT draft by jamesob)
#26625 (test: Run mempool_packages.py with MiniWallet by MarcoFalke)

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.

miles170 marked this as ready for review on Dec 8, 2022

miles170 force-pushed on Dec 8, 2022

maflcko added the label Docs on Dec 8, 2022

maflcko added the label Tests on Dec 8, 2022

maflcko removed the label Docs on Dec 8, 2022

maflcko removed the label Tests on Dec 8, 2022

DrahtBot added the label Tests on Dec 8, 2022

in test/functional/feature_bip68_sequence.py:96 in 192a00cf49 outdated

 97-        utxos = self.nodes[0].listunspent(0, 0)
 98-        assert len(utxos) > 0
 99+        self.wallet.send_self_transfer(from_node=self.nodes[0])
100 
101-        utxo = utxos[0]
102+        utxo = self.wallet.get_utxo()

maflcko commented at 3:12 pm on December 8, 2022:

This may not be unconfirmed. If you need the unconfirmed one from above you can fetch is with ["new_utxo"].

Unrelated, I wonder why this needs to be unconfirmed

miles170 commented at 3:29 pm on December 8, 2022:

Should I change the code to match the origin behavior, which is unconfirmed UTXO?

 0diff --git a/test/functional/feature_bip68_sequence.py b/test/functional/feature_bip68_sequence.py
 1index e53e177ce..e14867dc3 100755
 2--- a/test/functional/feature_bip68_sequence.py
 3+++ b/test/functional/feature_bip68_sequence.py
 4@@ -91,9 +91,7 @@ class BIP68Test(BitcoinTestFramework):
 5     # the first sequence bit is set.
 6     def test_disable_flag(self):
 7         # Create some unconfirmed inputs
 8-        self.wallet.send_self_transfer(from_node=self.nodes[0])
 9-
10-        utxo = self.wallet.get_utxo()
11+        utxo = self.wallet.send_self_transfer(from_node=self.nodes[0])["new_utxo"]
12 
13         tx1 = CTransaction()
14         value = int((utxo["value"] - self.relayfee) * COIN)

miles170 force-pushed on Dec 8, 2022

miles170 commented at 3:49 pm on December 8, 2022: contributor

Force-pushed with taking suggestion #26657 (review) and remove formatting fixes.

miles170 marked this as a draft on Dec 9, 2022

miles170 commented at 9:55 am on December 9, 2022: contributor

interface_usdt_utxocache.py is failing due to test: Ignore spent utxos in MiniWallet rescan_utxos, I’m working on it.

It seems gettxout in MiniWallet.rescan_utxos calls GetCoin, which adds the UTXO to the temporary cache. Do we have an alternative way to ignore spent utxos in MiniWallet rescan_utxos?

Or, to ensure that the mempool is cleared, could I replace self.wallet.rescan_utxos() with self.generate(self.wallet, 1)?

 0diff --git a/test/functional/feature_bip68_sequence.py b/test/functional/feature_bip68_sequence.py
 1index 379d04a1f..6030e7138 100755
 2--- a/test/functional/feature_bip68_sequence.py
 3+++ b/test/functional/feature_bip68_sequence.py
 4@@ -212,7 +212,7 @@ class BIP68Test(BitcoinTestFramework):
 5             else:
 6                 # This raw transaction should be accepted
 7                 self.wallet.sendrawtransaction(from_node=self.nodes[0], tx_hex=rawtx)
 8-                self.wallet.rescan_utxos()
 9+                self.generate(self.wallet, 1)
10                 utxos = self.wallet.get_utxos(include_immature_coinbase=False)
11
12     # Test that sequence locks on unconfirmed inputs must have nSequence

in test/functional/test_framework/wallet.py:128 in 912926528c outdated

124@@ -124,7 +125,16 @@ def rescan_utxos(self):
125         res = self._test_node.scantxoutset(action="start", scanobjects=[self.get_descriptor()])
126         assert_equal(True, res['success'])
127         for utxo in res['unspents']:
128-            self._utxos.append(self._create_utxo(txid=utxo["txid"], vout=utxo["vout"], value=utxo["amount"], height=utxo["height"]))
129+            # Ignore utxo inside mempool

maflcko commented at 10:52 am on December 9, 2022:

Can you explain why? scantxoutset doesn’t scan the mempool, does it?

miles170 commented at 11:55 am on December 9, 2022:

Without that, the test throw txn-mempool-conflict, and get_utxos indeed return the tx that is inside the mempool.

in test/functional/feature_bip68_sequence.py:215 in 912926528c outdated

215             else:
216                 # This raw transaction should be accepted
217-                self.nodes[0].sendrawtransaction(rawtx)
218-                utxos = self.nodes[0].listunspent()
219+                self.wallet.sendrawtransaction(from_node=self.nodes[0], tx_hex=rawtx)
220+                self.wallet.rescan_utxos()

maflcko commented at 4:16 pm on December 9, 2022:

Why is this needed? wallet.sendrawtransaction should already account and scan the tx

miles170 commented at 4:52 am on December 10, 2022:

utxos = self.wallet.get_utxos() implies mark_as_spent=True, which means self._utxos is cleared. wallet.sendrawtransaction only appends the tx to self._utxos, so we need self.wallet.rescan_utxos() to rescan unspent utxos.

If I remove utxos = self.wallet.get_utxos(), the test throw error.

02022-12-11T05:34:00.235000Z TestFramework (ERROR): Unexpected exception caught during testing
1Traceback (most recent call last):
2  File "/bitcoin/test/functional/test_framework/test_framework.py", line 134, in main
3    self.run_test()
4  File "/bitcoin/./test/functional/feature_bip68_sequence.py", line 73, in run_test
5    self.test_sequence_lock_confirmed_inputs()
6  File "/bitcoin/./test/functional/feature_bip68_sequence.py", line 202, in test_sequence_lock_confirmed_inputs
7    tx.vin.append(CTxIn(COutPoint(int(utxos[j]["txid"], 16), utxos[j]["vout"]), nSequence=sequence_value))
8IndexError: list index out of range

maflcko commented at 3:23 pm on January 16, 2023:

Ah, ok. So alternatively the wallet.sendrawtransaction could be dropped/reverted to just sendrawtransaction, because it is not needed to scan the tx.

miles170 force-pushed on Dec 10, 2022

miles170 commented at 7:20 am on December 10, 2022: contributor

Force-pushed with squash commits and replace test: Ignore spent utxos in MiniWallet rescan_utxos with test: Add “include mempool” flag to MiniWallet rescan_utxos

 0diff --git a/test/functional/test_framework/wallet.py b/test/functional/test_framework/wallet.py                                                                            
 1index 46b628ee3..52bfb0a2e 100644                                                     
 2--- a/test/functional/test_framework/wallet.py                                  
 3+++ b/test/functional/test_framework/wallet.py                                                                                                                                                 
 4@@ -118,14 +119,13 @@ class MiniWallet:                                               
 5     def get_balance(self):                                                           
 6         return sum(u['value'] for u in self._utxos)                                  
 7                                          
 8-    def rescan_utxos(self):                                                                                                                                                
 9+    def rescan_utxos(self, *, include_mempool=True):                                 
10         """Drop all utxos and rescan the utxo set"""                                 
11         self._utxos = []                                                             
12         res = self._test_node.scantxoutset(action="start", scanobjects=[self.get_descriptor()])                                                                            
13         assert_equal(True, res['success'])                                           
14         for utxo in res['unspents']:                                                 
15-            # Ignore utxo inside mempool                                             
16-            if not self._test_node.gettxout(txid=utxo["txid"], n=utxo["vout"]):      
17+            if not include_mempool and not self._test_node.gettxout(txid=utxo["txid"], n=utxo["vout"]):                                                                    
18                 continue                                                                                                                                                   
19             self._utxos.append(                                                                                                                                            
20                 self._create_utxo(txid=utxo["txid"],

miles170 marked this as ready for review on Dec 10, 2022

in test/functional/test_framework/wallet.py:312 in 647bb7dfe3 outdated

308@@ -319,6 +309,20 @@ def create_self_transfer(self, *, fee_rate=Decimal("0.003"), fee=Decimal("0"), u
309 
310         return {"txid": tx["txid"], "wtxid": tx["tx"].getwtxid(), "hex": tx["hex"], "tx": tx["tx"], "new_utxo": tx["new_utxos"][0]}
311 
312+    def signrawtransactionwithwallet(self, *, tx):

maflcko commented at 9:41 am on December 12, 2022:

Any reason for a duplicate method when sign_tx already exists?

miles170 commented at 9:59 am on December 12, 2022:

sign_tx only works for MiniWalletMode.RAW_P2PK, and the default wallet mode is MiniWalletMode.ADDRESS_OP_TRUE, if we change tx.vin size we need signrawtransactionwithwallet to sign raw transaction on mode MiniWalletMode.RAW_OP_TRUE or MiniWalletMode.ADDRESS_OP_TRUE.

maflcko commented at 10:11 am on December 12, 2022:

I mean you could modify sign_tx instead of adding a new method?

miles170 force-pushed on Dec 12, 2022

miles170 commented at 12:44 pm on December 12, 2022: contributor

Force-pushed with taking suggestion #26657 (review)

in test/functional/test_framework/wallet.py:145 in 0c20a83fa6 outdated

154-            der_sig = self._priv_key.sign_ecdsa(sighash)
155-            if not fixed_length:
156-                break
157-        tx.vin[0].scriptSig = CScript([der_sig + bytes(bytearray([SIGHASH_ALL]))])
158-        tx.rehash()
159+        """Sign tx that has been created by MiniWallet in P2TR/RAWSCRIPT/P2PK mode"""

maflcko commented at 12:53 pm on December 12, 2022:

in 0c20a83fa658fc660b138382d907615b9a60b845: Any reason to duplicate the list of modes, given that it just repeats all modes?

miles170 commented at 12:58 pm on December 12, 2022:

Should I remove the comment or replace it with “Sign tx that has been created by MiniWallet”?

maflcko commented at 1:02 pm on December 12, 2022:

It is just a nit. You ignore it or can do whatever you feel most appropriate. Personally I think it is self-explanatory that a method named MiniWallet::sign_tx signs a transaction by MiniWallet, so it can be removed. Though “Sign tx that has been created by MiniWallet” is also fine.

in test/functional/test_framework/wallet.py:128 in 8213ff98c8 outdated

124         """Drop all utxos and rescan the utxo set"""
125         self._utxos = []
126         res = self._test_node.scantxoutset(action="start", scanobjects=[self.get_descriptor()])
127         assert_equal(True, res['success'])
128         for utxo in res['unspents']:
129+            if not include_mempool and not self._test_node.gettxout(txid=utxo["txid"], n=utxo["vout"]):

maflcko commented at 12:55 pm on December 12, 2022:

This just seems to exclude utxos if they are spent in mempool transactions. Shouldn’t it also include utxos if they are created in mempool transactions?

miles170 commented at 1:06 pm on December 12, 2022:

test_sequence_lock_confirmed_inputs would choose a random number of utxos from get_utxos, and if I included the transaction that is inside the mempool, the txn-mempool-conflict would occur.

maflcko commented at 1:14 pm on December 12, 2022:

I mean that someone setting the bool on the method would expect that this includes utxos created in the mempool. Also, I fail to see how including utxos created in the mempool can cause a conflict, as long as you remove utxos spent in the mempool.

This was also the behavior previously without MiniWallet, so it seems better to keep it, no?

Might be possible by calling scan_tx on all mempool txs, assuming they are topologically sorted.

miles170 commented at 1:15 pm on December 12, 2022:

Or should I use getrawtransaction to scan mempool, but this would require txindex=1.

maflcko commented at 1:19 pm on December 12, 2022:

getrawmempool does not require txindex

miles170 commented at 1:33 pm on December 12, 2022:

getrawmempool returns only the transactionid but not the tx.vin, which is required to scan spent UTXOs.

miles170 commented at 1:38 pm on December 12, 2022:

I mean that someone setting the bool on the method would expect that this includes utxos created in the mempool. Also, I fail to see how including utxos created in the mempool can cause a conflict, as long as you remove utxos spent in the mempool.

This was also the behavior previously without MiniWallet, so it seems better to keep it, no?

Might be possible by calling scan_tx on all mempool txs, assuming they are topologically sorted.

To summarize, keep the origin scan_tx behavior and, if necessary, remove UTXOs spent in mempool by the caller?

maflcko commented at 1:40 pm on December 12, 2022:

getrawmempool can return the transaction hex-encoded. You can decode it, or you may be able to call getmempoolentry if you need it decoded.

miles170 commented at 1:43 pm on December 12, 2022:

I’ll see what I can do tomorrow morning, thanks!

maflcko commented at 2:04 pm on December 12, 2022:

Ah, sorry. You can decode the full raw hex in python or with the decoderawtransaction RPC.

Unrelated: Maybe it could make sense to introduce a verbosity=3 to getrawmempool(entry)? But this is unrelated and can be done in a separate pull.

miles170 commented at 2:29 pm on December 12, 2022:

Sorry to bother you, but neither getrawmempool nor getmempoolentry return full raw hex.

maflcko commented at 2:35 pm on December 12, 2022:

Right, but getrawtransaction can

miles170 force-pushed on Dec 14, 2022

miles170 commented at 1:20 am on December 14, 2022: contributor

Force-pushed with taking suggestions #26657 (review) and #26657 (review)

in test/functional/test_framework/wallet.py:133 in 984114a5e1 outdated

129+        if not include_mempool:
130+            for txid in self._test_node.getrawmempool():
131+                tx = self._test_node.decoderawtransaction(self._test_node.getrawtransaction(txid))
132+                spent_txs.update([(i["txid"], i["vout"]) for i in tx["vin"]])
133         for utxo in res['unspents']:
134+            if not include_mempool and (utxo["txid"], utxo["vout"]) in spent_txs:

maflcko commented at 10:15 am on December 14, 2022:

Again, this will only exclude spent utxos in the mempool, but never include created ones, no?

miles170 commented at 10:18 am on December 14, 2022:

Do you mean the utxo created by vout?

include_mempool=False exclude spent utxos in the mempool and do not include utxos created in the mempool?
include_mempool=True include utxos created in the mempool?

 0diff --git a/test/functional/test_framework/wallet.py b/test/functional/test_framework/wallet.py
 1index dc1a41fcc..b9ba40f46 100644
 2--- a/test/functional/test_framework/wallet.py
 3+++ b/test/functional/test_framework/wallet.py
 4@@ -127,8 +127,9 @@ class MiniWallet:
 5         spent_txs = set()
 6         if not include_mempool:
 7             for txid in self._test_node.getrawmempool():
 8                 tx = self._test_node.decoderawtransaction(self._test_node.getrawtransaction(txid))
 9                 spent_txs.update([(i["txid"], i["vout"]) for i in tx["vin"]])
10+                # check tx["vout"] for unspent utxos?
11         for utxo in res['unspents']:
12             if not include_mempool and (utxo["txid"], utxo["vout"]) in spent_txs:
13                 continue

maflcko commented at 10:20 am on December 14, 2022:

Yes, a transaction in the mempool may have outputs (vout) that pay to the miniwallet

miles170 commented at 10:33 am on December 14, 2022:

Thank you for your patience, and I’m sorry for the misunderstanding.

miles170 commented at 1:25 pm on December 14, 2022:

Since ADDRESS_OP_TRUE or RAW_OP_TRUE are anyone-can-spend, should I have to validate scriptPubKey?

Or could I simply assume that all outputs (vout) are paid to the miniwallet?

maflcko commented at 1:40 pm on December 14, 2022:

Or could I simply assume that all outputs (vout) are paid to the miniwallet?

No, only the ones that pay to the address of the miniwallet.

miles170 commented at 2:02 pm on December 14, 2022:

There are so many different types of scriptPubKey, do we have a RPC API that can check if a certain vout belongs to the miniwallet?

Or should I write “helper method” (something like ExtractDestination in c++) to do the work?

maflcko commented at 2:26 pm on December 14, 2022:

You can use scan_tx

in test/functional/test_framework/wallet.py:130 in 984114a5e1 outdated

126         res = self._test_node.scantxoutset(action="start", scanobjects=[self.get_descriptor()])
127         assert_equal(True, res['success'])
128+        spent_txs = set()
129+        if not include_mempool:
130+            for txid in self._test_node.getrawmempool():
131+                tx = self._test_node.decoderawtransaction(self._test_node.getrawtransaction(txid))

maflcko commented at 10:15 am on December 14, 2022:

Does getrawtransaction have a verbose options to skip the decode call?

miles170 force-pushed on Dec 14, 2022

miles170 commented at 2:53 pm on December 14, 2022: contributor

Force-pushed with taking suggestions #26657 (review) and #26657 (review)

in test/functional/test_framework/wallet.py:127 in 979f8fdde1 outdated

123+    def rescan_utxos(self, *, include_mempool=True):
124         """Drop all utxos and rescan the utxo set"""
125         self._utxos = []
126         res = self._test_node.scantxoutset(action="start", scanobjects=[self.get_descriptor()])
127         assert_equal(True, res['success'])
128+        spents = set()

maflcko commented at 1:18 pm on December 15, 2022:

No need for this set if you move the mempool scan logic last

in test/functional/test_framework/wallet.py:131 in 979f8fdde1 outdated

127         assert_equal(True, res['success'])
128+        spents = set()
129+        for txid in self._test_node.getrawmempool():
130+            tx = self._test_node.getrawtransaction(txid=txid, verbose=True)
131+            if include_mempool:
132+                self.scan_tx(tx)

maflcko commented at 1:19 pm on December 15, 2022:

You will need to sort the transactions topologically, see also BlockAssembler::SortForBlock

in test/functional/test_framework/wallet.py:133 in 979f8fdde1 outdated

129+        for txid in self._test_node.getrawmempool():
130+            tx = self._test_node.getrawtransaction(txid=txid, verbose=True)
131+            if include_mempool:
132+                self.scan_tx(tx)
133+            else:
134+                spents.update([(i["txid"], i["vout"]) for i in tx["vin"]])

maflcko commented at 1:20 pm on December 15, 2022:

This will consider the mempool if !include_mempool, which may be confusing

miles170 commented at 1:37 pm on December 15, 2022:

What’s proper way that you might consider reasonable?

Remove the include_mempool argument and just ignore the spent txs in mempool?

maflcko commented at 1:52 pm on December 15, 2022:

No, just remove the else branch

miles170 commented at 1:58 pm on December 15, 2022:

If I remove the else branch, test_sequence_lock_confirmed_inputs will fail because of txn-mempool-conflict. This is why include_mempool was added in the first place.

maflcko commented at 2:00 pm on December 15, 2022:

Even if you move the mempool scan last? See #26657 (review)

miles170 commented at 2:01 pm on December 15, 2022:

You are so right..

miles170 force-pushed on Dec 15, 2022

miles170 commented at 2:56 pm on December 15, 2022: contributor

Force-pushed with taking suggestions #26657 (review) and #26657 (review) and #26657 (review)

in test/functional/feature_bip68_sequence.py:239 in 2512e1a37d outdated

233@@ -247,11 +234,12 @@ def test_sequence_lock_unconfirmed_inputs(self):
234         tx2.nVersion = 2
235         tx2.vin = [CTxIn(COutPoint(tx1.sha256, 0), nSequence=0)]
236         tx2.vout = [CTxOut(int(tx1.vout[0].nValue - self.relayfee * COIN), SCRIPT_W0_SH_OP_TRUE)]
237-        tx2_raw = self.nodes[0].signrawtransactionwithwallet(tx2.serialize().hex())["hex"]
238+        self.wallet.sign_tx(tx=tx2)
239+        tx2_raw = tx2.serialize().hex()
240         tx2 = tx_from_hex(tx2_raw)

achow101 commented at 7:27 pm on January 5, 2023:

In 2512e1a37df42e47a1d2ea63511ba72c2ca8a92f “test: Run feature_bip68_sequence.py with MiniWallet”

This round trip is unnecessary.

test: Add signs P2TR and RAWSCRIPT to MiniWallet sign_tx e5b9127d9e

test: Add "include immature coinbase" flag to MiniWallet get_utxos d0a909ae54

test: Add "include mempool" flag to MiniWallet rescan_utxos fc0caaf4aa

test: Run feature_bip68_sequence.py with MiniWallet 4159ccd031

miles170 commented at 1:53 am on January 6, 2023: contributor

Force-pushed with taking suggestion discussion_r1062820833

miles170 force-pushed on Jan 6, 2023

achow101 commented at 10:46 pm on January 10, 2023: member

ACK 4159ccd03142899028019a7cb44ee4120e68505a

fanquake requested review from maflcko on Jan 11, 2023

in test/functional/feature_bip68_sequence.py:415 in 4159ccd031

417-        rawtx = self.nodes[1].createrawtransaction(inputs, outputs)
418-        rawtxfund = self.nodes[1].fundrawtransaction(rawtx)['hex']
419-        tx = tx_from_hex(rawtxfund)
420+        mini_wallet = MiniWallet(self.nodes[1])
421+        mini_wallet.rescan_utxos()
422+        tx = mini_wallet.create_self_transfer()["tx"]

maflcko commented at 3:25 pm on January 16, 2023:

nit in the last commit (can be fixed in a follow-up):

MiniWallet shouldn’t care about the node, so a single instance should suffice per test.
Maybe the send_self_transfer function can accept a version option?

miles170 commented at 3:46 pm on January 16, 2023:

May I create another PR to refactor those nits?

maflcko commented at 3:48 pm on January 16, 2023:

Yes, if you agree with them and if the changes are possible (haven’t checked myself)

maflcko approved

maflcko commented at 3:25 pm on January 16, 2023: member

Very nice. Thanks!

review ACK 4159ccd03142899028019a7cb44ee4120e68505a 🤸

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 4159ccd03142899028019a7cb44ee4120e68505a 🤸
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUisvgv/YsGnHt+lU5rGQdwFN6eIUrKwK/0pjUAIaDIeOp/61fuwDrJc3MhNE+/j
 83cAiUyvHTtwo3eWkvV9FdQ9V+gVNOoiBWG1yEJtZNoM/syt5uaMi4zUvs2CisODn
 9tlr0jDcMSBw0E0Dc/YEKkwWytysPER5kPQiCleuzUbWqYD/QVX3xUOXGf+a0rttX
10DpuzTcUu5ojaZh4YMEBkgprStIADIdxE2WPb/0eXd1pmwD+TpvnjVA/uRICQAft5
11pBcePzrFLuGqJCOdsg5Fw72JmXJh1RjuBnCiOGiFL/xN02QH4R5MY401RNaXZtYO
12B8USAMS8mO8lJB45t7njTpp1KSgrOJN0RJyEbja6BTROzyi6JttFJVRx4vie2hea
13jICFDu8hNAWrkvXDQXWf/KaUSLOaWITDtYLRdK4bfirNy1PxiFsUjBPkrn0KJgaG
14xI2AaYpNQqV7Krprdm9sEJpO8lev89aL+TEwZv7i2DjZONJArSOEE/Rtmm0nbQEI
152t90dylY
16=isq1
17-----END PGP SIGNATURE-----

maflcko merged this on Jan 16, 2023

maflcko closed this on Jan 16, 2023

miles170 deleted the branch on Jan 16, 2023

sidhujag referenced this in commit 9846ec07b7 on Jan 16, 2023

bitcoin locked this on Apr 4, 2024

test: Run feature_bip68_sequence.py with MiniWallet #26657

Reviews

Conflicts