#36521 Keep witness stacks in serialized form in CScriptWitness

ReviewabilityReviewsAgreementSize
#36521 sipa Keep witness stacks in serialized form in CScriptWitnessNeeds rebase0 (+1) -1Blocked502 + 752 tests
  • Protect nodes from memory exhaustion and slow deserialization caused by crafted witness data
  • Stop peers from inducing large memory allocations with malformed transactions or blocks
  • Review is worthwhile now as nothing is known to invalidate it
  • Broad concept support for mitigating exceptional transaction memory bloat (furszy, 0xB10C, instagibbs)
  • Verified by testing on Windows after initial efficiency suggestions were addressed (Yudis-bit)
  • Unaddressed objection: memory allocation remains high for unknown witness versions (fametrano)
  • Approach objection: excessive churn across 76 files before abstracting data access (purpleKarrot)
  • 502 lines added or modified outside tests
  • 752 lines added or modified in tests
  • 279 lines removed in total
  • 76 files, 17 commits
P2P#1 of 71
P1 · DoS protection
  • P1 because it closes a denial-of-service vector where peers trigger large allocations before rejection
  • Untrusted peers can no longer force nodes to allocate excess memory for oversized payloads
Ranking notes
Validation#1 of 53
P1 · DoS protection
  • P1 because it removes a severe block deserialization denial-of-service vulnerability
  • Worst-case block deserialization drops from 279 ms to 19 ms and peak memory drops from 182 MiB to 8 MiB
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Replaces the internal storage of CScriptWitness from a vector of byte vectors with a single allocation storing its serialized wire format, decoding elements on the fly as spans and making witness stacks immutable after creation. Transaction deserialization is updated to immediately reject transactions claiming more inputs or outputs than could fit in a block before allocating vector storage. For P2P streams, deserialization also decrements a weight budget to reject blocks and transactions exceeding the maximum block weight limit and drops transactions with zero inputs or outputs.

Problem: Holding witness stacks as nested vectors adds 24 bytes of object overhead and a distinct heap allocation for each element, allowing attackers to craft 4 MB blocks with millions of small elements that consume over 100 MiB of RAM and hundreds of milliseconds to deserialize. Furthermore, deserializing malformed transactions or blocks without pre-allocation limits permits peers to induce large memory allocations before validation rejects them.

Evidence of importance:

  • Description benchmarks show worst-case valid block deserialization time drops from 279 ms to 19 ms, with persisted block memory usage dropping from 106.8 MiB to 3.8 MiB and peak memory dropping from 182 MiB to 7.8 MiB.
  • PR description notes the underlying vulnerability was reported by furszy and 0xB10C.

Scope: Touches 76 files, but a substantial fraction is a scripted-diff refactor migrating direct .stack accesses to CScriptWitness accessor methods across tests and validation code, accompanied by dedicated deserialization benchmarks and fuzz targets.

Risk of a mistake: high (consensus, validation, p2p). Altering witness serialization and adding early deserialization rejection could cause nodes to reject valid transactions or blocks, leading to chain splits or network partitions.

Changed since the previous assessment: Added deserialization pre-checks that reject transactions with more inputs or outputs than can physically fit in a block across all decoders, and introduced a 4M weight budget check during P2P message deserialization.

Reviewability

Ready · Needs rebase

Conflicts with master; review the logic now, the rebase is mechanical.

Waiting on (from the thread read):

  • author: response or fix for fametrano's allocation regression concern in VerifyWitnessProgram 2026-10-25
  • author: response to purpleKarrot's approach objection and NACK 2026-10-29

Agreement

Blocked

objection that needs a change, nobody has replied (fametrano, purpleKarrot)

