fix: correct implementations of Encodable and Decodable for sidecars#1528
fix: correct implementations of Encodable and Decodable for sidecars#1528
Conversation
a7b7e2c to
1b42a07
Compare
|
nit fixed 🫡
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 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 |
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
left a comment
There was a problem hiding this comment.
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
|
yeah @Rjected the trait impls in the sidecar we should remove |
…lloy-rs#1528) * fix: correct implementations of Encodable and Decodable for sidecars * fix: remove debug
Closes #1499
Motivation
current
EncodableandDecodableimplementations do not RLP encode or RLP decode the sidecar. Instead they encode only the fields, without the header, producing incorrect RLP outputSolution
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