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

preload sender and check for errors #1480

Merged
merged 2 commits into from
Nov 20, 2024
Merged

Conversation

hexoscott
Copy link
Collaborator

No description provided.

@cla-bot cla-bot bot added the cla-signed label Nov 15, 2024
# Conflicts:
#	zk/stages/stage_sequence_execute_transactions.go
@hexoscott hexoscott merged commit b8d3c79 into zkevm Nov 20, 2024
13 of 14 checks passed
@hexoscott hexoscott deleted the fix/invalid-chain-id-for-signer branch November 20, 2024 09:34
afa7789 pushed a commit that referenced this pull request Nov 21, 2024
copy(sender[:], slot.Senders.At(idx))

// now attempt to recover the sender
sender, err := signer.SenderWithContext(cryptoContext, transaction)
Copy link
Collaborator

Choose a reason for hiding this comment

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

pprof shows that this line takes 27% of total time. I run sequencer and benchmark

Copy link
Collaborator

Choose a reason for hiding this comment

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

sender, err := signer.SenderWithContext(cryptoContext, transaction)

Copy link
Collaborator

@doutv doutv Dec 27, 2024

Choose a reason for hiding this comment

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

Related issue:
okx#211

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why attempt to recover sender from public key? there should be sender address in the transaction

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The sender always needs recovering at some point, it isn't part of the RLP for a transaction. Once decoded it is cached on the transaction but until then something needs to recover it.

ids := make([]common.Hash, 0, len(slot.TxIds))
transactions := make([]types.Transaction, 0, len(slot.Txs))
toRemove := make([]common.Hash, 0)
signer := types.MakeSigner(cfg.chainConfig, currentHeight, 0)
cryptoContext := secp256k1.ContextForThread(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use only 1 context for all transactions. This is sequential computation. Why not parallel?

doutv added a commit to okx/xlayer-erigon that referenced this pull request Dec 27, 2024
Revert some changes made in 0xPolygonHermez#1480
@doutv
Copy link
Collaborator

doutv commented Jan 7, 2025

@hexoscott @revitteth
I need your help! Please check my review comment.

I want to know more background about this PR.

  • Why recover sender address from public key? Is it necessary?

Some optimization ideas:

  • Read sender address in slot first, when read failed, fallback to recover sender address
  • Parallel cryptoContext, recover sender address in parallel

My draft PR just remove "recover sender address from public key" and read sender address from transaction slot. Sequencer can be 50% faster under our benchmark.

@hexoscott
Copy link
Collaborator Author

hexoscott commented Jan 7, 2025

The context for this change was that some chains were receiving spurious transactions that looked fine but the R,S,V values on the transaction would not allow for the sender to be recovered which caused an error. So, we pre-load the sender now and check for this error ahead of time to ensure this doesn't happen. We need to do it at this point in the process otherwise the transaction has already been added to the block and it's too late to do anything about it.

Fixing the issue was the priority at the time, if there is degraded performance then I'll raise a new issue with your points above.

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

Successfully merging this pull request may close these issues.

3 participants