sipa
commented at 6:23 AM on October 14, 2026:
member
CScriptWitness stores a witness stack as a std::vector<std::vector<unsigned char>>, which costs a 24-byte vector object and an allocation for every element. This makes it possible to construct transactions that take a large amount of memory after deserialization, while becoming very slow to deserialize.
This PR stores the stack as its serialization in a single allocation (or none if the stack is empty), with a small header holding its size and the offset of the last element. Witnesses can no longer be modified in place, only allowing replacing them entirely. Access to the individual witness stack elements is done by on-the-fly deserialization instead. CScriptWitness shrinks from 24 to 8 bytes, and CTxIn from 112 to 96 bytes.
Later commits also introduce stricter checking during deserialization, rejecting the following before fully deserializing:
Transactions with more inputs/outputs than are possible in a valid transaction.
Transactions with 0 inputs or 0 outputs (applied in P2P, but not RPC etc. for backward compatibility).
Transactions/blocks that exceed the 4M weight limit (also only applied in P2P).
The result is a significant reduction in the worst-case time and memory allocations needed for deserialization, both in valid and invalid transactions. With it, the worst-case memory of a valid deserialized block drops from 106.8 MiB to 6.0 MiB. Memory usage during deserialization can temporarily be higher: up to 7.8 MiB for valid blocks, and up to 13.4 MiB for invalid blocks, which can combine a few temporary vector allocations of up to 5 MB each.
Deserialization time and memory of the deserialized block, master → PR. Runtimes are medians in milliseconds. Memory usage is in MiB, and ✕ means rejected at deserialization time. Peak memory refers to the memory during deserialization itself, not persisted in the deserialized CBlock object. Numbers over 50 ms, 20 MiB usage, or 100 MiB peak are in bold.
So in addition to improving worst cases very significantly, it also provides modest memory usage improvements to the common case. This translates to mildly increasing the effective capacity of memory-bounded data structures that store transactions, like the mempool.
Some notable changes:
By implementing some checks (for inherently consensus-invalid things) in the deserialization logic, behavior may change. In particular, deserialization errors in P2P cause messages to be dropped/ignored, rather than triggering validation or net processing logic (which may include disconnections and/or inserting into reject filters).
The rejection of transactions with too many inputs or outputs applies everywhere, including in RPC. That's hopefully a desirable change, but a change in behavior still.
Random access to the witness stack elements becomes $\mathcal{O}(n)$. This is not a problem for any validation logic, but may be relevant for the libbitcoinkernel API which exposes an individual element accessor.
Issue reported by furszy and 0xB10C.
There are a few possible follow-ups:
Enforce the 400k WU policy limit during transaction deserialization when active (instead of the 4M WU consensus limit).
Extend this approach to entire CTransaction, allowing all transaction data to be stored packed together in a few allocations (rather than one per input, one per output, one per witness) plus some index structure to access it efficiently. That's a much bigger change, needing abstractions like #35569, but would reduce the maximum memory usage per WU for valid blocks from around 1.5 bytes/WU to 1.1 bytes/WU (compared to ~28 bytes/WU in master).
DrahtBot
commented at 6:23 AM on October 14, 2026:
contributor
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Conflicts
Reviewers, this pull request conflicts with the following ones:
#36787 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36787.svg"></sub> (cmpctblock: Cache serialized cmpctblock msg for relay [part of prefill series] by davidgumberg)
#36776 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36776.svg"></sub> (bip352: skip scriptSig eval for non-segwit P2SH inputs by Eunovo)
#36721 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36721.svg"></sub> (script: store interpreter stack elements as reference-counted objects by conduition)
#36548 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36548.svg"></sub> (net: serve MSG_BLOCK by stripping witnesses in place by l0rinc)
#36466 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36466.svg"></sub> (policy: make unstructured Taproot annexes standard by RobinLinus)
#36122 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36122.svg"></sub> (BIP460: CISA for Taproot key path spends by fjahr)
#36109 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36109.svg"></sub> (test: fix coinbases built on arbitrary parents by l0rinc)
#35788 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35788.svg"></sub> (bitcoin-util: Add evalscript subcommand by ajtowns)
#35716 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35716.svg"></sub> (wallet: Replace mapWallet and wtxOrdered with a boost::multi_index by achow101)
#35671 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35671.svg"></sub> (mining: add TxCollection to bandwidth-efficiently validate external block templates by Sjors)
#35662 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35662.svg"></sub> (script: prevent partial reinitialization of sighash data by l0rinc)
#35558 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35558.svg"></sub> (p2p: Prefill compact blocks by davidgumberg)
#35522 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35522.svg"></sub> (refactor: Extract per-message helpers from SendMessages() (move-only) by pablomartin4btc)
#35502 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35502.svg"></sub> (refactor: extract per-message helpers from ProcessMessage (move-only) by w0xlt)
#31252 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/31252.svg"></sub> (rpc: print P2WSH and P2SH redem Script in getrawtransaction and getblock by polespinasa)
#29247 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/29247.svg"></sub> (CAT in Tapscript (BIP-347) by arminsabouri)
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
sipa force-pushed on Oct 14, 2026
DrahtBot added the label CI failed on Oct 14, 2026
furszy
commented at 6:35 AM on October 14, 2026:
member
Obvious concept ACK
0xB10C
commented at 6:35 AM on October 14, 2026:
contributor
Concept ACK
sipa force-pushed on Oct 14, 2026
sipa force-pushed on Oct 14, 2026
DrahtBot removed the label CI failed on Oct 14, 2026
instagibbs
commented at 12:13 AM on October 15, 2026:
member
definite concept ACK, this issue with exceptional tx types shows up in a lot of places, like #36015
w0xlt
commented at 6:10 AM on October 15, 2026:
contributor
Concept ACK
rkrux
commented at 1:06 PM on October 15, 2026:
contributor
Concept ACK
Yudis-bit
commented at 1:22 PM on October 17, 2026:
none
In IsWitnessStandard (src/policy/policy.cpp), tx.vin[i].scriptWitness.ToSpans() allocates on the heap for every Taproot input, even though for standard keypath spends (size() == 1) no policy rules apply. Since back() is O(1), checking witness.size() >= 2 and witness.back() upfront before calling ToSpans() avoids that allocation.
Similarly in the P2WSH check, ToSpans() is used for indexed access up to sizeWitnessStack. Iterating directly over scriptWitness with a counter avoids allocating the vector of spans without doing O(n^2) indexing.
sipa force-pushed on Oct 19, 2026
sipa
commented at 1:01 AM on October 20, 2026:
member
@Yudis-bit Thanks, addressed in a separate commit at the end.
Yudis-bit
commented at 3:08 AM on October 20, 2026:
none
Tested ACKd3fb8e8 on Windows.
DrahtBot requested review from furszy on Oct 20, 2026
DrahtBot requested review from rkrux on Oct 20, 2026
DrahtBot requested review from instagibbs on Oct 20, 2026
DrahtBot requested review from 0xB10C on Oct 20, 2026
DrahtBot added the label Needs rebase on Oct 22, 2026
sipa force-pushed on Oct 22, 2026
sipa
commented at 6:52 PM on October 22, 2026:
member
I have added changes that also reject transactions/blocks early during deserialization which exceed the 4M WU limit. This reduces the worst-case memory and deserialization time for invalid transactions/blocks further.
DrahtBot removed the label Needs rebase on Oct 22, 2026
fametrano
commented at 7:45 AM on October 25, 2026:
contributor
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 script verification.
Such a witness is only valid when no script runs on it, as when it spends an unknown witness version, so the copy is never used. As in 8072be3acb, calling ToVectors() only in the branches that reach ExecuteWitnessScript would keep the saving through validation.
I counted heap in a program that replaces operator new, linked against each build, and took times from regtest nodes running each build. With 3,990,000 empty elements spending a v2 output, deserialization peaks at 7.8 MiB on bd51226995 against 182.7 MiB on its base. VerifyScript then makes a 91.3 MiB copy that the base does not make. The Verify 1 txins bench line goes from 0.02-0.03 ms to 18-24 ms. The block is still accepted faster than on the base (0.04-0.05 s against 0.14-0.20 s).
I also compared the two builds over RPC and P2P in one functional test (edge cases, test vectors, blocks at the weight limit, PSBTs), and on 400,000 mutated transactions fed to each deserializer. I checked decoding, wtxids, sighashes, weight and sigops against btclib, a parser I maintain. Apart from error messages, the only differences were those the description lists. I can share the code, corpus generator included, as a gist.
purpleKarrot
commented at 5:13 PM on October 29, 2026:
contributor
May I dare to NACK this? I understand that keeping the witness data in serialized format considerably improves performance, but as I wrote in #35569 (comment), there sure are hundreds of possibilities for data layouts that may be even more optimized for fast data access.
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? This would give us the possibility to easily experiment with different data layouts and lead to an even faster implementation?
DrahtBot added the label Needs rebase on Oct 30, 2026
alexanderwiederin
commented at 11:24 AM on November 2, 2026:
contributor
Would it make sense to split this into (1) the CScriptWitness interface and caller conversion (e41dd47 through 9e22357), (2) the serialised representation, and (3) the deserialisation checks?
Part 1 is a pure refactor that localises the representation and makes parts 2 and 3 smaller to review. The P2P behaviour changes in part 3 might also benefit from separate discussion.
bench: add block deserialization benchmarks for synthetic blocks
Add benchmarks deserializing blocks filled up to the weight limit with
synthetic transactions of a few shapes that stress different parts of
deserialization:
* spends of P2WPKH outputs to P2WPKH outputs (a typical segwit block)
* the smallest segwit transactions
* one transaction with as many inputs as fit,
* one transaction with as many outputs as fit,
* one transaction whose witness has as many 1-byte elements as fit.
ceb38d05cc
script: give CScriptWitness an interface that hides its representation
Add element access through spans (forward iteration, front(), back(), and
indexing), conversion to and construction from a vector of elements,
serialization, and memory usage accounting, so that users no longer need to
access the stack vector directly. This prepares for a future commit which
will replace the representation with a more compact & efficient one.
The stack vector itself stays public until the next commits have converted
its users.
0354397577
test: check CScriptWitness against the vector of vectors it represents
Add a unit test and a fuzz test that compare CScriptWitness's interface
(accessors, construction, and serialization) with the
std::vector<std::vector<unsigned char>> it represents. The unit test
focuses on element counts and sizes around CompactSize encoding
boundaries.
95c73e5d25
scripted-diff: use CScriptWitness's interface instead of its stack
Convert the uses of CScriptWitness::stack for which the new interface
is a simple substitution. The remaining ones are converted manually in
the next commits.
-BEGIN VERIFY SCRIPT-
FILES=$(git grep -l -E '\.stack\b|(scriptWitness|final_script_witness)\.SetNull\(\)' -- src ':!src/script/miniscript.*' ':!src/script/script.h')
# Element count, emptiness, and last element: the witness has size(), empty() and back() itself. Lines that need the
# stack itself (to compute the size of its serialization without the element count, to pass the last element where a
# vector is expected, or to index all elements in a loop) are left for the manual changes.
sed -i -E -e '/GetSizeOfCompactSize|TAPROOT_LEAF_TAPSCRIPT|i < txin\.scriptWitness/b' \
-e 's/\.stack\.(size|empty|back)\(\)/.\1()/g' $FILES
# Reading an element by index: the witness has operator[] itself. Writes to elements, and loops indexing all elements
# (which can be quadratic with indexing that is not constant-time), are left for the manual changes.
sed -i -E -e '/MAX_STANDARD_P2WSH_STACK_ITEM_SIZE|HexStr\(txin\.scriptWitness|\.stack\[[^]]*\]( =|\.resize)|^\s*\+\+/b' \
-e 's/\.stack\[/[/g' $FILES
# Const references to the stack: refer to the witness itself, which provides the same read-only accessors.
sed -i -E 's/^(\s*const auto& \w+( = |\{).*)\.stack(\}?;)$/\1\3/' $FILES
# Comparisons of two stacks: compare the witnesses, which are equal exactly when their stacks are.
sed -i -E '/\.stack [!=]= /s/\.stack\b//g' $FILES
# Serialization of the stack, and its size: serialize the witness itself, which has the same serialization.
sed -i -E -e 's/ (>>|<<) ([][A-Za-z0-9_.]+)\.stack( <<|;)/ \1 \2\3/' \
-e 's/(SerializeToVector|UnserializeFromVector)\(s, (\w+)\.stack\)/\1(s, \2)/' \
-e 's/::GetSerializeSize\(([][A-Za-z0-9_.]+)\.stack\)/::GetSerializeSize(\1)/' $FILES
# Range-for loops over the stack: iterate over the witness. Its elements are spans, returned by value, so they are not
# bound by reference.
sed -i -E 's/for \(const auto& (\w+) : ([][A-Za-z0-9_.]+)\.stack\)/for (const auto \1 : \2)/' $FILES
# Clearing the stack (SetNull(), clear() or resize(0)): assign an empty witness. A clear() followed by pushing elements
# in a loop is left for the manual changes.
sed -i -E -e '/txin\.scriptWitness\.stack\.clear/b' \
-e 's/(scriptWitness|final_script_witness)\.SetNull\(\)/\1 = {}/' \
-e 's/(scriptWitness)\.stack\.(clear\(\)|resize\(0\));$/\1 = {};/' $FILES
# Assigning a vector of elements to the stack: assign a witness constructed from it (moving from the vector is
# pointless, as the witness is constructed from a const reference).
sed -i -E 's/(\w+)\.stack = (std::move\((\w+)\)|(.*));$/\1 = CScriptWitness{\3\4};/' $FILES
# Pushing a single element onto an empty witness (in these benchmarks and tests): assign a witness constructed from
# that element. The one place that pushes onto a non-empty witness (to make a different witness) is left for the
# manual changes, as are pushes in loops.
sed -i -E '/ tx\.vin\[0\]\.scriptWitness\.stack\.push_back\(\{5\}\)|Consume/!s/(\w+)\.stack\.push_back\((.*)\);$/\1 = CScriptWitness{{\2}};/' \
src/bench/block_assemble.cpp src/bench/blockencodings.cpp src/bench/mempool_eviction.cpp src/bench/mempool_stress.cpp src/bench/rpc_mempool.cpp \
src/test/fuzz/connect_block.cpp src/test/fuzz/txdownloadman.cpp src/test/orphanage_tests.cpp src/test/txdownload_tests.cpp src/test/validation_block_tests.cpp
# IsNull(): use empty(), which the witness provides along with size() and its other container-like accessors.
sed -i -E -e 's/(scriptWitness|final_script_witness)\.IsNull\(\)/\1.empty()/g' -e 's/witness->IsNull\(\)/witness->empty()/g' \
$(git grep -l -E '(scriptWitness|final_script_witness)\.IsNull\(\)|witness->IsNull\(\)' -- src)
-END VERIFY SCRIPT-
e78a86fa58
script: let ExecuteWitnessScript take its initial stack by value
This lets callers move the stack into it, rather than having it be
copied from a span. This will be used in a future commit.
b0c0cbd0f4
refactor: use CScriptWitness's interface where that needs more than a rename
Convert the remaining non-test uses of CScriptWitness::stack, which need
more than a substitution. The script interpreter now converts the
witness stack to a vector of vectors once, and moves the remaining
elements into the execution stack, rather than copying them.
11b5566f68
test, bench, fuzz: build witness stacks before constructing a CScriptWitness
Convert the remaining uses of CScriptWitness::stack in tests, benchmarks,
and fuzz tests, by constructing witnesses from a separately-built vector
of elements, rather than modifying the stack in place.
b791e22fa4
streams: add SpanReader::Remaining()
This returns the data that has not been read yet, as a span referring
to the underlying data. It will be used in the next commit to find the
elements of a witness stack within its serialization.
c4fb996171
script: store witness stacks as their serialization
Replace CScriptWitness's vector of vectors with a single allocation,
containing the size of the stack's serialization (4 bytes, little
endian), followed by that serialization. An empty stack uses no
allocation at all.
This means a witness stack no longer needs one allocation per non-empty
element plus one for the outer vector, and no longer has a 24-byte
overhead per element. A block whose witnesses consist of millions of
tiny elements thus no longer costs tens of bytes of memory and an
allocation per byte. It also shrinks CScriptWitness from 24 to 8 bytes,
and thus CTxIn from 112 to 96 bytes. The downside is that indexing
becomes O(n) in the distance from the bottom of the stack.
This is the simplest implementation of this representation:
deserialization still goes through a vector of vectors, and elements
are found by parsing the serialization with SpanReader and
ReadCompactSize. Future commits will make both faster.
a1cea0d7e0
script: deserialize witness stacks directly into their serialization
Instead of deserializing a witness stack as a vector of vectors and then
serializing that again, build the serialization directly while reading,
with the same checks. This makes deserializing a block whose witness
consists of millions of 1-byte elements about 10x faster.
To support this, add AppendCompactSize to serialize.h, which cheaply
appends a CompactSize to a byte vector.
5c175e82b3
script: decode the CompactSizes in witness stacks inline
Add DecodeCompactSize to serialize.h, which decodes a CompactSize from a
span without any checks, making it small enough to be inlined.
Use it to find the elements in a CScriptWitness's serialization, which
is known to be valid as CScriptWitness constructed it. This makes
iterating over witness stacks 5-7x faster, both for real blocks and for
blocks whose witnesses consist of millions of 1-byte elements. As a
bonus, script.h no longer needs streams.h.
b28174d41c
script: store the offset of a witness stack's last element
Add the offset of the last element to the header of CScriptWitness's
allocation, making back() and accessing the last element by index O(1).
The last element is used in P2WSH sigop counting, policy checks, and
BIP352 public key extraction.
This adds 4 bytes to the allocation of every non-empty witness stack,
but does not change the minimum allocation size of a single-element
stack, nor the worst-case ratio of memory usage to weight or size.
c190556259
primitives: reject txs with more inputs or outputs than fit in a block
When deserializing a transaction, read the input and output counts
first, and fail before reading the inputs or outputs if more of them
are claimed than can fit in a block, given their minimum size (41 bytes
for an input, 9 for an output). Such a transaction can never be valid.
This allows us to allocate the vin and vout vectors immediately, rather
than sequentially in batches of 5 MB as the std::vector deserializer
does, because that memory becomes bounded: to 2.3 MB for 24390 inputs,
and 5.3 MB for 111111 outputs. It is a preparation for further changes
that rely on this structuring of conditionals.
This affects all places where transactions are deserialized. Through
RPC, such transactions can no longer be decoded. From P2P, a transaction
or block containing one is ignored like other malformed messages, rather
than rejected by validation.
2d869d8186
sipa force-pushed on Nov 5, 2026
DrahtBot removed the label Needs rebase on Nov 5, 2026
p2p: reject transactions without inputs or outputs when deserializing
Add a reject_empty flag to TransactionSerParams, which makes
deserialization fail as soon as a transaction is found to have no
inputs or no outputs. Such transactions are never valid, but are
currently fully deserialized (along with the rest of the message) before
validation rejects them.
This reduces the memory a peer can make us allocate by sending a block.
A block full of 10-byte empty transactions was the previous worst case;
with this change, the worst case becomes 1-input 1-output transactions
with a significantly lower per-WU memory usage.
The flag is enabled (as TX_WITH_WITNESS_P2P) only where transactions are
received from P2P peers: tx and block messages, and the transactions in
cmpctblock and blocktxn messages. Like other malformed messages, these
are now ignored, rather than, for a block, causing the peer to be
disconnected, or, for a transaction, being added to the reject filters.
The functional tests are updated accordingly.
b2ee9d29f2
policy: iterate over witness stacks directly in IsWitnessStandard
Instead of converting a witness stack to a vector of spans for the P2WSH
and Taproot checks, use its size() and back() (which are constant-time)
and iterate over its elements directly. This avoids an allocation for
every witness spend that is checked.
60b2da3cc8
refactor: use the stream's parameters for compact block transactions
BlockTransactions and PrefilledTransaction (de)serialized their
transactions with TX_WITH_WITNESS_P2P, overriding any parameters of the
stream. Use the stream's parameters instead, like CBlock does, and pass
TX_WITH_WITNESS where they are serialized and TX_WITH_WITNESS_P2P where
they are deserialized. This does not change behavior (serialization only
depends on allow_witness), but makes it possible to deserialize them
with other parameters.
5ed576d99a
p2p: limit the weight of transactions and blocks while deserializing
Add an optional weight budget to TransactionSerParams. When it is set,
the budget is decremented while transactions are being deserialized
(charging the minimum weight of inputs and outputs before allocating
memory for them), causing failure as soon as the budget is exceeded.
Use this for tx, block, cmpctblock and blocktxn messages, with a budget
of MAX_BLOCK_WEIGHT minus the header: the transactions in a block,
cmpctblock or blocktxn message are part of one block (for blocktxn, this
does not account for those of the block that are already known), and a
transaction that does not fit in a block cannot be valid.
The P2P message size limit allows blocks of up to 4 MB, which is up to
16M weight, all of which is currently deserialized before validation
rejects it (unless a transaction has more inputs or outputs than fit in
a block). With this change, the weight deserialized from a message is
at most MAX_BLOCK_WEIGHT minus the header.
Messages exceeding the budget are ignored, like other malformed
messages, rather than causing the peer to be disconnected for sending an
invalid block.
c655a696d8
sipa force-pushed on Nov 5, 2026
DrahtBot added the label CI failed on Nov 5, 2026
sipa
commented at 10:44 AM on November 5, 2026:
member
May I dare to NACK this? I understand that keeping the witness data in serialized format considerably improves performance, but as I wrote in #35569 (comment), there sure are hundreds of possibilities for data layouts that may be even more optimized for fast data access.
I consider this PR a bugfix for a significant problem. It doesn't rise to the level of vulnerability that needs immediate fixing and emergency releases, but I do think it's an issue that should be fixed and ideally soon. My goal is not at all to find the optimal solution; the goal is simply to get a sufficient and acceptable solution in, and I think this is one. Providing the ability to experiment with even better representations is what I'd consider a secondary goal here.
That said, I don't think it's far off from what an optimal solution can achieve. In terms of memory usage, it drops from ~28 bytes/WU to 1.6 bytes/WU. Clearly nothing can go below 1 byte/WU. In terms of deserialization performance it's harder to give a lower bound, but I think this brings values to an acceptable level regardless.
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? This would give us the possibility to easily experiment with different data layouts and lead to an even faster implementation?
I think that is exactly what (the first half of) this PR is: the introduction of an abstraction layer for CScriptWitness, sufficient to hide the representation aspects. The later commits then introduce one particular representation that is decent, plus some optimizations to it, and to the P2P deserialization code to improve further upon what become the new worst-case scenarios. Nothing prevents experimenting with better layouts as follow-ups, and the changes here make that easy; this is demonstrated by how narrow the changes in a1cea0d7e024e838be8ea295688693116ec357a1 are.
While I'm sure there are code-level conflicts with with #35569, I don't think those are of a conceptual nature. This PR is about an abstraction at the CScriptWitness level, while yours is about abstraction at the CTransaction level as I understand it. We can do either, or both, and I'm happy to rebase on yours if that were the consensus for going forward (though I personally think the changes here are more urgent). If we do both, in whatever order, it opens up the ability for a CTransaction-wide shared allocation approach (packing everything in 1 or a small number of per-transaction allocations + index structures). I've experimented with those too, getting down to around 1.1 bytes/WU memory usage, but I don't think those benefits are worth gating behind the more involved refactorings needed as we can get 90% of the benefit with a more localized change.
@alexanderwiederin That sounds pretty reasonable; the PR has grown a bit beyond its original intentions and it may be worth splitting up. I'll wait for a few more review comments to see what others think.
ismaelsadeeq
commented at 11:03 AM on November 5, 2026:
member
Concept ACK
DrahtBot removed the label CI failed on Nov 5, 2026
Yudis-bit
commented at 12:44 PM on November 5, 2026:
none
+1 on splitting. The interface abstraction is uncontroversial and can go in first.
The views::take change in 60b2da3cc looks clean, drops the allocations without touching behavior.
DrahtBot requested review from ismaelsadeeq on Nov 5, 2026
purpleKarrot
commented at 7:57 AM on November 6, 2026:
contributor
(the first half of) this PR is: the introduction of an abstraction layer for CScriptWitness, sufficient to hide the representation aspects
Thanks for the explanation @sipa. I think that resolves my concerns. I would still prefer another approach:
Let's agree that encapsulation on Core's vocabulary types provides us a benefit by itself, and that we want to utilize it here.
Let's discuss how the API of Core's vocabulary types should look like.
Let's agree on an how we want to transform the current code base to our desired state (eg use a custom clang-tidy check).
Let's make the transition in a concentrated effort. This step is expected to be invasive.
Show in this PR that changing the data layout is actually not invasive.
Step 1 is the most important one, and I think it does not receive sufficient attention in the current approach. This is where we should discuss how CTxIn exposes CScriptWitness. If we provide a GetScriptWitness() observer that returns CScriptWitness by const reference, we are leaking the implementation details that CTxIn definitely contains a CScriptWitness object. But if that observer returns CScriptWitness by value, we can keep its details internal. This would allow storing a vector<vector<byte>> inside CTxIn and define CScriptWitness as a view (inheriting from ranges::view_interface) of span<const byte> initially, and later change the implementation without affecting the API.
sedited referenced this in commit e71cebc016 on Nov 6, 2026
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2026-11-06 23:51 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