Thread notes: Added purpleKarrot's approach objection/NACK questioning widespread code changes across 76 files prior to completing transaction abstraction work.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
Yudis-bit (none)suggestion, approachAvoid heap allocations in IsWitnessStandard for Taproot inputs by checking witness size and the last element before calling ToSpansaccepted · fix pushed · author addressed in a separate commit and Yudis-bit tested ACKedansweredyes2026-10-17: 'checking witness.size() >= 2 and witness.back() upfront before calling ToSpans() avoids that allocation.'
Status from: 2026-10-20: Yudis-bit: 'Tested ACK d3fb8e8 on Windows.'
Yudis-bit (none)suggestion, approachIterate directly over scriptWitness with a counter in the P2WSH check to avoid allocating a vector of spansaccepted · fix pushed · author addressed in a separate commit and Yudis-bit tested ACKedansweredyes2026-10-17: 'Iterating directly over scriptWitness with a counter avoids allocating the vector of spans without doing O(n^2) indexing.'
Status from: 2026-10-20: Yudis-bit: 'Tested ACK d3fb8e8 on Windows.'
fametrano (none)objection, approachVerifyWitnessProgram calls witness.ToVectors() before checking the witness version, causing a 91 MiB allocation and verification slowdown for unknown witness versionsopenchangedno2026-10-25: 'VerifyWitnessProgram calls witness.ToVectors() before it looks at the witness version, so the valid "1 tx, max empty wit elems" block from the description still allocates about 91 MiB, now during scri'
purpleKarrot (member)objection, approachTouching 76 files with 1000+ lines for an interim data layout before localizing transaction access introduces unnecessary churn instead of first abstracting data accessopenchangedno2026-10-29: 'Instead pushing 17 commits touching over 1000 lines in 76 files, just to establish an implementation that is proven better but not proven best, should we maybe concentrate on the task that would allow us to make such a change localized in a single place?'

Support:

  • furszy (member) (Concept ACK, 2026-10-14): Obvious concept ACK [not substantive]
  • 0xB10C (member) (Concept ACK, 2026-10-14): Concept ACK [not substantive]
  • instagibbs (member) (Concept ACK, 2026-10-15) [read]: this issue with exceptional tx types shows up in a lot of places, like #36015
  • w0xlt (contributor) (Concept ACK, 2026-10-15): Concept ACK [not substantive]
  • rkrux (member) (Concept ACK, 2026-10-15): Concept ACK [not substantive]
  • Yudis-bit (none) (Tested ACK, 2026-10-20) [tested]: Tested ACK d3fb8e8 on Windows. [not substantive]

Participants:

  • 0xB10C (member, support, 1 statements 2026-10-14 to 2026-10-14): concept ACK
  • furszy (member, support, 1 statements 2026-10-14 to 2026-10-14): concept ACK
  • instagibbs (member, support, 1 statements 2026-10-15 to 2026-10-15): concept ACK citing recurring issues with exceptional transaction types
  • rkrux (member, support, 1 statements 2026-10-15 to 2026-10-15): concept ACK
  • w0xlt (contributor, support, 1 statements 2026-10-15 to 2026-10-15): concept ACK
  • Yudis-bit (none, support, 2 statements 2026-10-17 to 2026-10-20): tested ACK after suggestions were addressed
  • fametrano (none, objection, 1 statements 2026-10-25 to 2026-10-25): objected to 91 MiB allocation in VerifyWitnessProgram for unknown witness versions
  • purpleKarrot (member, objection, 1 statements 2026-10-29 to 2026-10-29): NACKed, arguing against touching 76 files before localizing transaction data access

Files

