Witness CScriptWitness cleanup#444
Open
Naviabheeman wants to merge 8 commits into
Open
Conversation
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>
- 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>
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>
- 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>
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>
- 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>
…sted in the review. Also remove fSupportsDesiredCmpctVersion as it is now redundant with fProvidesHeaderAndIDs.
…re only for p2pkh, p2sh and multisig addresses. no colored coin support
Contributor
|
Reads to me like this branch got left based on old master and hasn't been rebased — If the intent is Tier 3 — removing the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Earlier witness cleanup #441 didn't remove its support from script related code.
CTxInstill hasCScriptWitness. This PR removes CScriptWitness and all code using it layer by layer.