fix(crypto): take *types.Transaction in RecoverPublicKey - #567
Open
0xrlawrence wants to merge 1 commit into
Open
0xrlawrence wants to merge 1 commit into
0xrlawrence wants to merge 1 commit into
Conversation
go-ethereum's types.Transaction carries atomic.Pointer caches for the hash,
size and sender, and is documented as non-copyable. Accepting it by value
copied those caches on every call; 'go vet' reports it as a lock copy:
lib/crypto/eth_secp256k1.go:114: RecoverPublicKey passes lock by value:
types.Transaction contains sync/atomic.Pointer[...] contains atomic.noCopy
The only caller is rlpToCanopyTransaction, which handles untrusted inbound
Ethereum-compatibility transactions, so this is on a hot path.
Switching the parameter to a pointer removes the copy and lets
signer.Hash(tx) use the caller's transaction (and its hash cache) directly
instead of a throwaway copy. The caller already has an addressable local, so
it just passes &tx.
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
1 participant
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.
Description
RecoverPublicKeyaccepts go-ethereum'stypes.Transactionby value. That type carriesatomic.Pointercaches for the hash, size and sender, and is documented as non-copyable.go vetreports it:The only caller is
rlpToCanopyTransaction, which handles untrusted inbound Ethereum-compatibility transactions, so this is on a hot path.Changes Made
*types.Transaction.signer.Hash(tx)now hashes the caller's transaction — and can use its hash cache — instead of a throwaway copy.&tx.Testing
go build ./...go test ./lib/crypto/ ./fsm/passesgo vet ./lib/crypto/no longer reports the copylocks finding🤖 Generated with Claude Code