Add engine_getPayloadBodiesBy*V2 methods#9674
Conversation
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
mirgee
force-pushed
the
engine-payload-bodies
branch
from
b55a6b3 to
a5f5c28
Compare
daniellehrner
left a comment
daniellehrner
left a comment
There was a problem hiding this comment.
I have some clarifying questions that might be fine. There is some code duplication, but we could clean that up in a future PR.
|
|
||
| public class EngineGetPayloadBodiesByHashV2 extends ExecutionEngineJsonRpcMethod { | ||
|
|
||
| private static final int MAX_REQUEST_BLOCKS = 1024; |
There was a problem hiding this comment.
This limit seems not to be in the spec, how do other clients handle that?
There was a problem hiding this comment.
The specs define only the lower bound, it seems that it's up to clients to define the upper bound on the range:
Client software MUST support count values of at least 32 blocks. The call MUST return -38004: Too large request error if the requested range is too large.
| } | ||
|
|
||
| private static String encodeBlockAccessList(final BlockAccessList blockAccessList) { | ||
| final BytesValueRLPOutput output = new BytesValueRLPOutput(); |
There was a problem hiding this comment.
How are block access lists stored? It seems we are not storing them as RLP and need to encode them every single time. Why not store them as RLP directly?
There was a problem hiding this comment.
BAL is RLP encoded. So in this case we are decoding BAL from RLP and encode it again in order to return it. clearly not optimized
There was a problem hiding this comment.
We are storing them in RLP, decoding them every time on retrieval, and then sometimes re-encode them back to RLP when necessary. I agree this redundant round-trip is not ideal in those cases, but it is consistent with how block headers, block bodies and transaction receipts are currently handled.
| return new JsonRpcSuccessResponse(reqId, engineGetPayloadBodiesResultV2); | ||
| } | ||
|
|
||
| protected int getMaxRequestBlocks() { |
There was a problem hiding this comment.
Those methods are duplicated and are the same in EngineGetPayloadBodiesByHashV2
There was a problem hiding this comment.
I agree there is a lot of duplication overall. Happy to clean it up in a follow-up PR 😇
There was a problem hiding this comment.
Can we create an issue for that @mirgee . I will merge the branch
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
3 participants
Adds the following two methods, which include BALs as part of the payload bodies, to the Engine API as per the proposed spec changes:
engine_getPayloadBodiesByHashV2engine_getPayloadBodiesByRangeV2Duplication was chosen over reuse for ease of implementation and simplicity (a lot of code and tests have been copied from
engine_getPayloadBodiesBy*V1), but deduplication refactor is certainly possible in a follow up PR.Closes: #9662