Skip to content

Conversation

7suyash7
Copy link

Leverages pre-encoded bytes from FlashbotsBundle to skip redundant transaction encoding. This improves performance, especially for the OP-Stack.

Signed-off-by: 7suyash7 <suyashnyn1@gmail.com>
Copy link
Collaborator

@julio4 julio4 left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, LGTM!
Can you fix CI, #39 changed bundle is_optional and is_allowed_to_fail to take &BundleHash

Signed-off-by: 7suyash7 <suyashnyn1@gmail.com>
@7suyash7
Copy link
Author

Thank you for the contribution, LGTM! Can you fix CI, #39 changed bundle is_optional and is_allowed_to_fail to take &BundleHash

Thank you and Done!

Copy link
Contributor

@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.

closes #40

Comment on lines +115 to +123
fn executables<'a>(
&'a self,
) -> Box<
dyn Iterator<Item = WithEncoded<&'a Recovered<types::Transaction<P>>>> + 'a,
> {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you can even use impl Iterator here

but unsure if this trait should be object safe

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think impl Iterator is good.
To keep Bundle object safe, we can consider moving it in a BundleExt in payload::ext

Copy link
Author

Choose a reason for hiding this comment

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

Hi, I tried the impl Iterator and BundleExt suggestions, but hit a compiler error due to different iterator types between the optimised and fallback paths.

I switched to using a GAT on the Bundle trait instead. Let me know what you think of this approach.

/// By default, this wraps the plain `Recovered` transactions. Implementors
/// that store pre-encoded bytes can override this to provide the more
/// efficient `WithEncoded` wrapper.
fn executables<'a>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

As this could be slightly confusing with the concept of Executable in the payload API, we could rename this function to something like fn transactions_encoded instead

Comment on lines +115 to +123
fn executables<'a>(
&'a self,
) -> Box<
dyn Iterator<Item = WithEncoded<&'a Recovered<types::Transaction<P>>>> + 'a,
> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think impl Iterator is good.
To keep Bundle object safe, we can consider moving it in a BundleExt in payload::ext

Signed-off-by: 7suyash7 <suyashnyn1@gmail.com>
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.

4 participants