Skip to content

Witness cleanup#441

Merged
azuchi merged 8 commits into
chaintope:masterfrom
Naviabheeman:WitnessCleanup
Jul 14, 2026
Merged

Witness cleanup#441
azuchi merged 8 commits into
chaintope:masterfrom
Naviabheeman:WitnessCleanup

Conversation

@Naviabheeman

@Naviabheeman Naviabheeman commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Remove witness related code completely:(issue #405)
1 —Remove unreachable code (zero behavior change)
2 — Remove Hash/serialization (Tier 2 core: m_witness_hash, SERIALIZE_TRANSACTION_NO_WITNESS, scriptWitness)
3 — Fix script-verification witness path
4 — Fix network protocol constants and rename NODE_WITNESS to NODE_UNSUPPORTED_SERVICE and keep the peer disconnect behavior
5 — Fix tests and benchmarks
6 — Edit documentation/comments

Naviabheeman and others added 6 commits July 9, 2026 18:14
@Naviabheeman @claude
Remove entirely-unreachable witness dead code (Tier 1)
36f5eae 
Tapyrus never activated SegWit, so this scaffolding inherited from
Bitcoin Core guards invariants nothing violates:

- BlockWitnessMerkleRoot(): no callers anywhere in the tree.
- IsWitnessStandard(): only callsite was inside `#ifdef DEBUG`, which
  is never defined in a production build; collapse to the `#else` body.
- The HasWitness() guard in BlockAssembler::TestPackageTransactions
  and the "unexpected-witness" reject in ContextualCheckBlock: both
  always evaluate false since CTxIn::scriptWitness is never populated
  by production code.
- The MSG_WITNESS_TX/MSG_WITNESS_BLOCK handling in net_processing.cpp
  (AlreadyHave, ProcessGetBlockData's disk fast-path, ProcessGetData's
  dispatch condition): SERIALIZE_TRANSACTION_NO_WITNESS is already a
  no-op flag (transaction (de)serialization never reads it), so the
  witness/non-witness branches produced byte-identical output.

Ref: chaintope#405 (Part 1, Tier 1).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Naviabheeman @claude
Remove witness hash/serialization plumbing (Tier 2 core)
00c8c04 
- Remove m_witness_hash, ComputeWitnessHash(), GetWitnessHash() from
  CTransaction. ComputeWitnessHash() always returned the plain `hash`
  (HasWitness() is always false), so GetWitnessHash() == GetHash();
  replace all three callers (core_write.cpp, rpc/mining.cpp, and the
  script-execution-cache key in validation.cpp) with GetHash()
  directly. Note this is the behavior-preserving substitution, not
  GetHashMalFix() -- the "txid"/"hash" RPC fields already use
  GetHashMalFix()/GetWitnessHash() respectively, and GetHashMalFix()
  would have collapsed that distinction.
- Remove SERIALIZE_TRANSACTION_NO_WITNESS: the (de)serializer never
  checked it (transaction.h's Serialize/UnserializeTransaction has no
  witness handling at all), so every one of its ~19 callsites was
  passing a flag that changed nothing. Drop the flag from all of them
  (validation.cpp, net_processing.cpp send paths, core_read.cpp,
  consensus/tx_verify.cpp, script/sign.h's PSBT (de)serialization,
  rpc/rawtransaction.cpp).
- Remove CTxIn::scriptWitness and its ToString()/memory-usage
  accounting. It was never serialized (not touched by
  CTxIn::SerializationOp) and never populated by production code.
- Remove CTransaction::HasWitness()/CMutableTransaction::HasWitness()
  now that scriptWitness is gone, and simplify their remaining callers
  in net_processing.cpp (orphan/relay rejection-cache logic), which
  were always taking the "no witness" branch anyway.

Ref: chaintope#405 (Part 1, Tier 2).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Naviabheeman @claude
Remove witness script-verification path from the interpreter
e7c54ee 
Delete the witness-program verification machinery in
script/interpreter.cpp, all of it unreachable because
SCRIPT_VERIFY_WITNESS was never included in any production flag set:

- VerifyWitnessProgram() and the two flag-gated call sites inside
  VerifyScript() (bare and P2SH witness programs). The surrounding
  IsWitnessProgram()/stack.resize(1) cleanstack-bypass logic is left
  alone -- it isn't flag-gated and isn't part of this dead-code tier.
- WitnessSigOps()/CountWitnessSigOps() (always returned 0 given the
  flag was never set) and its sole caller in
  consensus/tx_verify.cpp's GetTransactionSigOps().
- SCRIPT_VERIFY_WITNESS, SCRIPT_VERIFY_WITNESS_PUBKEYTYPE and
  SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM from the script
  flags enum and STANDARD_SCRIPT_VERIFY_FLAGS, along with the
  now-dead CheckPubKeyEncoding() branch and IsSolvable()'s
  static_assert that referenced SCRIPT_VERIFY_WITNESS_PUBKEYTYPE.
- The now-unused `const CScriptWitness* witness` parameter from
  VerifyScript() -- 5 of its 7 callers already passed nullptr: the
  only two real callers were validation.cpp's CScriptCheck and
  tapyrusconsensus.cpp, both updated to drop the argument now that
  CTxIn::scriptWitness no longer exists to point at.
- PrecomputedTransactionData's BIP143 witness-cache branch, which
  called the now-removed CTransaction::HasWitness().

Ref: chaintope#405 (Part 1, Tier 1/2 -- VerifyScript).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Naviabheeman @claude
Remove witness wire-protocol constants; rename the peer-reject bit
ed907b6 
- Remove MSG_WITNESS_FLAG, MSG_WITNESS_BLOCK, MSG_WITNESS_TX,
  MSG_FILTERED_WITNESS_BLOCK and MSG_TYPE_MASK from protocol.h, and
  the "witness-" command-prefix branch in CInv::GetCommand() --
  net_processing.cpp no longer constructs or dispatches on these
  after the Tier 1 cleanup, so they were unused.
- Remove the "WITNESS" case from the GUI's peer-services formatter
  (qt/guiutil.cpp), which displayed the old NODE_WITNESS bit.
- NODE_WITNESS was not actually dead: it's what makes Tapyrus
  disconnect any peer that advertises it (net_processing.cpp's
  version-message handler). Since Tapyrus has no witness/SegWit
  concept, rename it to NODE_UNSUPPORTED_FEATURE -- same bit
  position, same disconnect behavior, but no longer named after a
  feature Tapyrus never had. This preserves the peer-rejection
  behavior; it's the one bit in the removed set that guards live
  network-facing behavior rather than an unreachable code path.

Ref: chaintope#405 (Part 1, Tier 2 -- protocol.h).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Naviabheeman @claude
Fix tests broken by witness removal; prune dead witness test vectors
360f4d8 
Update every test/benchmark that referenced now-removed witness APIs
(CTxIn::scriptWitness, GetWitnessHash(), SERIALIZE_TRANSACTION_NO_WITNESS,
VerifyScript()'s witness parameter, SCRIPT_VERIFY_WITNESS and friends,
NODE_WITNESS) to compile and pass again:

- denialofservice_tests.cpp, sighash_tests.cpp, script_p2sh_tests.cpp,
  script_risky_combinations_tests.cpp, txvalidationcache_tests.cpp,
  validation_block_tests.cpp, multisig_tests.cpp, bench/mempool_eviction.cpp:
  mechanical updates (drop removed flags/arguments/fields).
- transaction_tests.cpp: map the "WITNESS"/"WITNESS_PUBKEYTYPE"/
  "DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM" flag-name strings to 0
  instead of deleting them, so JSON test vectors that still name them
  in their verifyFlags column keep parsing.
- sigopcount_tests.cpp: GetTxSigOpCost no longer needs
  SCRIPT_VERIFY_WITNESS as its default flags (GetTransactionSigOps()
  no longer counts witness sigops at all), and drops the now-redundant
  "no change without SCRIPT_VERIFY_WITNESS" assertion.
- script_tests.cpp: delete the dedicated P2WSH/P2WPKH BIP143
  TestBuilder block (~380 lines) -- it specifically exercised witness
  verification behavior that no longer exists, so patching it to
  compile without also testing something real wasn't meaningful.
  BuildSpendingTransaction/DoTest keep their CScriptWitness parameter
  (harmless, unused) rather than threading a signature change through
  every remaining non-witness call site.

Also prune the ~196 JSON test-vector entries in script_tests.json,
tx_valid.json and tx_invalid.json whose verifyFlags column names a
WITNESS-family flag: these assert specific witness-verification
pass/fail outcomes (e.g. a P2WSH scriptPubKey expected to fail with
UNBALANCED_CONDITIONAL under the WITNESS flag) that no longer hold
now that VerifyScript() never inspects witness data. Removal is a
span-preserving text-level deletion (verified byte-identical
formatting on surviving entries), not a re-serialization of the
files, so the diff is pure deletion.

Ref: chaintope#405 (Part 1).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Naviabheeman @claude
Fix stale witness references in documentation comments
e5c39ba 
- txmempool.h: totalTxSize claimed serialized size "differs...since
  witness data is discounted" (a BIP 141 concept) -- Tapyrus has no
  witness discount.
- transaction.h: GetTotalSize()'s doc comment claimed the size
  includes witness data per BIP141/BIP144; the implementation is a
  plain ::GetSerializeSize() with no witness handling.

Ref: chaintope#405 (Part 1).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Naviabheeman
Naviabheeman requested a review from azuchi July 9, 2026 13:16
@azuchi

azuchi commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Two small in-scope items I'd like to see folded into this PR before it lands — both are within the spirit of the sweep and small enough that a separate PR feels like overhead.

rpc/mining.cpp — dead fSupportsSegwit / default_witness_commitment block

bool fSupportsSegwit = false; at rpc/mining.cpp:502 is hardcoded and never assigned true anywhere, so every downstream branch collapses:

  • :513 fLastTemplateSupportsSegwit != fSupportsSegwit — always fLastTemplateSupportsSegwit != false
  • :522 fLastTemplateSupportsSegwit = fSupportsSegwit; — always writing false
  • :526 CreateNewBlock(scriptDummy, fSupportsSegwit) — always passes false
  • :605-606: 
    if (!pblocktemplate->vchCoinbaseCommitment.empty() && fSupportsSegwit) {
    result.pushKV("default_witness_commitment", HexStr(...));
}
    never fires — getblocktemplate will never emit the default_witness_commitment field, so miners polling this RPC see an unreachable-in-practice key documented in our output.

Please strip fSupportsSegwit and its two bookkeeping variables, drop the fSupportsSegwit argument from CreateNewBlock (miner.h / miner.cpp), delete the default_witness_commitment push, and remove the now-stale // to select witness transactions, after segwit activates ... comment at :500. All of this falls under the same "Tapyrus never activated segwit, so this is unreachable" rationale as the commits already here.

net_processing.cpp:262 — stale doc comment

fSupportsDesiredCmpctVersion's doc comment still opens with

/**
 * If we've announced NODE_WITNESS to this peer: whether the peer sends witnesses in cmpctblocks/blocktxns,
 * otherwise: whether this peer sends non-witnesses in cmpctblocks/blocktxns.
 */

which is factually wrong now — we don't announce NODE_WITNESS (it's NODE_UNSUPPORTED_FEATURE and used purely for peer rejection), and the "sends witnesses in cmpctblocks/blocktxns" concept doesn't apply to Tapyrus. Just needs a one-shot rewording — commit 6 caught txmempool.h and transaction.h doc comments but missed this one.

The rest of what I noted (the SigVersion::WITNESS_V0/BIP143-sighash branch, SCRIPT_VERIFY_MINIMALIF, the IsWitnessProgram() CLEANSTACK bypass, and the CScriptWitness& test-helper parameter) is fine as a follow-up "witness residuals" PR — those either touch semantics worth discussing separately or are pure noise that doesn't affect correctness.

@Naviabheeman
Reivew feedback - remove fSupportsSegwit and fWitnessEnabled as sugge…
987d841 
…sted in the review. Also remove fSupportsDesiredCmpctVersion as it is now redundant with fProvidesHeaderAndIDs.
@Naviabheeman
Cleanup witness related code in descriptor feature. now descriptors a…
568690c 
…re only for p2pkh, p2sh and multisig addresses. no colored coin support
@azuchi
azuchi merged commit 86ad72b into chaintope:master Jul 14, 2026
32 of 34 checks passed
@Naviabheeman Naviabheeman mentioned this pull request Jul 17, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@azuchi azuchi azuchi approved these changes

Assignees

@azuchi azuchi

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Naviabheeman @azuchi