Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce allocs signer #31258

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

MariusVanDerWijden
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden commented Feb 25, 2025

This PR roughly halves the number of allocations needed to compute the sigHash for a transaction.
This sigHash is used whenever we recover a signature of a transaction, so quite often. During a recent benchmark full syncing on Holesky, roughly 2.8% of all allocations were happening here because the fields from the transaction would be copied multiple times.

66168733  153175654 (flat, cum)  2.80% of Total
         .          .    368:func (s londonSigner) Hash(tx *Transaction) common.Hash {
         .          .    369:	if tx.Type() != DynamicFeeTxType {
         .          .    370:		return s.eip2930Signer.Hash(tx)
         .          .    371:	}
         .   19169966    372:	return prefixedRlpHash(
         .          .    373:		tx.Type(),
  26442187   26442187    374:		[]interface{}{
         .          .    375:			s.chainId,
   6848616    6848616    376:			tx.Nonce(),
         .   19694077    377:			tx.GasTipCap(),
         .   18956774    378:			tx.GasFeeCap(),
   6357089    6357089    379:			tx.Gas(),
         .   12321050    380:			tx.To(),
         .   16865054    381:			tx.Value(),
  13435187   13435187    382:			tx.Data(),
  13085654   13085654    383:			tx.AccessList(),
         .          .    384:		})
         .          .    385:}

This PR reduces the allocations and speeds up the computation of the sigHash by ~22%, which is quite significantly given that this operation involves a call to Keccak

// BenchmarkHash-8   	  440082	      2639 ns/op	     384 B/op	      13 allocs/op
// BenchmarkHash-8   	  493566	      2033 ns/op	     240 B/op	       6 allocs/op
Hash-8   2.691µ ± 8%   2.097µ ± 9%  -22.07% (p=0.000 n=10)

It also kinda cleans up stuff in my opinion, since the transaction should itself know best how to compute the sighash

Screenshot_2025-02-25_13-52-41

@@ -123,3 +123,16 @@ func (tx *LegacyTx) encode(*bytes.Buffer) error {
func (tx *LegacyTx) decode([]byte) error {
panic("decode called on LegacyTx)")
}

// OBS: This is the post-frontier hash, the pre-frontier does not contain a chainID
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually Post-EIP155

@rjl493456442
Copy link
Member

Let's deploy it on benchmarks before merging it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants