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

feat(core/types): Block RLP + Body hooks #105

Draft
wants to merge 2 commits into
base: qdm12/core/types/body-hooks-rlp
Choose a base branch
from

Conversation

qdm12
Copy link
Collaborator

@qdm12 qdm12 commented Jan 16, 2025

Why this should be merged

How this works

How this was tested

Copy link
Collaborator

@darioush darioush left a comment

Choose a reason for hiding this comment

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

unfortunately I think we may need to patch up Body as well, since this type is used in core/rawdb in Read/WriteBody, which we intend to use upstream code for.

The added fields are the same, so I am not sure if there is a clever way to handle them both with one registration, but having overall 3 types we patch in like this:

  • Header
  • Body
  • Block

seems fine to me too.

@@ -309,11 +311,14 @@ func CopyHeader(h *Header) *Header {
cpy.ParentBeaconRoot = new(common.Hash)
*cpy.ParentBeaconRoot = *h.ParentBeaconRoot
}
if h.extra != nil {
cpy.extra = h.extra
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 this copy should be a deep copy so likely adding a method to header hooks is needed.

@qdm12
Copy link
Collaborator Author

qdm12 commented Jan 17, 2025

unfortunately I think we may need to patch up Body as well

EDIT: you're right, never mind! Working on it!

Doesn't https://github.com/ava-labs/coreth/pull/750/files#diff-e177ddb5610d1f3426c15cb512999613785b20838954e48187883e6263bb282bR45 suffice? 🤔

Or at least for now, we can then do a PR for the body after to keep things tidy and one-step-at-a-time?

@qdm12 qdm12 force-pushed the qdm12/core/types/block-hooks-rlp branch from f3bb9ed to dc9e4f8 Compare January 17, 2025 13:46
@darioush
Copy link
Collaborator

darioush commented Jan 17, 2025

Or at least for now, we can then do a PR for the body after to keep things tidy and one-step-at-a-time?

We can merge 1 PR at a time to libevm, but I'd like to close the loop on the whole point of this exercise which is to be able to drop the core/rawdb package, to ensure there are not any more surprises that would cause re-work.

We can make the next PR to this branch if you think it's cleaner that way, so we can continue verifying the integration with coreth/subnet-evm contains everything we need before fully fleshing out tests & solidifying the changes.

@qdm12 qdm12 force-pushed the qdm12/core/types/block-hooks-rlp branch 7 times, most recently from 5ea4a39 to 463996d Compare January 21, 2025 16:47
@qdm12 qdm12 changed the title feat(core/types): block hooks for rlp encoding feat(core/types): Block RLP codec hooks Jan 21, 2025
@qdm12 qdm12 changed the base branch from main to darioush/libevm-phase-2.5 January 21, 2025 17:46
@qdm12 qdm12 force-pushed the qdm12/core/types/block-hooks-rlp branch from 463996d to abbba71 Compare January 22, 2025 09:03
@qdm12 qdm12 changed the base branch from darioush/libevm-phase-2.5 to qdm12/core/types/body-hooks-rlp January 22, 2025 09:04
@qdm12 qdm12 force-pushed the qdm12/core/types/body-hooks-rlp branch from e7193b7 to 7f6afca Compare January 22, 2025 09:40
@qdm12 qdm12 changed the title feat(core/types): Block RLP codec hooks feat(core/types): Block RLP + Body hooks Jan 22, 2025
@qdm12 qdm12 force-pushed the qdm12/core/types/block-hooks-rlp branch from 818f7ab to 11c780f Compare January 22, 2025 09:50
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