rpc: show P2(W)SH redeemScript in getrawtransaction #27637

Riahiamirreza commented at 9:05 am on May 12, 2023: contributor

This a work in progress PR. I’ll squash all my commits at the end to make commits atomic (as mentioned here) It’s an effort to solve this issue #27391.

DrahtBot commented at 9:05 am on May 12, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

DrahtBot added the label RPC/REST/ZMQ on May 12, 2023

pinheadmz commented at 4:49 pm on July 24, 2023: member

@Riahiamirreza are you still working on this?

Riahiamirreza commented at 5:09 pm on July 24, 2023: contributor

@pinheadmz Yes, but I have some difficulties in creating a transaction which spends from a P2SH to be able to view the structure of the transaction.

pinheadmz commented at 6:00 pm on July 24, 2023: member

There are some hints in test/rpc_createmultisig.py and probably a few other tests as well. I also tried to create a P2SH multisig and spend from it manually on regtest, it is quite involved…

https://gist.github.com/pinheadmz/2e85bbdd1b21ed2946f84a88d38f172a

maflcko commented at 4:55 pm on September 20, 2023: member

Are you still working on this?

Riahiamirreza commented at 3:14 pm on September 21, 2023: contributor

@MarcoFalke Actually in the last couple of weeks I didn’t put effort on it. If you think this is not critical and can be delayed more, I need more time to spend on it. I know it’s somewhat a basic issue in the Bitcoin core, but it’s my first issue.

DrahtBot added the label CI failed on Oct 20, 2023

DrahtBot removed the label CI failed on Oct 24, 2023

DrahtBot added the label CI failed on Oct 25, 2023

Riahiamirreza marked this as ready for review on Oct 27, 2023

Riahiamirreza commented at 8:34 pm on October 27, 2023: contributor

I’m fixing tests.

Riahiamirreza marked this as a draft on Oct 27, 2023

in src/core_write.cpp:217 in 1c5004f9af outdated

212+            txin.scriptSig.GetOp(pc, opcode, vch);
213+            CScript script;
214+            std::vector<unsigned char> scriptData(ParseHex(HexStr(vch)));
215+            script = CScript(scriptData.begin(), scriptData.end());
216+            ScriptToUniv(script, /*out=*/redeemScript, /*include_hex=*/false, /*include_address=*/true);
217+            in.pushKV("redeemScrtipt", redeemScript);

Sjors commented at 1:43 am on November 7, 2023:

redeemScrtipt is a typo

Sjors commented at 1:44 am on November 7, 2023: member

Fixing the test is hopefully a matter of just adding the new field(s) to these tests.

Also make sure to squash your commits.

Riahiamirreza commented at 7:09 am on November 7, 2023: contributor

@Sjors Do you have any suggestion to make the code more cleaner? I think it’s not a convenient way to call txin.scriptSig.GetOp(pc, opcode, vch); three times.

Sjors commented at 10:58 am on November 7, 2023: member

I think it’s not a convenient way to call txin.scriptSig.GetOp(pc, opcode, vch); three times.

Indeed it’s not. But I don’t understand what you’re trying to do here.

I would suggest first fixing the tests, so that it’s clear if the code is correct. And then worry about making the code nicer.

Riahiamirreza commented at 3:43 pm on November 9, 2023: contributor

@Sjors For adding new field to a transaction (in my case, adding redeemScript to vin) is it correct to add new fields to CTxIn class in test/functional/test_framework/messages.py? While adding it will cause mismatch in weight calculation of transactions, I think it’s not the correct way. But I’m not sure what’s the correct way.

Riahiamirreza requested review from Sjors on Nov 9, 2023

Riahiamirreza force-pushed on Nov 9, 2023

sipa commented at 1:06 pm on November 13, 2023: member

@Riahiamirreza You cannot modify CTxIn as that’s the actual data structure for transaction inputs. If you change it, all attempts to decode transactions will fail.

Riahiamirreza commented at 1:42 pm on November 13, 2023: contributor

@sipa Yes and I’m not sure how should I fix the problem. Actually the redeemScript is a new field which is not part of the actual data, it’s only a new representation of data (decompiled version of redeemScript). Would you suggest any approach to fix it?

maflcko commented at 1:43 pm on November 13, 2023: member

Would you suggest any approach to fix it?

Fix what? Without any information, there is nothing we can do here.

If you want to fix the tests, make sure to revert the test changes and run the test locally.

sipa commented at 1:47 pm on November 13, 2023: member

I don’t see why you’d need to make changes to the test framework at all.

Riahiamirreza commented at 1:48 pm on November 13, 2023: contributor

@sipa @maflcko I added new field to the output of TxToUniv function, called redeemScript. I’ve manually tested the rpc and that works fine. After running tests I faced issue of decoding transactions. Because of adding a new field, transactions couldn’t be decoded successfully. I attempt to change CTxIn message in my next effort and add the redeemScript which now seems trivially incorrect way.

