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

fix: use body.recover_signers_unchecked instead on try_with_senders_unchecked #12668

Merged
merged 2 commits into from
Nov 20, 2024

Conversation

joshieDo
Copy link
Collaborator

@joshieDo joshieDo commented Nov 19, 2024

alternative to #12666

Block::try_with_senders_unchecked was not using an unchecked recovery method

@joshieDo joshieDo added C-bug An unexpected or incorrect behavior A-db Related to the database labels Nov 19, 2024
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

makes sense

@alessandromazza98
Copy link
Contributor

I'm checking if this fixes my problem, again thanks a lot for being so fast to reply. will update here shortly

@alessandromazza98
Copy link
Contributor

alessandromazza98 commented Nov 19, 2024

with this PR I still have the same problem, with my (#12666) PR i don't have it

@joshieDo
Copy link
Collaborator Author

joshieDo commented Nov 20, 2024

SealedBlock which is used in this request actually needed to be changed as well (not just Block). Can you try again?

Tbh, I'm still unsure on what causes the issue. It should not matter if it's unchecked or not, since TxDeposit are "caught" on :

pub fn recover_signer(&self) -> Option<Address> {
// Optimism's Deposit transaction does not have a signature. Directly return the
// `from` address.
#[cfg(feature = "optimism")]
if let Transaction::Deposit(TxDeposit { from, .. }) = self.transaction {
return Some(from)
}
let signature_hash = self.signature_hash();
recover_signer(&self.signature, signature_hash)
}

@joshieDo
Copy link
Collaborator Author

merging since this still makes sense on its own

@joshieDo joshieDo added this pull request to the merge queue Nov 20, 2024
Merged via the queue into main with commit 10caa9f Nov 20, 2024
40 checks passed
@joshieDo joshieDo deleted the joshie/unchecked branch November 20, 2024 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-db Related to the database C-bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants