transaction: Adding script witness to ToString for CTxIn

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 <!–meta-tag:bot-skip–> 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: none

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

0    for (const auto& tx_in : vin)
1        str += "    " + tx_in.ToString() + "\n";
2    for (const auto& tx_in : vin)
3        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: none

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:

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

transaction: Adding script witness to ToString for CTxIn bc02cc8bd2

Ataraxia009 force-pushed on Dec 4, 2025

transaction: Adding script witness to ToString for CTxIn #33711

Code Coverage & Benchmarks

Reviews

Conflicts