transaction: Adding script witness to ToString for CTxIn #33711

Ataraxia009 · 2025-10-27T05:19:54Z

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 · 2025-10-27T05:19:59Z

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  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 · 2025-11-01T05:42:38Z

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 · 2025-11-07T06:19:19Z

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.

frankomosh

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

frankomosh · 2025-11-12T09:28:56Z

src/primitives/transaction.cpp

        str += strprintf(", scriptSig=%s", HexStr(scriptSig).substr(0, 24));
+        str += strprintf(", scriptWitness=%s", scriptWitness.ToString());


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 ?

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.

frankomosh · 2025-11-12T09:40:36Z

src/primitives/transaction.cpp

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


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.

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.

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.

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.

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

@frankomosh should be addressed

@optout21 for vision

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

optout21

utACK 812495a426a3ea9ef15567983ff31e22d05e9f5d

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

src/primitives/transaction.cpp

DrahtBot · 2025-12-08T17:45:42Z

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

DrahtBot · 2025-12-15T16:17:31Z

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

Hints

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.

Ataraxia009 force-pushed the core-txIn-toString-enhancement branch from 61cd0e5 to e8b262a Compare November 7, 2025 06:18

frankomosh reviewed Nov 12, 2025

View reviewed changes

Ataraxia009 force-pushed the core-txIn-toString-enhancement branch from e8b262a to 812495a Compare November 24, 2025 19:28

DrahtBot mentioned this pull request Nov 27, 2025

refactor: C++20 operators #33771

Merged

optout21 reviewed Nov 27, 2025

View reviewed changes

maflcko reviewed Nov 27, 2025

View reviewed changes

src/primitives/transaction.cpp Outdated Show resolved Hide resolved

transaction: Adding script witness to ToString for CTxIn

bc02cc8

Ataraxia009 force-pushed the core-txIn-toString-enhancement branch from 812495a to bc02cc8 Compare December 4, 2025 04:53

DrahtBot added the Needs rebase label Dec 8, 2025

DrahtBot added the CI failed label Dec 15, 2025

		str += strprintf(", scriptSig=%s", HexStr(scriptSig).substr(0, 24));
		str += strprintf(", scriptWitness=%s", scriptWitness.ToString());

transaction: Adding script witness to ToString for CTxIn #33711

Are you sure you want to change the base?

transaction: Adding script witness to ToString for CTxIn #33711

Conversation

Ataraxia009 commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

optout21 commented Nov 1, 2025

Uh oh!

Ataraxia009 commented Nov 7, 2025

Uh oh!

frankomosh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

optout21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Dec 8, 2025

Uh oh!

DrahtBot commented Dec 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Ataraxia009 commented Oct 27, 2025 •

edited

Loading

DrahtBot commented Oct 27, 2025 •

edited

Loading