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
feat: Implement BlockReader::block_with_senders_range #7402
Conversation
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.
ptal @joshieDo
crates/rpc/rpc/src/trace.rs
Outdated
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.
please keep this pr limited to adding the new function
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.
Done.
let len = range.end().saturating_sub(*range.start()) as usize; | ||
let mut blocks = Vec::with_capacity(len); | ||
|
||
let mut headers_cursor = self.tx.cursor_read::<tables::Headers>()?; |
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.
Done.
let senders = senders_cursor | ||
.walk_range(tx_range)? | ||
.map(|entry| entry.map(|(_, sender)| sender)) | ||
.collect::<Result<Vec<_>, _>>()?; |
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.
senders might be missing from the table if they are pruned, there other provider functions that can be used to get them, so you might need to do a mix of fetching what you can, and recover the rest (take a look at senders_by_tx_range
)
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.
otherwise, if they are pruned try_with_senders_unchecked
will be recalculating every sender from the body, since txes.len != senders.len
If the above comment suggestion seems too complex, you could just let try_with_senders_unchecked
calculate the senders by passing an empty vec. It can be improved on another PR
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.
Never mind, will give it a try later.
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.
Done.
ping @joshieDo |
for (tx_num, tx) in tx_range.zip(body.iter()) { | ||
match known_senders.get(&tx_num) { | ||
None => { | ||
// recover the sender from the transaction if not found | ||
let sender = tx | ||
.recover_signer_unchecked() | ||
.ok_or_else(|| ProviderError::SenderRecoveryError)?; | ||
senders.push(sender); | ||
} | ||
Some(sender) => senders.push(*sender), |
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 might be an improved performance route here by collecting all txes ref in a list and recover all signers in parallel, but can be done in a follow-up imo
.map(|(_, o)| o.ommers) | ||
.unwrap_or_default() | ||
}; | ||
if let Ok(b) = process_tx(tx_range, header, ommers, withdrawals) { |
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.
process_tx is a weird name here tho
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.
updated
@mattsse PTAL |
close #7395