perf(subtree data): keep the transaction id that deserialization already computed - #160
Merged
mrz1836 merged 1 commit intoSep 9, 2026
Conversation
…ady computed serializeFromReader hashes every transaction it reads so it can check the id against the node hash the subtree already holds. That check is necessary. What was wasteful is that the computed id was then discarded. bt.Tx.TxIDChainHash reads the transaction's cache but never fills it; only SetTxHash does, which go-bt documents in as many words. So every later caller serialized the whole transaction again and hashed it again. Measured on a Teranode node reading a mainnet block, the two halves cost almost exactly the same. Of 19.28 core-seconds spent in TxIDChainHash across the whole process, 10.10 were the check here and 9.17 were one downstream stage recomputing the identical values on the identical objects. Reading a 4,096-transaction subtree and then asking each transaction for its id, which is what the next stage does: before 4.75 ms 5,734,857 B 172,037 allocs after 3.80 ms 4,423,872 B 163,845 allocs The 8,192 allocations are exactly two per transaction: the serialization buffer and the hash. What is left is the deserialization itself, which has to happen. Storing the id is safe. It has just been verified against the node hash the subtree carries, and a transaction's id is defined over its standard serialization, so a caller that goes on to extend these transactions, filling in each input's parent satoshis and locking script, does not change it. TestCachedIDSurvivesExtension holds go-bt to that by recomputing from scratch after extending rather than reading the cache back. The coinbase gets the same treatment on its own branch, where there is no node hash to check against because node zero is the coinbase placeholder. One transaction in four thousand does not pay for itself; a uniform contract does. "Every transaction this returns knows its id" is something a caller can rely on, where "every transaction except the coinbase" is a footnote nobody reads. That branch needs a hand-built stream to reach, because Serialize deliberately omits the coinbase, so the test builds one. The property is asserted by allocation count rather than by inspecting the cache, since go-bt exports no predicate for it and allocations are what the caller feels. All three tests were mutation-checked: removing either SetTxHash fails the matching test, and storing a wrong id fails the identity check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
fork-pr
Contributor
👋 Thanks, @freemans13!This pull request comes from a fork. For security, our CI runs in a restricted mode.
Thanks for contributing to bsv-blockchain/go-subtree! 🚀 |
|
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
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.
What happened
serializeFromReaderhashes every transaction it reads, so it can check the id against the node hash the subtree already holds. That check is necessary. What was wasteful is that the computed id was then thrown away.bt.Tx.TxIDChainHashreads the transaction's cache but never fills it. OnlySetTxHashdoes, and go-bt documents this in as many words: "TxIDChainHash itself does not populate the cache; only SetTxHash does." So every later caller serialized the whole transaction again and hashed it again.Measured on a Teranode node reading a mainnet block, the two halves cost almost exactly the same. Of 19.28 core-seconds spent in
TxIDChainHashacross the whole process, 10.10 were the check here and 9.17 were one downstream stage recomputing the identical values on the identical objects.The change
One line on each of the two paths: hash once, check it, keep it.
Reading a 4,096-transaction subtree and then asking each transaction for its id, which is what the next stage does:
The 8,192 allocations saved are exactly two per transaction: the serialization buffer and the hash. What remains is the deserialization itself, which has to happen.
Why storing it is safe
The id has just been verified against the node hash the subtree carries, so it is known correct at the moment it is stored.
A transaction's id is defined over its standard serialization. A caller that goes on to extend these transactions, filling in each input's parent satoshis and locking script, does not change it.
TestCachedIDSurvivesExtensionholds go-bt to that, and it recomputes from scratch after extending rather than reading the cache back, or it would be checking nothing.The coinbase
It gets the same treatment on its own branch, where there is no node hash to check against because node zero is the coinbase placeholder.
One transaction in four thousand does not pay for itself. A uniform contract does: "every transaction this returns knows its id" is something a caller can rely on, where "every transaction except the coinbase" is a footnote nobody reads.
That branch needs a hand-built stream to reach, because
Serializedeliberately omits the coinbase, so the test builds one.Test plan
TestDeserializedTransactionsCarryTheirID— asking a returned transaction for its id allocates nothingTestDeserializedCoinbaseCarriesItsID— same, on the coinbase branchTestCachedIDSurvivesExtension— extending a transaction does not change its idBenchmarkDeserializeThenIdentify— the numbers abovego test -race ./...green,golangci-lint run ./...cleanThe property is asserted by allocation count rather than by inspecting the cache, since go-bt exports no predicate for it, and allocations are what the caller feels.
All three tests were mutation-checked. Removing either
SetTxHashfails the matching test, and storing a wrong id fails the identity check.🤖 Generated with Claude Code