fix: correct implementations of Encodable and Decodable for sidecars

[prestwich]

Closes #1499

Motivation

current Encodable and Decodable implementations do not RLP encode or RLP decode the sidecar. Instead they encode only the fields, without the header, producing incorrect RLP output

Solution

Current behavior has been moved into more-explicitly named functions and is used to construct correct RLP implementations

This is a breaking change as things that previously (incorrectly) deserialized now will not

PR Checklist

• Added Tests
• Added Documentation
• Breaking changes

[onbjerg]

smol nit,

do we not use this in reth right now? @Rjected or @mattsse

if so, any reason for the encoding being the way it is? i assume we use this for network

[prestwich]

nit fixed 🫡

if so, any reason for the encoding being the way it is? i assume we use this for network

It appears to have been a hack to deal with the eip4844 2718 encoding when the sidecar is present. See #1496 for more cleanup around these APIs (this bug was found while working on that). The sketch is that 4844 specifies that the 2718 payload is rlp([tx_payload_body, blobs, commitments, proofs]) in the network. Each field is encoded in that list. This PR makes that behavior an explicit, separate, non-trait function

More interestingly, do we have any use case in which a sidecar itself should be RLP encoded at all? We have to RLP encode it's fields, but when is a standalone sidecar rlp encoded or included as a sublist in another list? We could just delete these impls if there's not a use case

[onbjerg]

More interestingly, do we have any use case in which a sidecar itself should be RLP encoded at all?

not that I can recall, overall this PR looks good to me, but I'm wary of merging it mostly because I don't have a full overview of 4844 in reth, so I'd like to get either @mattsse or @Rjected to take a look since they worked on this and might know if there is such a usecase

[prestwich]

not that I can recall, overall this PR looks good to me, but I'm wary of merging it mostly because I don't have a full overview of 4844 in reth, so I'd like to get either @mattsse or @Rjected to take a look since they worked on this and might know if there is such a usecase

In the interest of fixing the logic bug, can we merge this as is, without deleting the impls? Reth's tests will easily flag reliance on the current faulty coding logic, and it's trivial to fix when reth bumps to a new alloy version

[Rjected]

Yeah, we should definitely make the no-header functions not Encodable / Decodable fns. Sidecars are basically never rlp encoded on their own. We can easily switch over the decoding fn to call, and we have blob tx encoding tests that would catch the behavior change

[mattsse]

yeah @Rjected the trait impls in the sidecar we should remove