917 lines under test/bench/ci.

  • src/script/script.h +184/-8
  • src/test/transaction_tests.cpp +166/-3
  • src/bench/block_deserialize.cpp +155/-0
  • src/test/fuzz/script_witness.cpp +92/-0
  • src/script/script.cpp +86/-5
  • src/primitives/transaction.h +70/-12
  • src/test/script_tests.cpp +67/-11
  • src/test/blockencodings_tests.cpp +49/-12
  • src/serialize.h +46/-0
  • src/script/interpreter.cpp +25/-16
  • src/net_processing.cpp +25/-10
  • src/test/streams_tests.cpp +29/-0
  • src/test/miniscript_tests.cpp +15/-13
  • src/test/serialize_tests.cpp +28/-0
  • src/test/fuzz/miniscript.cpp +13/-11
  • src/bench/mempool_eviction.cpp +11/-11
  • src/test/txpackage_tests.cpp +6/-16
  • src/policy/policy.cpp +10/-9
  • src/test/sigopcount_tests.cpp +4/-14
  • src/test/fuzz/transaction.cpp +17/-0
  • src/test/bip352_tests.cpp +6/-8
  • src/test/validation_tests.cpp +7/-7
  • src/psbt.cpp +6/-6
  • src/script/sign.cpp +6/-6
  • src/psbt.h +5/-5
  • src/rpc/rawtransaction.cpp +5/-5
  • src/test/fuzz/connect_block.cpp +5/-4
  • src/test/fuzz/util.cpp +5/-4
  • test/functional/mempool_accept.py +9/-0
  • src/common/bip352.cpp +4/-4
  • src/kernel/bitcoinkernel.cpp +5/-3
  • src/test/fuzz/cmpctblock.cpp +4/-4
  • src/test/txvalidationcache_tests.cpp +5/-3
  • test/functional/feature_block.py +5/-3
  • src/core_memusage.h +2/-5
  • src/test/fuzz/script.cpp +4/-3
  • src/core_io.cpp +3/-3
  • src/test/fuzz/deserialize.cpp +3/-3
  • src/test/fuzz/script_assets_test_minimizer.cpp +3/-3
  • src/test/orphanage_tests.cpp +4/-2
  • src/test/script_assets_tests.cpp +3/-3
  • src/validation.cpp +3/-3
  • test/functional/data/invalid_txs.py +6/-0
  • src/rpc/rawtransaction_util.cpp +3/-2
  • src/test/fuzz/txorphan.cpp +2/-3
  • src/wallet/test/feebumper_tests.cpp +4/-1
  • src/bench/mempool_stress.cpp +2/-2
  • src/blockencodings.cpp +2/-2
  • src/blockencodings.h +2/-2
  • src/test/fuzz/package_eval.cpp +2/-2
  • src/test/psbt_tests.cpp +2/-2
  • src/test/validation_block_tests.cpp +2/-2
  • src/wallet/test/fuzz/coincontrol.cpp +2/-2
  • src/test/rbf_tests.cpp +1/-2
  • test/functional/p2p_segwit.py +2/-1
  • src/bench/block_assemble.cpp +1/-1
  • src/bench/blockencodings.cpp +1/-1
  • src/bench/rpc_mempool.cpp +1/-1
  • src/bench/verify_script.cpp +1/-1
  • src/bitcoin-tx.cpp +1/-1
  • src/consensus/validation.h +1/-1
  • src/node/miner.cpp +1/-1
  • src/primitives/transaction.cpp +1/-1
  • src/rpc/blockchain.cpp +1/-1
  • src/signet.cpp +1/-1
  • src/streams.h +2/-0
  • src/test/fuzz/tx_pool.cpp +1/-1
  • src/test/fuzz/txdownloadman.cpp +1/-1
  • src/test/private_broadcast_tests.cpp +1/-1
  • src/test/txdownload_tests.cpp +1/-1
  • src/test/txvalidation_tests.cpp +1/-1
  • src/wallet/feebumper.cpp +1/-1
  • src/wallet/wallet.cpp +1/-1
  • test/functional/p2p_invalid_tx.py +1/-1
  • src/bench/CMakeLists.txt +1/-0
  • src/test/fuzz/CMakeLists.txt +1/-0

Card

The short text the ranking pass compares across PRs.

CScriptWitness currently stores stack items as a vector of byte vectors, adding a 24-byte header and a separate heap allocation for each item. This allows peers to send 4 MB blocks that take over 100 MiB of memory and nearly 300 ms to deserialize. This PR changes CScriptWitness to store its serialized representation in a single allocation, decoding elements on the fly, and introduces early size and weight budget bounds during transaction and P2P block deserialization. Benchmarks show worst-case block memory dropping from 106.8 MiB to 3.8 MiB and deserialization times dropping by up to 93%.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-30T01:14, confidence high, input hash 6494f632f7699f80
thread read through 2026-10-29 (11 statements), head bd51226995
labels: Needs rebase

Categories

P2P

P1 · DoS protection

Mitigates a P2P DoS vector where peers can induce large memory allocations before validation rejects messages. Stream deserialization is updated to track weight budgets to prevent memory exhaustion from crafted P2P payloads.

Membership: Modifies net_processing.cpp to enforce weight budgets during P2P stream deserialization, dropping invalid transactions immediately.

Factors: security/stability 3, bug 0, performance 3, user value 0, leverage 1

Validation

P1 · DoS protection

Eliminates a severe validation-time memory bloat and deserialization DoS vector reported by researchers. Description benchmarks show worst-case block deserialization time drops from 279 ms to 19 ms, persisted block memory from 106.8 MiB to 3.8 MiB, and peak memory from 182 MiB to 7.8 MiB.

Membership: Changes transaction and witness primitives in src/script/ and src/primitives/transaction.h, dramatically cutting memory and CPU overhead in block deserialization.

Factors: security/stability 3, bug 0, performance 3, user value 0, leverage 1