feat(core/types): fine-grained Body RLP override
#109
Conversation
ARR4N
added
the
Status: 🔴 DO NOT MERGE
…or empty, non-nil optional slice
ARR4N
removed
the
Status: 🔴 DO NOT MERGE
Co-authored-by: Quentin McGaw <[email protected]> Signed-off-by: Arran Schlosberg <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]> Signed-off-by: Arran Schlosberg <[email protected]>
ARR4N
requested review from
qdm12,
a team,
darioush and
michaelkaplan13
and removed request for
a team
Co-authored-by: Quentin McGaw <[email protected]> Signed-off-by: Arran Schlosberg <[email protected]>
There was a problem hiding this comment.
fwiw I prefer the version with the entire body overrides.
Parsing and unparsing RLP seems error prone and complex, the simple version which overrides the entire RLP call seems obviously correct without the need for fuzz tests or reasoning about optional fields. There will be some duplication between block and body, but that doesn't seem like a problem to me.
this said I don't want to block the progress here and we can take whichever approach works to parse the bytes.
| package common | ||
|
|
||
| // PointerTo is a convenience wrapper for creating a pointer to a value. | ||
| func PointerTo[T any](x T) *T { return &x } |
There was a problem hiding this comment.
nit: Should we consider adding new code to separate packages when they are not needed for use in the shared package with geth?
There was a problem hiding this comment.
In the overwhelming number of cases I do this (in the /libevm directory) but see my rationale for adding to common.
Co-authored-by: Quentin McGaw <[email protected]> Signed-off-by: Arran Schlosberg <[email protected]>
| t.Run("", func(t *testing.T) { | ||
| t.Logf("\n%s", pretty.Sprint(body)) |
There was a problem hiding this comment.
nit there is no point running this in a subtest then really
There was a problem hiding this comment.
It limits the effects of t.Fatal and require to only the sub test.
Sorry, I was viewing comments in the file-changed tab so didn't see this. Lets address it in our next meeting because I think it's complex enough to warrant in-person discussion rather than asynchronously. |
## Why this should be merged
Allows for modification of `types.Body` payload data + RLP encoding
without placing the entire RLP burden on the `libevm` user as we did
with `types.HeaderHooks`.
## How this works
RLP encoding of a struct is simply a concatenation of RLP encodings of
fields, encompassed by an RLP "list". The
`AppendRLPFields(rlp.EncoderBuffer, ...)` hook exploits this and plugs
in before all `rlp:"optional"`-tagged fields to allow for inclusion of
any new fields. The `EncoderBuffer` SHOULD be used as the `io.Writer`
passed when encoding each field: `rlp.Encode(buffer, fieldValue)`.
`Body` doesn't have `{En,De}codeRLP` methods so they are implemented to
identically replicate original behaviour when a no-op hook is present.
This pattern is sufficient for the `ava-labs/coreth` modifications of
`Body` but can be modified / extended for more complex scenarios, like
`Header`.
> [!NOTE]
> This PR does not include registration of the hooks as that was not the
initial goal and adding them would create too much PR bloat. There is a
placeholder `var todoRegisteredBodyHooks` global variable that can only
be set in tests.
## How this was tested
- Backwards compatibility: the new methods are fuzzed against a `type
withoutMethods Body` passed directly to `rlp.{En,De}code()`
- `coreth` compatibility: unit test of a local implementation of
`BodyHooks` demonstrating reproducibility of RLP encoding.
---------
Signed-off-by: Arran Schlosberg <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]>
Why this should be merged
Allows for modification of
types.Bodypayload data + RLP encoding without placing the entire RLP burden on thelibevmuser as we did withtypes.HeaderHooks.How this works
RLP encoding of a struct is simply a concatenation of RLP encodings of fields, encompassed by an RLP "list". The
AppendRLPFields(rlp.EncoderBuffer, ...)hook exploits this and plugs in before allrlp:"optional"-tagged fields to allow for inclusion of any new fields. TheEncoderBufferSHOULD be used as theio.Writerpassed when encoding each field:rlp.Encode(buffer, fieldValue).Bodydoesn't have{En,De}codeRLPmethods so they are implemented to identically replicate original behaviour when a no-op hook is present.This pattern is sufficient for the
ava-labs/corethmodifications ofBodybut can be modified / extended for more complex scenarios, likeHeader.Note
This PR does not include registration of the hooks as that was not the initial goal and adding them would create too much PR bloat. There is a placeholder
var todoRegisteredBodyHooksglobal variable that can only be set in tests.How this was tested
type withoutMethods Bodypassed directly torlp.{En,De}code()corethcompatibility: unit test of a local implementation ofBodyHooksdemonstrating reproducibility of RLP encoding.