Skip to content

refactor(core/types): simplify Body RLP override #120

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

Merged
merged 13 commits into from
Feb 7, 2025

Conversation

ARR4N
Copy link
Collaborator

@ARR4N ARR4N commented Feb 6, 2025

Why this should be merged

Simplification of types.Body RLP overriding, resulting in reduced code at both the implementation and consumer ends.

How this works

Introduction of rlp.Fields type, to mirror regular RLP encoding of a struct. The RLP override hook now only needs to return the fields of interest, which MAY come from either the Body or the registered extra.

This pattern allows for arbitrary modification of upstream fields via (1) reordering; (2) addition; (3) deletion; and (4) inverting required vs optional status. While less important for Body, this allows for complete support of ava-labs/coreth Header modifications, which make use of 1-3.

How this was tested

Existing backwards-compatibility tests + new unit tests for introduced functionality.

@ARR4N ARR4N force-pushed the arr4n/rlp-field-encoding branch from dbc7ce0 to 64d39ca Compare February 6, 2025 12:43
@ARR4N ARR4N marked this pull request as ready for review February 6, 2025 12:56
@ARR4N ARR4N requested review from a team, darioush, qdm12 and michaelkaplan13 and removed request for a team February 6, 2025 12:56
&b.Transactions,
&b.Uncles,
&e.Version,
rlp.Nillable(&e.ExtData), // equivalent to `rlp:"nil"`
Copy link

@darioush darioush Feb 6, 2025

Choose a reason for hiding this comment

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

if we are adding something like rlp.Nillable, does it make sense to also add rlp.Optional?
It seems like a better callsite experience than returning 2 []any

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 thought of this but then got stuck on what the rest of the API looks like (e.g. what does EncodeStructField() accept? Do non-optional fields also have to be wrapped in something?). I'll ponder it a bit more, experiment with the rest of the code, and get back to you.

Copy link
Collaborator Author

@ARR4N ARR4N Feb 7, 2025

Choose a reason for hiding this comment

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

Done. WDYT? It feels a little bit clunky inside the implementation (hence the explanatory comment), but so much clearer here so I completely agree that:

It seems like a better callsite experience than returning 2 []any

When I originally just introduced type Optional []any, so the usage would have been rlp.Optional{a,b,c} (instead of parentheses), it still compiled with just []any. That's why I introduced the OptionalFields type, to force the clearer call sites.

Copy link

Choose a reason for hiding this comment

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

Now that we have gone this far, what do you think about adding rlp.Required as well? So the callsite can look like:

return rlp.Required(b.Transactions, b.Uncles), rlp.Optional(b.Withdrawals)

instead of:

return []any{b.Transactions, b.Uncles}, rlp.Optional(b.Withdrawals)

Copy link
Collaborator Author

@ARR4N ARR4N Feb 7, 2025

Choose a reason for hiding this comment

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

It became unwieldy and ugly so I did what I should've done all along and changed it to:

return &rlp.Fields{
  Required: []any{b.Transactions, b.Uncles},
  Optional: []any{b.Withdrawals},
}

Everything is much cleaner now; call sites and implementation.

Copy link

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

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

Pretty cool 👍 !

@ARR4N ARR4N requested review from darioush and qdm12 February 7, 2025 10:57
@ARR4N ARR4N merged commit d210cc4 into main Feb 7, 2025
4 checks passed
@ARR4N ARR4N deleted the arr4n/rlp-field-encoding branch February 7, 2025 15:42
ARR4N added a commit that referenced this pull request Feb 13, 2025
## Why this should be merged

Support for configurable `core/types.Block` with RLP encoding, including
interplay with `Body`.

## How this works

`Block` doesn't export most of its fields so relies on an internal type,
`extblock`, for RLP encoding. This type is modified to implement the
`rlp.Encoder` and `Decoder` methods as a point to inject hooks using
`rlp.Fields` (as in #120 for `Body`).

`Block` shares the same registered extra type as `Body`. Unlike
`Header`, which has its own field in a `Block`, the fields in `Body` are
promoted to be carried directly. This suggests that (at least for pure
data payloads) the modifications might be equivalent (and
`ava-labs/coreth` evidences this). Should different payloads be
absolutely required in the future, we can split the types—the
`RegisterExtras` signature is already too verbose though 😢.

## How this was tested

Explicit inclusion of a backwards-compatibility test for
`NOOPBlockBodyHooks` + implicit testing via the multiple upstream tests
in `block_test.go`. Re implicit testing: default behaviour is now to use
the noop hooks even when no registration is performed, but if we change
this then the tests in `block_test.go` can still be called as subtests
from a test that explicitly registers noops.

---------

Signed-off-by: Arran Schlosberg <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]>
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