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.

@Riahiamirreza are you still working on this?

@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.

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

Are you still working on this?

@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.

I'm fixing tests.

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

Also make sure to squash your commits.

@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.

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.

@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 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.

@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?

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.

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

@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.

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

feature_bip68_sequence.py                              | ✖ Failed  | 1 s
feature_cltv.py                                        | ✖ Failed  | 1 s
feature_coinstatsindex.py                              | ✖ Failed  | 2 s
feature_config_args.py                                 | ✖ Failed  | 10 s
feature_csv_activation.py                              | ✖ Failed  | 1 s
feature_dersig.py                                      | ✖ Failed  | 1 s
feature_fee_estimation.py                              | ✖ Failed  | 1 s
feature_init.py                                        | ✖ Failed  | 63 s
feature_nulldummy.py                                   | ✖ Failed  | 1 s
feature_rbf.py                                         | ✖ Failed  | 1 s
feature_utxo_set_hash.py                               | ✖ Failed  | 1 s
interface_rest.py                                      | ✖ Failed  | 1 s
mempool_accept.py                                      | ✖ Failed  | 1 s
mempool_datacarrier.py                                 | ✖ Failed  | 1 s
mempool_dust.py                                        | ✖ Failed  | 1 s
mempool_expiry.py                                      | ✖ Failed  | 1 s
mempool_limit.py                                       | ✖ Failed  | 1 s
mempool_package_limits.py                              | ✖ Failed  | 1 s
mempool_package_onemore.py                             | ✖ Failed  | 1 s
mempool_packages.py                                    | ✖ Failed  | 1 s
mempool_persist.py --descriptors                       | ✖ Failed  | 1 s
mempool_reorg.py                                       | ✖ Failed  | 1 s
mempool_resurrect.py                                   | ✖ Failed  | 1 s
mempool_sigoplimit.py                                  | ✖ Failed  | 1 s
mempool_spend_coinbase.py                              | ✖ Failed  | 1 s
mempool_unbroadcast.py                                 | ✖ Failed  | 7 s
mempool_updatefromblock.py                             | ✖ Failed  | 1 s
mining_basic.py                                        | ✖ Failed  | 12 s
mining_getblocktemplate_longpoll.py                    | ✖ Failed  | 8 s
mining_prioritisetransaction.py                        | ✖ Failed  | 1 s
p2p_blockfilters.py                                    | ✖ Failed  | 491 s
p2p_compactblocks.py                                   | ✖ Failed  | 2 s
p2p_feefilter.py                                       | ✖ Failed  | 2 s
p2p_filter.py                                          | ✖ Failed  | 1 s
p2p_ibd_txrelay.py                                     | ✖ Failed  | 1 s
p2p_invalid_messages.py                                | ✖ Failed  | 4 s
p2p_leak_tx.py                                         | ✖ Failed  | 1 s
p2p_permissions.py                                     | ✖ Failed  | 8 s
p2p_segwit.py                                          | ✖ Failed  | 4 s
rpc_blockchain.py                                      | ✖ Failed  | 14 s
rpc_createmultisig.py                                  | ✖ Failed  | 1 s
rpc_decodescript.py                                    | ✖ Failed  | 1 s
rpc_deriveaddresses.py                                 | ✖ Failed  | 1 s
rpc_deriveaddresses.py --usecli                        | ✖ Failed  | 1 s
rpc_dumptxoutset.py                                    | ✖ Failed  | 1 s
rpc_generate.py                                        | ✖ Failed  | 1 s
rpc_invalid_address_message.py                         | ✖ Failed  | 1 s
rpc_mempool_info.py                                    | ✖ Failed  | 1 s
rpc_misc.py                                            | ✖ Failed  | 1 s
rpc_net.py                                             | ✖ Failed  | 1 s
rpc_packages.py                                        | ✖ Failed  | 1 s
rpc_rawtransaction.py --legacy-wallet                  | ✖ Failed  | 1 s
rpc_scanblocks.py                                      | ✖ Failed  | 1 s
rpc_scantxoutset.py                                    | ✖ Failed  | 1 s
rpc_txoutproof.py                                      | ✖ Failed  | 1 s

ALL                                                    | ✖ Failed  | 1165 s (accumulated)

These are the currently failing tests.

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.

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?

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

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

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.

@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)

@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).

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?

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

🤔 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.

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

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

• Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
• Is it no longer relevant? ➡️ Please close.
• Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

Closing for now due to inactivity. You can leave a comment below to have this reopened. Also, a new pull request can be opened.

@maflcko Would you please review the PR if it worth? If you think it does not have enough quality or is out of priority just ignore my comment.

You'll have to put in some effort to create a pull request that is reviewable. It is fine to have a draft pull request, while you work on it, or ask specific questions about the implementation. However, it is unlikely that someone is going to review an incomplete draft pull request that needs rebase.

@maflcko You're right, I didn't put much effort on it. Do you think the issue is still relevant?