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

primitives: use alloy Receipts #12059

Closed
wants to merge 8 commits into from

Conversation

tcoratger
Copy link
Contributor

@tcoratger tcoratger commented Oct 24, 2024

Ref #7238

@tcoratger tcoratger marked this pull request as ready for review October 26, 2024 21:44
@@ -17,12 +17,10 @@ where
&mut self,
first_tx_index: TxNumber,
_: BlockNumber,
receipts: Vec<Option<Receipt>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this would break receipt pruning during staged sync

Copy link
Collaborator

Choose a reason for hiding this comment

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

and the fact that all tests pass, probably means we should create a test to ensure this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so you mean that for this specific function we should keep Vec<Option<Receipt>> or does it affect also other methods? In summary you want to keep this behavior right? (Was thinking that this was equivalent to simply have no receipt)

If receipt is None, it has been pruned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshieDo This means also that in ExecutionOutcome, we cannot use Receipts directly here no?

Because with Receipts, we have a vec[vec[Receipt]] but here you mention the need for vec[vec[Option<Receipt>]] due to staged sync pruning flags no?

Copy link
Collaborator

@joshieDo joshieDo Oct 29, 2024

Choose a reason for hiding this comment

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

we should keep Vec<Option>

yes. im not sure on effects on other methods (cc @shekhirin )

But maybe we could just stick to the PR's goal of replacing the type and not touch anything else? Or is this somehow required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the initial goal of this PR was indeed to replace the Receipts type with its alloy equivalent but as discussed here alloy-rs/alloy#1247 (review), in alloy we no longer have an Option around the Receipt natively inside Receipts.

To try to do things little by little I just created another PR #12162 to minimize the changes with:

pub type Receipts = alloy_consensus::Receipts<Option<Receipt>>;

What I propose:

cc @joshieDo @mattsse @shekhirin

Copy link
Collaborator

Choose a reason for hiding this comment

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

I misread this as replacing Receipt and not Receipts, my bad.

Maybe we should tacke first the pruning rather the type replacement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshieDo As you prefer, I think both ways are fine:

  • We could maybe first merge primitives: use alloy Receipts with Option #12162 first so that we can play easier with the Option or not because the alloy Receipts<T> has the generic type T so that maybe we can replace step by step.
  • Also it would maybe allow to directly focus on alloy and not cause additional regression on reth.
  • But if you don't agree no worries, we can first fix the pruning and then merge primitives: use alloy Receipts #12059

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshieDo @mattsse After rethinking about this, maybe this is simpler to merge #12162 first and then we can play with the new type at the required places, progressively, by doing, at choice:

  • alloy::Receipts<Option<Receipt>>
  • alloy::Receipts<Receipt>

depending on the locations.

In this way we could progress slowly, what do you think?

@tcoratger
Copy link
Contributor Author

Closed with #12162

@tcoratger tcoratger closed this Oct 30, 2024
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.

2 participants