Merge bitcoin/bitcoin#27200: test: psbt: check non-witness UTXO removal for segwit v1 input

3dd2f64 test: psbt: check non-witness UTXO removal for segwit v1 input (Sebastian Falbesoner)
dd78e3f test: speedup rpc_psbt.py by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
e194e3e test: PSBT: eliminate magic numbers for global unsigned tx key (0) (Sebastian Falbesoner)

Pull request description:

  This PR adds missing test coverage for dropping non-witness UTXOs from PSBTs for segwit v1+ inputs (see commit 103c6fd). The formerly [disabled](bitcoin/bitcoin@4600479) method `test_utxo_conversion` is re-enabled and adapted to spend a Taproot (`bech32m`) instead of a wrapped SegWit (`p2sh-segwit`) output. Note that in contrast to the original test, we have to add the non-witness UTXO manually here using the test framework's PSBT module, since the constructing node knows that the output is segwit v1 and hence doesn't add the non-witness UTXO in the first place (see also [BIP371]( https://github.com/bitcoin/bips/blob/master/bip-0371.mediawiki#user-content-UTXO_Types)).

  I strongly assume that most wallets would behave the same as Bitcoin Core here and wouldn't create PSBTs with non-witness UTXOs for Taproot inputs, but it's still good to test everything works as expected if it's still done and that the non-witness UTXO is simply dropped in that case.

  The first two commits contain a small refactor (magic number elimination in PSBT module) and test speedup of ~2-3x (using whitelisting peers / immediate tx relay).

ACKs for top commit:
  achow101:
    ACK 3dd2f64
  instagibbs:
    ACK 3dd2f64

Tree-SHA512: b8d7f7ea5d7d21def024b70dfca61991cc96a4193be8857018b4d7cf3ca1465d185619fd4a77623803d9da309aa489c53273e9b7683d970ce12e2399b5b50031

Author: achow101

test/functional/rpc_psbt.py

@@ -4,7 +4,6 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test the Partially Signed Transaction RPCs.
 """
-
 from decimal import Decimal
 from itertools import product
 
@@ -27,6 +26,7 @@
     PSBT_IN_SHA256,
     PSBT_IN_HASH160,
     PSBT_IN_HASH256,
+    PSBT_IN_NON_WITNESS_UTXO,
     PSBT_IN_WITNESS_UTXO,
     PSBT_OUT_TAP_TREE,
 )
@@ -59,13 +59,16 @@ def set_test_params(self):
             ["-walletrbf=0", "-changetype=legacy"],
             []
         ]
+        # whitelist peers to speed up tx relay / mempool sync
+        for args in self.extra_args:
+            args.append("[email protected]")
         self.supports_cli = False
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
 
-    # TODO: Re-enable this test with segwit v1
     def test_utxo_conversion(self):
+        self.log.info("Check that non-witness UTXOs are removed for segwit v1+ inputs")
         mining_node = self.nodes[2]
         offline_node = self.nodes[0]
         online_node = self.nodes[1]
@@ -77,34 +80,41 @@ def test_utxo_conversion(self):
         # Create watchonly on online_node
         online_node.createwallet(wallet_name='wonline', disable_private_keys=True)
         wonline = online_node.get_wallet_rpc('wonline')
-        w2 = online_node.get_wallet_rpc('')
+        w2 = online_node.get_wallet_rpc(self.default_wallet_name)
 
         # Mine a transaction that credits the offline address
-        offline_addr = offline_node.getnewaddress(address_type="p2sh-segwit")
-        online_addr = w2.getnewaddress(address_type="p2sh-segwit")
+        offline_addr = offline_node.getnewaddress(address_type="bech32m")
+        online_addr = w2.getnewaddress(address_type="bech32m")
         wonline.importaddress(offline_addr, "", False)
-        mining_node.sendtoaddress(address=offline_addr, amount=1.0)
-        self.generate(mining_node, nblocks=1)
+        mining_wallet = mining_node.get_wallet_rpc(self.default_wallet_name)
+        mining_wallet.sendtoaddress(address=offline_addr, amount=1.0)
+        self.generate(mining_node, nblocks=1, sync_fun=lambda: self.sync_all([online_node, mining_node]))
 
-        # Construct an unsigned PSBT on the online node (who doesn't know the output is Segwit, so will include a non-witness UTXO)
+        # Construct an unsigned PSBT on the online node
         utxos = wonline.listunspent(addresses=[offline_addr])
         raw = wonline.createrawtransaction([{"txid":utxos[0]["txid"], "vout":utxos[0]["vout"]}],[{online_addr:0.9999}])
         psbt = wonline.walletprocesspsbt(online_node.converttopsbt(raw))["psbt"]
-        assert "non_witness_utxo" in mining_node.decodepsbt(psbt)["inputs"][0]
+        assert not "not_witness_utxo" in mining_node.decodepsbt(psbt)["inputs"][0]
+
+        # add non-witness UTXO manually
+        psbt_new = PSBT.from_base64(psbt)
+        prev_tx = wonline.gettransaction(utxos[0]["txid"])["hex"]
+        psbt_new.i[0].map[PSBT_IN_NON_WITNESS_UTXO] = bytes.fromhex(prev_tx)
+        assert "non_witness_utxo" in mining_node.decodepsbt(psbt_new.to_base64())["inputs"][0]
 
-        # Have the offline node sign the PSBT (which will update the UTXO to segwit)
-        signed_psbt = offline_node.walletprocesspsbt(psbt)["psbt"]
-        assert "witness_utxo" in mining_node.decodepsbt(signed_psbt)["inputs"][0]
+        # Have the offline node sign the PSBT (which will remove the non-witness UTXO)
+        signed_psbt = offline_node.walletprocesspsbt(psbt_new.to_base64())["psbt"]
+        assert not "non_witness_utxo" in mining_node.decodepsbt(signed_psbt)["inputs"][0]
 
         # Make sure we can mine the resulting transaction
         txid = mining_node.sendrawtransaction(mining_node.finalizepsbt(signed_psbt)["hex"])
-        self.generate(mining_node, 1)
+        self.generate(mining_node, nblocks=1, sync_fun=lambda: self.sync_all([online_node, mining_node]))
         assert_equal(online_node.gettxout(txid,0)["confirmations"], 1)
 
         wonline.unloadwallet()
 
         # Reconnect
-        self.connect_nodes(0, 1)
+        self.connect_nodes(1, 0)
         self.connect_nodes(0, 2)
 
     def test_input_confs_control(self):
@@ -571,8 +581,8 @@ def run_test(self):
         for i, signer in enumerate(signers):
             self.nodes[2].unloadwallet("wallet{}".format(i))
 
-        # TODO: Re-enable this for segwit v1
-        # self.test_utxo_conversion()
+        if self.options.descriptors:
+            self.test_utxo_conversion()
 
         self.test_input_confs_control()
 

test/functional/test_framework/psbt.py

@@ -105,8 +105,8 @@ def __init__(self, *, g=None, i=None, o=None):
     def deserialize(self, f):
         assert f.read(5) == b"psbt\xff"
         self.g = from_binary(PSBTMap, f)
-        assert 0 in self.g.map
-        self.tx = from_binary(CTransaction, self.g.map[0])
+        assert PSBT_GLOBAL_UNSIGNED_TX in self.g.map
+        self.tx = from_binary(CTransaction, self.g.map[PSBT_GLOBAL_UNSIGNED_TX])
         self.i = [from_binary(PSBTMap, f) for _ in self.tx.vin]
         self.o = [from_binary(PSBTMap, f) for _ in self.tx.vout]
         return self
@@ -115,8 +115,8 @@ def serialize(self):
         assert isinstance(self.g, PSBTMap)
         assert isinstance(self.i, list) and all(isinstance(x, PSBTMap) for x in self.i)
         assert isinstance(self.o, list) and all(isinstance(x, PSBTMap) for x in self.o)
-        assert 0 in self.g.map
-        tx = from_binary(CTransaction, self.g.map[0])
+        assert PSBT_GLOBAL_UNSIGNED_TX in self.g.map
+        tx = from_binary(CTransaction, self.g.map[PSBT_GLOBAL_UNSIGNED_TX])
         assert len(tx.vin) == len(self.i)
         assert len(tx.vout) == len(self.o)
 
@@ -130,7 +130,7 @@ def make_blank(self):
         for m in self.i + self.o:
             m.map.clear()
 
-        self.g = PSBTMap(map={0: self.g.map[0]})
+        self.g = PSBTMap(map={PSBT_GLOBAL_UNSIGNED_TX: self.g.map[PSBT_GLOBAL_UNSIGNED_TX]})
 
     def to_base64(self):
         return base64.b64encode(self.serialize()).decode("utf8")