core/types,node/types: add cached tx hash to ktypes.Transaction, create node/types.Tx #1422
Conversation
This file contains 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
jchappelow
commented
Feb 26, 2025
| @@ -34,7 +34,7 @@ func (ce *ConsensusEngine) validateBlock(blk *ktypes.Block) error { | |||
| } | |||
|
|
|||
| // Verify the merkle root of the block transactions | |||
| merkleRoot := blk.MerkleRoot() | |||
| merkleRoot := blk.CalcMerkleRoot() // NOTE: this expects CalcMerkleRoot to use tx.HashCache() to prepare the Transaction's internal hash cache | |||
There was a problem hiding this comment.
validateBlock is a key method that is one of the first points at which a node will begin processing a block it has received, e.g. from p2p, block store, etc. Ensuring that this method warms the internal tx hash cached in core/types.Transaction benefits many other spots downstream.
jchappelow
commented
Feb 26, 2025
Comment on lines
-126
to
-127
| txHash := tx.Hash() | ||
| have, rejected := ce.mempool.Store(txHash, tx) |
There was a problem hiding this comment.
This silly optimization of having mempool.Store take a hash and the transaction separately was silly and now obsolete.
jchappelow
force-pushed
the
txhashcache
branch
from
b74b231 to
ce5c5de
Compare
charithabandi
approved these changes
Feb 27, 2025
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.
This updates
core/types.Transactiona.k.a.ktypes.Transationwith an unexportedcachedHashfield, which is only populated or referenced via the newHashCachemethod, rather than modifying theHashmethod to do this automatically. A few reasons are outlined in the docs, but they include concurrency concerns, needless allocs in many cases, the tx mutability gotchas particularly in client side applications.This also removes the
node/types.NamedTxwhich was used in a handful of places to pair the hash with thecore/types.Transaction, replacingNamedTxwithnode/types.Txfor brevity. This new types adds a constructorNewTxthat computes the has on construction. TheHash()method of this type shadows the embeddedtypes.Transaction's method to return this stored hash. This should be less likely to become an issue since it is "internal" to thenode/...packages, and the hash being computed in the constructor avoids any concurrency concerns. Plus it is documented explicitly as an immutable transaction type.The
node/types.Txwith it's construction-time hash storage is primarily used in mempool and the CE.The
HashCachemethod is used in other places that deal with acore/types.Block, which is necessarily defined in terms of acore/types.Transaction.