rpc: Correctly compute redeemScript from witnessScript for signrawtransaction

achow101 commented at 5:45 pm on March 31, 2020: member

ParsePrevouts uses GetScriptForWitness on the given witnessScript to find the corresponding redeemScript. This is incorrect when the witnessScript is either a P2PK or P2PKH script as it returns the corresponding P2WPK script instead of turning the witnessScript into a P2WSH script. Instead this should make the script a WitnessV0ScriptHash destination and get the script for that.

Test cases are also added. These will fail on master with a redeemScript does not correspond to witnessScript

Reported on Bitcointalk

sipa commented at 6:07 pm on March 31, 2020: member

utACK c0f9a61793ffccb6af97ffb6b4a156780e1a4597

After this it seems that GetScriptForWitness is only used in bitcoin-tx (all in places were more explicit constructions of scripts would be preferable) and in tests. Maybe it’s time (in a different PR) to get rid of it entirely.

in test/functional/rpc_signrawtransaction.py:177 in c0f9a61793 outdated

169@@ -168,6 +170,44 @@ def witness_script_test(self):
170         assert 'complete' in spending_tx_signed
171         assert_equal(spending_tx_signed['complete'], True)
172 
173+        # Now try with a P2PKH script as the witnessScript
174+        embedded_addr_info = self.nodes[1].getaddressinfo(self.nodes[1].getnewaddress('', 'legacy'))
175+        embedded_privkey = self.nodes[1].dumpprivkey(embedded_addr_info['address'])
176+        witness_script = embedded_addr_info['scriptPubKey']
177+        redeem_script = hexlify(CScript([OP_0, sha256(check_script(witness_script))])).decode()

MarcoFalke commented at 6:26 pm on March 31, 2020:

nit. I think this might work:

0        redeem_script = CScript([OP_0, sha256(check_script(witness_script))]).hex()

achow101 commented at 7:04 pm on March 31, 2020:

Done

MarcoFalke renamed this:
~~Correctly compute redeemScript from witnessScript for signrawtransaction~~
rpc: Correctly compute redeemScript from witnessScript for signrawtransaction
on Mar 31, 2020

MarcoFalke added the label RPC/REST/ZMQ on Mar 31, 2020

achow101 force-pushed on Mar 31, 2020

Correctly compute redeemScript from witnessScript for signrawtransaction

ParsePrevouts uses GetScriptForWitness on the given witnessScript
to find the corresponding redeemScript. This is incorrect when the
witnessScript is either a P2PK or P2PKH script as it returns the
corresponding P2WPK script instead of turning the witnessScript
into a P2WSH script. Instead this should make the script a
WitnessV0ScriptHash destination and get the script for that.

Test cases are also added.

cd3b1569d9

achow101 force-pushed on Mar 31, 2020

MarcoFalke added this to the milestone 0.20.0 on Apr 3, 2020

MarcoFalke commented at 1:33 am on April 3, 2020: member

weak ACK cd3b1569d9, only checked that the test fails without the code change 🚰

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3weak ACK cd3b1569d9, only checked that the test fails without the code change 🚰
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjCbwwAsp2heFGb1866a8x98h5pUos0kinxV/wI5wCnXhsD698tiLJMjYSr0q6U
 8Y5GZwCHJLQTgAwI+SB7j4c/YZONMro+sspVT6RzeBNEmZ+sGRTfpDSDJ8ia95zuZ
 9lKbSab1ClG7IgmuLpxOag2twuwtY48RT8HOcC1xcMaipDylBdM2uRrFHKJE7CW6H
10UDaFGunaLDJDqjWjYG7Y62G1u2IF4nNQu/8FqoyMOx0gXTDQo5pv/M6HYDD0JYZV
11fNC2clH/wcaqY2dP0khpBTrBoOh8V/1VPswc9VgW7/TEI36c8Vk0KZ2u8DbKY9e9
123PSwN6gE6U6VgDfTLclF52lC+OR0snp8meIgziPey9IhPXZITNRFVaE963EtjVed
13uOI4T20GU8f7tIOw4om7Y7dVRMY+z08VfksdhppwgdMwSwW2CIeRl+KByBRmUbGw
14aRGLVysoFFI26MgI37KyLGIptLGC/zPPgJHxvS+l6X6Ofhp0RxyyTyrDycc6yiQi
15HH6jdJlB
16=HXml
17-----END PGP SIGNATURE-----

Timestamp of file with hash 881a6eae6982f18b4068be3f58ea3caa8e81d392053eea501c317ee5ae66cb93 -

MarcoFalke commented at 1:34 am on April 3, 2020: member

Appveyor failure in signrawtransaction: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/31864368

Open-Close to re-run ci. See #15847 (comment)

MarcoFalke closed this on Apr 3, 2020

MarcoFalke reopened this on Apr 3, 2020

instagibbs commented at 4:41 pm on April 6, 2020: member

utACK https://github.com/bitcoin/bitcoin/pull/18484/commits/cd3b1569d9ad8e24d3a222aff74e0c254baadf79

MarcoFalke merged this on Apr 6, 2020

MarcoFalke closed this on Apr 6, 2020

in test/functional/rpc_signrawtransaction.py:185 in cd3b1569d9

180+        txid = self.nodes[0].sendtoaddress(addr, 10)
181+        vout = find_vout_for_address(self.nodes[0], txid, addr)
182+        self.nodes[0].generate(1)
183+        # Now create and sign a transaction spending that output on node[0], which doesn't know the scripts or keys
184+        spending_tx = self.nodes[0].createrawtransaction([{'txid': txid, 'vout': vout}], {self.nodes[1].getnewaddress(): Decimal("9.999")})
185+        spending_tx_signed = self.nodes[0].signrawtransactionwithkey(spending_tx, [embedded_privkey], [{'txid': txid, 'vout': vout, 'scriptPubKey': script_pub_key, 'redeemScript': redeem_script, 'witnessScript': witness_script, 'amount': 10}])

jonatack commented at 5:24 pm on April 6, 2020:

verified that reverting the change in rawtransaction_util.cpp::219 causes the test to fail here with redeemScript does not correspond to witnessScript (-8)

jonatack commented at 5:30 pm on April 6, 2020: member

ACK cd3b1569d9ad8e

It looks like we are close to being able to remove GetScriptForWitness as per:

0src/script/standard.h:202: * TODO: replace calls to GetScriptForWitness with GetScriptForDestination using the various witness-specific CTxDestination subtypes.

sidhujag referenced this in commit 7443175d86 on Apr 8, 2020

MarcoFalke referenced this in commit e28e5353c4 on Apr 13, 2020

Iceymann18777 commented at 8:16 am on September 24, 2021: none

.

MarcoFalke locked this on Sep 24, 2021

rpc: Correctly compute redeemScript from witnessScript for signrawtransaction #18484