Riahiamirreza commented at 1:52 pm on November 13, 2023: contributor

@maflcko Sure, I’ll revert my changes on test framework and then try another way to fix tests.

Riahiamirreza force-pushed on Nov 13, 2023

Riahiamirreza commented at 2:50 pm on November 13, 2023: contributor

 0feature_bip68_sequence.py                              | ✖ Failed  | 1 s
 1feature_cltv.py                                        | ✖ Failed  | 1 s
 2feature_coinstatsindex.py                              | ✖ Failed  | 2 s
 3feature_config_args.py                                 | ✖ Failed  | 10 s
 4feature_csv_activation.py                              | ✖ Failed  | 1 s
 5feature_dersig.py                                      | ✖ Failed  | 1 s
 6feature_fee_estimation.py                              | ✖ Failed  | 1 s
 7feature_init.py                                        | ✖ Failed  | 63 s
 8feature_nulldummy.py                                   | ✖ Failed  | 1 s
 9feature_rbf.py                                         | ✖ Failed  | 1 s
10feature_utxo_set_hash.py                               | ✖ Failed  | 1 s
11interface_rest.py                                      | ✖ Failed  | 1 s
12mempool_accept.py                                      | ✖ Failed  | 1 s
13mempool_datacarrier.py                                 | ✖ Failed  | 1 s
14mempool_dust.py                                        | ✖ Failed  | 1 s
15mempool_expiry.py                                      | ✖ Failed  | 1 s
16mempool_limit.py                                       | ✖ Failed  | 1 s
17mempool_package_limits.py                              | ✖ Failed  | 1 s
18mempool_package_onemore.py                             | ✖ Failed  | 1 s
19mempool_packages.py                                    | ✖ Failed  | 1 s
20mempool_persist.py --descriptors                       | ✖ Failed  | 1 s
21mempool_reorg.py                                       | ✖ Failed  | 1 s
22mempool_resurrect.py                                   | ✖ Failed  | 1 s
23mempool_sigoplimit.py                                  | ✖ Failed  | 1 s
24mempool_spend_coinbase.py                              | ✖ Failed  | 1 s
25mempool_unbroadcast.py                                 | ✖ Failed  | 7 s
26mempool_updatefromblock.py                             | ✖ Failed  | 1 s
27mining_basic.py                                        | ✖ Failed  | 12 s
28mining_getblocktemplate_longpoll.py                    | ✖ Failed  | 8 s
29mining_prioritisetransaction.py                        | ✖ Failed  | 1 s
30p2p_blockfilters.py                                    | ✖ Failed  | 491 s
31p2p_compactblocks.py                                   | ✖ Failed  | 2 s
32p2p_feefilter.py                                       | ✖ Failed  | 2 s
33p2p_filter.py                                          | ✖ Failed  | 1 s
34p2p_ibd_txrelay.py                                     | ✖ Failed  | 1 s
35p2p_invalid_messages.py                                | ✖ Failed  | 4 s
36p2p_leak_tx.py                                         | ✖ Failed  | 1 s
37p2p_permissions.py                                     | ✖ Failed  | 8 s
38p2p_segwit.py                                          | ✖ Failed  | 4 s
39rpc_blockchain.py                                      | ✖ Failed  | 14 s
40rpc_createmultisig.py                                  | ✖ Failed  | 1 s
41rpc_decodescript.py                                    | ✖ Failed  | 1 s
42rpc_deriveaddresses.py                                 | ✖ Failed  | 1 s
43rpc_deriveaddresses.py --usecli                        | ✖ Failed  | 1 s
44rpc_dumptxoutset.py                                    | ✖ Failed  | 1 s
45rpc_generate.py                                        | ✖ Failed  | 1 s
46rpc_invalid_address_message.py                         | ✖ Failed  | 1 s
47rpc_mempool_info.py                                    | ✖ Failed  | 1 s
48rpc_misc.py                                            | ✖ Failed  | 1 s
49rpc_net.py                                             | ✖ Failed  | 1 s
50rpc_packages.py                                        | ✖ Failed  | 1 s
51rpc_rawtransaction.py --legacy-wallet                  | ✖ Failed  | 1 s
52rpc_scanblocks.py                                      | ✖ Failed  | 1 s
53rpc_scantxoutset.py                                    | ✖ Failed  | 1 s
54rpc_txoutproof.py                                      | ✖ Failed  | 1 s
55
56ALL                                                    | ✖ Failed  | 1165 s (accumulated)

These are the currently failing tests.

in src/core_write.cpp:205 in 753e0074b4 outdated

