Skip to content

Conversation

ARR4N
Copy link
Collaborator

@ARR4N ARR4N commented Dec 12, 2024

Why this should be merged

The types.Header fields of both coreth and subnet-evm have been modified such that their RLP encodings (i.e. block hashes) aren't compatible with vanilla geth nor each other. This PR adds support for arbitrary RLP encoding coupled with type-safe extra payloads.

How this works

Equivalent to #1 (params) and #44 (types.StateAccount) registration of pseudo-generic payloads. The only major difference is the guarantee of a non-nil payload pointer, which means that the payload hooks are never called on nil pointers as this would make it difficult to decode RLP into them.

How this was tested

Round-trip RLP {en,de}coding via a registered stub hook.

@ARR4N ARR4N mentioned this pull request Dec 12, 2024
@ARR4N ARR4N marked this pull request as ready for review December 12, 2024 15:01
@ARR4N ARR4N requested review from a team, darioush, michaelkaplan13 and qdm12 and removed request for a team December 12, 2024 15:01
defer TestOnlyClearRegisteredExtras()

extras := RegisterExtras[stubHeaderHooks, *stubHeaderHooks, struct{}]()
rng := ethtest.NewPseudoRand(13579)
Copy link

@qdm12 qdm12 Dec 12, 2024

Choose a reason for hiding this comment

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

Any particular reason for 13579? Or just smashed digits on the keyboard? ⌨️ I feel we should really just seed everything with 0 - if we want to test with various random values, we should fuzz instead 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I feel we should really just seed everything with 0

If there's a particular reason then I'm happy to consider it as the only reason I choose numbers is for some fun. Practically I think it's all the same, no?

Copy link

@qdm12 qdm12 Dec 17, 2024

Choose a reason for hiding this comment

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

It does look like a magic number although it's not, at least with 0 it does look like a this-number-does-not-matter-see-I-m-using-zero. My mind might had been trained too much by the https://github.com/tommy-muehle/go-mnd linter to be fair 😄 What do you think?

Maybe we could address those in a chore PR later finding all numbers with a regex, I can create a super low priority issue for this 😸

@ARR4N ARR4N merged commit dc7e27a into main Dec 17, 2024
4 checks passed
@ARR4N ARR4N deleted the arr4n/header-rlp-hooks branch December 17, 2024 13:55
ARR4N added a commit that referenced this pull request Dec 19, 2024
## Why this should be merged

JSON equivalent of #89.

## How this works

The check for registered extras, previously used in `{En,De}codeRLP()`
methods is abstracted into a `Header.hooks() HeaderHooks` method that
either returns (a) an instance of the registered type or (b) a
`NOOPHeaderHooks` if no registration was performed. This is then used
for all hooks, new (JSON) and old (RLP).

## How this was tested

Extension of existing unit tests.
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.

3 participants