The build failure is due to an eight year old bug in clang: https://bugs.llvm.org/show_bug.cgi?id=8263

I like the goal here but I find the changes a bit overwhelming for achieving this. It seems like we could achieve this more easily in other ways, like (1) just building the hash on demand the first time it's requested, or (2) adding a deserialization flag to CTransaction that instructs it to skip the hash if we're not planning to use it?

[I'm not sure if (1) is a great solution, since there might be some memory-cache-benefits to building the hash at the time of deserialization, and maybe no one likes the design of having a mutable hash inside of CTransaction, but it would be a super-small diff...]

I didn't particularly like option (1) because it makes the caching run-time depended (it is always compiled in, but only used when it happens to be requested), compared to having it compile time enforced by typing CPureTransaction instead of CTransaction.

Note that my approach is a variation of option (2) by passing through the type of the transaction to ReadBlockFromDisk instead of passing through an additional flag that is converted to a deserialization option and then passed on to the constructor of the transaction.

So I think having explicit types is advantageous in both code clarity and cleanness, with the tradeoff of a minimally larger diff.

Note: I will push a fixed up version of the patch by next Monday to minimize the diff and (hopefully) work around the gcc and clang compiler bugs.

Fair enough, I just don't want us to overlook simpler solutions that might be acceptable. I'm happy to let the reviewers with stronger feelings about the design of these data types chime in.

I just pushed a version that might compile on our buggy clang and that is according to the design that @sipa requested in #13011 (comment)

Then at the very least introduce alternate versions of CTransaction/CBlock that don't precompute anything, for use in reindexing and rescanning and serving blocks from disk.

@MarcoFalke Thanks for working on this, I'll review soon.

@sdaftuar My preference is having separate types, for these reasons:

• Having lazily-computed hashes in transactions involves synchronization structures (atomics at least to avoid multiple threads simultaneously getting a hash for the first time).
• Smaller in memory in places where it's not needed.
• We have the option of later moving the with-hash types into consensus code and not exposing it.

This seems like very useful work, but for serving out historic blocks it would perhaps be better if we could (largely) avoid deserializing and re-serializing them entirely as doing so is tremendously expensive (involving a billionity mallocs). Better is better however.

Having lazily-computed hashes in transactions involves synchronization structures (atomics at least to avoid multiple threads simultaneously getting a hash for the first time).

Agree. The main thing I don't like about lazy evaluation is that it's much harder to reason about performance then. I think I prefer the current explicit solution, even though much more verbose.

This seems like very useful work, but for serving out historic blocks it would perhaps be better if we could (largely) avoid deserializing and re-serializing them entirely as doing so is tremendously expensive (involving a billionity mallocs). Better is better however.

Yes - I was also about to say this. How difficult would it be to serve the data directly from disk, without serialization/deserialization roundtrip?

Edit: to do this efficiently would require a change to the on-disk format to add the size, or adding the on-disk block size to the block index. OH this is actually already there, situated before the nPos:

    // Write index header
    unsigned int nSize = GetSerializeSize(fileout, block);
    fileout << messageStart << nSize;

but for serving out historic blocks it would perhaps be better if we could (largely) avoid deserializing and re-serializing them entirely

My dumb try at this: #13151

Even with 13151 something like this might very useful for making rescan faster... rescan is slow enough to make it largely unusable now. I'd been thinking deserialization was worse than hashing in it, but the performance numbers above make me wonder.

Certainly Concept ACK if we can get rescan up for this (even as a separate PR), just using it for serving blocks for old nodes is maybe less worth it (at least post-13151).

Will do a rebase once the other pull is merged

Nice work!
Concept ACK

Any reason not to reuse CMutableTransaction for the purposes where CPureTransaction is introduced here? The only reason why CTransaction is immutable is to not interfere with or require synchronization on its precomputed hashes. As CMutableTransaction has no cashed hashes, this is not an issue.

@sipa I'd prefer to make it "type-safe", i.e. make it impossible/compile-time enforced that none of the expensive operations are called. Otherwise this seems too fragile.

I think to make it compile-time enforced, this would require to change one of transaction types to be a class that takes template arguments and then add a static assert in Get*Hash. I am happy to change to that implementation.

@MarcoFalke But as long as you keep CMutableTransaction around, such a guarantee doesn't really exist, as that class still has uncached hashing operations (and should probably remain so, see a recent PR that speeds up signing by avoiding CMutableTransaction -> CTransaction conversions).

My preference is just to evolve CTransaction into the validation-logic-optimized-efficient transaction representation, and CMutableTransaction into the simple-generic representation. That may mean that uncached hashes are still invoked in places where performance doesn't matter that much.

After discussing this a bit with @MarcoFalke, I see the advantage of being able to enforce through the type system that no GetHash is unexpectedly called on transactions that don't have a cache in certain specific cases (like serving historical blocks).

On the other hand, it's also introducing yet another type for the same data, which seems a high price. As it has const fields, conversion to other transaction types will be expensive if it's ever needed, but it does get the ability to take advantage of more efficient storage if they get implemented (like one malloc per transaction).

Overall, I think my preference is to reuse CMutableTransaction here, but if we want the extra guarantees and an extra type, the current approach seems fine.

• [refactoring] Create common interface for CMutableTransaction and CTransaction with static polymorphism #14337 ([refactoring] Create common interface for CMutableTransaction and CTransaction with static polymorphism by lucash-dev)
• Annotate unused parameters with [[maybe_unused]] #14194 (Annotate unused parameters with [[maybe_unused]] by practicalswift)
• validation: Document where we are intentionally ignoring bool return values from validation related functions #13864 (validation: Document where we are intentionally ignoring bool return values from validation related functions by practicalswift)

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.

Curious why this was closed.

Rebased (with the p2p changes removed) here: #17529