201@@ -202,6 +202,21 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry
202             o.pushKV("asm", ScriptToAsmStr(txin.scriptSig, true));
203             o.pushKV("hex", HexStr(txin.scriptSig));
204             in.pushKV("scriptSig", o);
205+            if (txin.scriptSig.IsPayToScriptHash()) {

sipa commented at 2:54 pm on November 13, 2023:

I don’t think this is correct. P2SH spends are identified by the spent scriptPubKey having the right form. This will, I suspect, not have any effect; the scriptSig is generally push-only (and non-standard otherwise), so it’ll never match the P2SH template.

Riahiamirreza commented at 5:48 pm on November 13, 2023:

@sipa How can I access the CTxout from the COutPoint? For a given CTxIn I want to reach the scriptPubKey of its output. The COutPoint only has transaction hash and index of that specific output in the vout. I don’t know how can I reach a transaction output using COutPoint. Should I use CCoinsViewDB::GetCoin? Does it contains all the transactions? (not only mempool transactions)

0    bool GetCoin(const COutPoint &outpoint, Coin &coin) const override;

sipa commented at 5:51 pm on November 13, 2023:

You can’t use the UTXO set for this as I pointed out elsewhere. The spent UTXO is however in the undo data that’s passed to TxToUniv.

Riahiamirreza commented at 6:27 pm on November 13, 2023:

@sipa The undo data contains Coins, which only have amount and scriptPubKey. The Txins’ outpoint have tx hash and index of vout. My problem is how to identify which one of Coins in the undo data match to a given txin’s prevout.

sipa commented at 6:30 pm on November 13, 2023:

txundo->vprevout[txin.prevout.n].scriptPubKey is the scriptPubKey you want.

maflcko commented at 2:54 pm on November 13, 2023: member

These are the currently failing tests.

Failing why? Without any information, there is nothing we can do here.

Recall that the macOS CI passes on this pull request currently.

This looks like a local problem on your side.

in src/core_write.cpp:213 in 753e0074b4 outdated

208+                std::vector<unsigned char> vch;
209+                CScript::const_iterator pc = txin.scriptSig.begin();
210+                txin.scriptSig.GetOp(pc, opcode, vch);
211+                txin.scriptSig.GetOp(pc, opcode, vch);
212+                txin.scriptSig.GetOp(pc, opcode, vch);
213+                txin.scriptSig.GetOp(pc, opcode, vch);

sipa commented at 2:56 pm on November 13, 2023:

This code seems to assume that the 4th scriptSig push is the redeemScript. That won’t always be correct; it’s the last one.

Riahiamirreza commented at 3:07 pm on November 13, 2023:

So can I always expect the last scriptSig push to be the redeemScript if it has one?

sipa commented at 3:09 pm on November 13, 2023:

Yes, BIP16 states that the redeemScript is the last push of the scriptSig.

Riahiamirreza commented at 5:07 pm on November 13, 2023:

@sipa Do you think this is a proper way of accessing the last push of the scriptSig? Or there are utilities in the codebase to do this for me?

0            while (true) {
1                if (!txin.scriptSig.GetOp(pc, opcode, vch))
2                    break;
3            }

Riahiamirreza commented at 3:00 pm on November 13, 2023: contributor

Failing why? Without any information, there is nothing we can do here.

As the exception suggests it’s due the addition of the new field redeemScript. Am I right?

 0{a.riahi@bitcoin} > python3.10 ./test/functional/rpc_generate.py
 12023-11-13T14:57:56.863000Z TestFramework (INFO): PRNG seed is: 2176820945159495690
 22023-11-13T14:57:56.863000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_nt1lv9on
 32023-11-13T14:57:57.137000Z TestFramework (INFO): Test rpc generate raises with message to use cli option
 42023-11-13T14:57:57.138000Z TestFramework (INFO): Test rpc generate help prints message to use cli option
 52023-11-13T14:57:57.138000Z TestFramework (INFO): Test rpc generate is a hidden command not discoverable in general help
 62023-11-13T14:57:57.234000Z TestFramework (INFO): Mine an empty block to address and return the hex
 72023-11-13T14:57:57.237000Z TestFramework (INFO): Generate an empty block to address
 82023-11-13T14:57:57.241000Z TestFramework (INFO): Generate an empty block to a descriptor
 92023-11-13T14:57:57.244000Z TestFramework (INFO): Generate an empty block to a combo descriptor with compressed pubkey
102023-11-13T14:57:57.247000Z TestFramework (INFO): Generate an empty block to a combo descriptor with uncompressed pubkey
112023-11-13T14:57:57.251000Z TestFramework (ERROR): JSONRPC error
12Traceback (most recent call last):
13  File "/home/a.riahi@asax.local/github_repos/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
14    self.run_test()
15  File "/home/a.riahi@asax.local/github_repos/bitcoin/./test/functional/rpc_generate.py", line 22, in run_test
16    self.test_generateblock()
17  File "/home/a.riahi@asax.local/github_repos/bitcoin/./test/functional/rpc_generate.py", line 68, in test_generateblock
18    miniwallet.send_self_transfer(from_node=node)
19  File "/home/a.riahi@asax.local/github_repos/bitcoin/test/functional/test_framework/wallet.py", line 247, in send_self_transfer
20    self.sendrawtransaction(from_node=from_node, tx_hex=tx['hex'])
21  File "/home/a.riahi@asax.local/github_repos/bitcoin/test/functional/test_framework/wallet.py", line 355, in sendrawtransaction
22    self.scan_tx(from_node.decoderawtransaction(tx_hex))
23  File "/home/a.riahi@asax.local/github_repos/bitcoin/test/functional/test_framework/coverage.py", line 49, in __call__
24    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
25  File "/home/a.riahi@asax.local/github_repos/bitcoin/test/functional/test_framework/authproxy.py", line 126, in __call__
26    raise JSONRPCException(response['error'], status)
27test_framework.authproxy.JSONRPCException: Internal bug detected: RPC call "decoderawtransaction" returned incorrect type:
28{
29    "vin": {
30        "0": {
31            "redeemScript": "key returned that was not in doc"
32        }
33    }
34}
35Bitcoin Core v26.99.0-8f567b65a345-dirty
36Please report this issue here: https://github.com/bitcoin/bitcoin/issues
37 (-1)

sipa commented at 3:10 pm on November 13, 2023: member

@Riahiamirreza To fix that error you’ll need to add your new field to the documentation of the affected RPCs.

sipa commented at 3:35 pm on November 13, 2023: member

To be clear: this cannot just be implemented inside TxToUniv. It needs access to the UTXOs being spent by a transaction. I’m not sure what the right approach is, but it’s not a trivial change.

EDIT: oh, actually, when running in non-pruned mode, the UTXO information is available to it through the undo data provided to it. That would allow detecting P2SH.

Riahiamirreza commented at 4:03 pm on November 13, 2023: contributor

@sipa As far as I understand having the information of UTXOs has nothing to do with pruned mode. Don’t pruned nodes store the whole UTXO set? (I can guess the problem can occur for pruned mode when a reorg happen and the old data to restore is pruned, but I’m not sure this is the case)

sipa commented at 4:06 pm on November 13, 2023: member

@Riahiamirreza Actually, ignore what I said about the UTXO set; that’s irrelevant. getrawtransaction works for confirmed transactions, and the UTXOs such transactions have spent are no longer unspent, and thus no longer in the UTXO set.

You need access to the spentness information, and that is only available on non-pruned nodes. Thankfully, TxToUniv already gets that information (through the undo structure passed to it).

DrahtBot removed the label CI failed on Nov 13, 2023

Riahiamirreza force-pushed on Nov 14, 2023

DrahtBot added the label CI failed on Nov 14, 2023

add decompiled redeemScript to TxToUniv result 65b7634976

Riahiamirreza force-pushed on Nov 14, 2023

Riahiamirreza requested review from sipa on Nov 14, 2023

in src/rpc/rawtransaction.cpp:117 in 65b7634976

113@@ -114,6 +114,12 @@ static std::vector<RPCResult> DecodeTxDoc(const std::string& txid_field_doc)
114                     {RPCResult::Type::STR, "asm", "Disassembly of the signature script"},
115                     {RPCResult::Type::STR_HEX, "hex", "The raw signature script bytes, hex-encoded"},
116                 }},
117+                {RPCResult::Type::OBJ, "redeemScript", /*optional=*/true, "",

Riahiamirreza commented at 9:22 am on November 14, 2023:

I’ll add description to description fields later, for now I just wanted to make sure that changing DecodeTxDoc wasn’t wrong.

DrahtBot removed the label CI failed on Nov 14, 2023

Riahiamirreza commented at 4:44 pm on November 15, 2023: contributor

I’m working on adding tests, but I’m not sure where is the correct place for putting tests. Should I put tests in RawTransactionsTest.getrawtransaction_verbositry_tests in the tests/functional/rpc_rawtransaction.py? I need to have a transaction of type P2SH in the chain and check whether the result has the correct redeemScript field? Is this approach correct?

Riahiamirreza commented at 5:36 pm on December 9, 2023: contributor

@sipa Would you please review my PR? I really appreciate it.

DrahtBot added the label CI failed on Jan 13, 2024

Riahiamirreza marked this as ready for review on Feb 22, 2024

DrahtBot commented at 2:03 am on May 22, 2024: contributor

🤔 There hasn’t been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn’t been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

DrahtBot added the label Needs rebase on May 23, 2024

DrahtBot commented at 7:02 pm on May 23, 2024: contributor

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

fanquake marked this as a draft on Jun 25, 2024

rpc: show P2(W)SH redeemScript in getrawtransaction #27637 #27638

Code Coverage

Reviews