-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: handle optimism deposit transactions on SenderRecovery
stage
#7376
Conversation
/// | ||
/// Returns `None` if the transaction's signature is invalid, see also | ||
/// [Signature::recover_signer_unchecked]. | ||
pub fn recover_signer_unchecked_with_buffer(&self, rlp_buf: &mut Vec<u8>) -> Option<Address> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a mouthful, accepting suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes sense,
need to properly document the expections wrt buffer content, perhaps ideally only use the [len..] slice when hashing
/// | ||
/// Returns `None` if the transaction's signature is invalid, see also | ||
/// [Signature::recover_signer_unchecked]. | ||
pub fn recover_signer_unchecked_with_buffer(&self, rlp_buf: &mut Vec<u8>) -> Option<Address> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this always clear the buffer before returning or clear at the beginning?
imo clear first would be safer
/// | ||
/// Re-uses a given buffer to avoid numerous reallocations when recovering batches. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
depending on where we clear the buffer, we need to document this
/// | ||
/// Returns `None` if the transaction's signature is invalid, see also | ||
/// [Signature::recover_signer_unchecked]. | ||
pub fn recover_signer_unchecked_with_buffer(&self, rlp_buf: &mut Vec<u8>) -> Option<Address> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that or encode_and_recover_unchecked
seem fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
![Screenshot 2024-03-28 at 16 35 54](https://private-user-images.githubusercontent.com/58548332/317787993-dd488242-d29e-4421-ba52-05cee57049f2.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk0NDU0MjksIm5iZiI6MTczOTQ0NTEyOSwicGF0aCI6Ii81ODU0ODMzMi8zMTc3ODc5OTMtZGQ0ODgyNDItZDI5ZS00NDIxLWJhNTItMDVjZWU1NzA0OWYyLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMTMlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjEzVDExMTIwOVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTEzZWJlNzQzYjZhMDRhNzRlNGE3NzEwYmRiN2U1ODU3YjY3YTM1MWExYWRmMWIwZGJmZTNjNzQ0YTFlNTZlNjYmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.R9bCl8Tzdp5sz6UYzBHLnQSDlMCmWYBg2IZs4Vz3Y8w)
pulled this fix into #6151. defo works as metrics shows, sender recovery stage passed!
pub fn encode_and_recover_unchecked(&self, buffer: &mut Vec<u8>) -> 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) | ||
} | ||
|
||
buffer.clear(); | ||
self.transaction.encode_without_signature(buffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a footgun here because op deposit is never encoded.
I think for the sake of consistency we should just encode them as well, even if this means overhead for recovery.
Deposit transactions do not have a signature, and we were trying to recover them during the stage.