transaction: Adding script witness to ToString for CTxIn #33711

Ataraxia009 commented at 5:19 AM on October 27, 2025: none

When debugging and trying to print the details of a CTxIn using the ToString, we get the scriptSig, but would also help to get the witness.

DrahtBot commented at 5:19 AM on October 27, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33711.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	optout21

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33771 (refactor: C++20 operators by purpleKarrot)

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.

optout21 commented at 5:42 AM on November 1, 2025: contributor

Concept ACK 61cd0e53ca45684cd5ef9084829ecc6d59154595

I notice that CTransaction::ToString() prints the inputs and the input script witnesses additionally, with that change script witnesses will be included duplicated. The order there is first all inputs, then all witnesses. Please consider removing the duplication. Options:

keep the duplication
remove duplication, change the order
remove the duplication and keep the order -- there should be a version of CTxIn::ToString() without witness?

https://github.com/bitcoin/bitcoin/blob/master/src/primitives/transaction.cpp#L126

    for (const auto& tx_in : vin)
        str += "    " + tx_in.ToString() + "\n";
    for (const auto& tx_in : vin)
        str += "    " + tx_in.scriptWitness.ToString() + "\n";

Ataraxia009 force-pushed on Nov 7, 2025

Ataraxia009 commented at 6:19 AM on November 7, 2025: none

I notice that CTransaction::ToString() prints the inputs and the input script witnesses additionally, with that change script witnesses will be included duplicated. The order there is first all inputs, then all witnesses. Please consider removing the duplication.

Hey, good call out. Updated to remove duplicate. Think this makes the most sense.

in src/primitives/transaction.cpp:49 in e8b262a91d

  43 | @@ -44,8 +44,10 @@ std::string CTxIn::ToString() const
  44 |      str += prevout.ToString();
  45 |      if (prevout.IsNull())
  46 |          str += strprintf(", coinbase %s", HexStr(scriptSig));
  47 | -    else
  48 | +    else {
  49 |          str += strprintf(", scriptSig=%s", HexStr(scriptSig).substr(0, 24));
  50 | +        str += strprintf(", scriptWitness=%s", scriptWitness.ToString());

frankomosh commented at 9:28 AM on November 12, 2025:

I think there might be an inconsistency in formatting here. scriptWitness.ToString() is called directly, while scriptSig uses HexStr().substr(0, 24). Shouldn't there be some symmetry ?

Ataraxia009 commented at 6:13 AM on November 14, 2025:

I see your point, but the underlying structures are different for ScriptSig and ScriptWitness: simple byte vector for script sig ScriptSig and vector of byte vectors for the ScriptWitness. So these are more accurate representatives of what is happening under the hood imo. Its also standard to print script sig like this from what i can see.

in src/primitives/transaction.cpp:126 in e8b262a91d outdated

 124 | @@ -123,8 +125,6 @@ std::string CTransaction::ToString() const
 125 |          nLockTime);
 126 |      for (const auto& tx_in : vin)
 127 |          str += "    " + tx_in.ToString() + "\n";
 128 | -    for (const auto& tx_in : vin)

frankomosh commented at 9:40 AM on November 12, 2025:

Though I have not done a thorough test, I think that completely removing this might affect debug workflow. Before removing it, Ctransaction::ToString() is able to print all inputs, then a separate block with all witnesses, then all outputs. I don’t think this is possible anymore.

Ataraxia009 commented at 6:14 AM on November 14, 2025:

yes but its better to roll up responsibility to the object that holds it? Can you point out exactly where maybe? so we can see? if not, this seems like a better approach to me.

optout21 commented at 9:15 AM on November 14, 2025:

To be safe, I suggest keeping the output here as it was, as I have no idea what was the rationale for it: just so happened, or to reflect that segwit segregation. A way to do it is to have a version of Tx tostring that omits the witness, and use that here, and print the witnesses separately as before. I think this is less intrusive change and higher change of positive review.

frankomosh commented at 8:34 AM on November 17, 2025:

yes but its better to roll up responsibility to the object that holds it? Can you point out exactly where maybe? so we can see? if not, this seems like a better approach to me.

I think rolling the witness into CTxIn::ToString() is fine only if you keep the old separate witness block in CTransaction::ToString() (or try adding a flag to suppress it). Right now you’ve deleted that block, so every existing log, test and user script would lose the witness section.

Ataraxia009 commented at 11:51 AM on November 17, 2025:

It wouldn't lose the witness section @frankomosh it would just be rolled up. It would still print, it would just print in the txIn instead. However i can breaking such a fundamental log would be a problem so I'm okay with making the change less intrusive

Ataraxia009 commented at 7:34 PM on November 24, 2025:

@frankomosh should be addressed

Ataraxia009 commented at 7:35 PM on November 24, 2025:

@optout21 for vision

optout21 commented at 11:10 AM on November 27, 2025:

The CTransaction::ToString() output looks the same now, my concern is addressed.

frankomosh commented at 9:47 AM on November 12, 2025: contributor

I think the goal for this is reasonable, but doesn't it remove witness data from CTransaction::ToString() and break existing debug output?

Ataraxia009 force-pushed on Nov 24, 2025

optout21 commented at 11:25 AM on November 27, 2025: contributor

utACK 812495a426a3ea9ef15567983ff31e22d05e9f5d

Minor extension to the ToString() method of the CTxIn class. Well localized change, minimal risk of unintended impact. Thanks for the initiative.

in src/primitives/transaction.cpp:129 in 812495a426

 125 | @@ -122,7 +126,7 @@ std::string CTransaction::ToString() const
 126 |          vout.size(),
 127 |          nLockTime);
 128 |      for (const auto& tx_in : vin)
 129 | -        str += "    " + tx_in.ToString() + "\n";
 130 | +        str += "    " + tx_in.ToString(false) + "\n";

maflcko commented at 1:47 PM on November 27, 2025:

pls use named args and snake_case:

        str += "    " + tx_in.ToString(/*include_witness=*/false) + "\n";

transaction: Adding script witness to ToString for CTxIn bc02cc8bd2

Ataraxia009 force-pushed on Dec 4, 2025

DrahtBot added the label Needs rebase on Dec 8, 2025

DrahtBot commented at 5:45 PM on December 8, 2025: contributor

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

DrahtBot added the label CI failed on Dec 15, 2025

DrahtBot commented at 4:17 PM on December 15, 2025: contributor

🚧 At least one of the CI tasks failed. <sub>Task lint: https://github.com/bitcoin/bitcoin/actions/runs/19918111499/job/58081204709</sub> <sub>LLM reason (✨ experimental): Lint check failed due to trailing whitespace in src/primitives/transaction.cpp:50, causing the CI to fail.</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

sedited commented at 12:57 PM on March 4, 2026: contributor

This has not seen activity in a while and needs a rebase. I'm also not sure what the scenario is where debugging is improved with this addition.

fanquake marked this as a draft on Mar 4, 2026

maflcko commented at 10:59 AM on March 19, 2026: member

Closing for now, due to inactivity

maflcko closed this on Mar 19, 2026