Skip to content
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

RLP-encoded blob transaction #3345

Closed
wants to merge 1 commit into from
Closed

RLP-encoded blob transaction #3345

wants to merge 1 commit into from

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented May 4, 2023

To evaluate ethereum/EIPs#6985 suggestion, this PR demonstrates the CL-side changes.

  • Fields are TBD: Since I have some questions about the fields (Update EIP-4844: de-sszify spec EIPs#6985 (comment)), the RLP data in this PR is not as same as what Update EIP-4844: de-sszify spec EIPs#6985 proposed.
  • Complexity of the decode function: Given RLP libraries are mature enough (according to EL devs), I think using existing RLP library may be safer than implementing an RLP offset parser in CL specs/clients. This PR extracts an abstract get_blob_versioned_hashes that implementers can use an RLP library to retrieve blob_versioned_hashes.

Declaration: I do not tend to this change. I just show what CL specs will be.

@hwwhww hwwhww added DO NOT MERGE Deneb was called: eip-4844 labels May 4, 2023
@lightclient
Copy link
Member

One idea to alleviate the need for RLP is to extend BlobSidecar with another field blob_versioned_hashes and to verify every KZG commitment matches the specified versioned hash. Then, the blob_versioned_hashes value can be sent to the EL over the Engine API. The EL will then check that each versioned hash matches the versioned hash in the RLP transaction.

@potuz
Copy link
Contributor

potuz commented May 9, 2023

Moving RLP to the CL would be a major pain for Deneb IMO. Matching KZG commitments and sending them over the Engine API has some drawbacks in that we can currently validate execution payload in parallel without having the full sidecar and wait for data validation before we put into forkchoice. Both of these approaches seem pretty dire to me.

@djrtwo
Copy link
Contributor

djrtwo commented May 9, 2023

Engine API has some drawbacks in that we can currently validate execution payload in parallel

So you could do this as well as long as you had an ejection (or filtering) mechanism. Essentially -- you have two flags on the block in the fork-choice (is_available, is_el_valid) and if either are false filter out of fc. I would presume if you are putting things in FC before calls to EL today, then you have something like is_el_valid already. So my main question is how does this additional EL validation change the game that much?

@potuz
Copy link
Contributor

potuz commented May 9, 2023

I would presume if you are putting things in FC before calls to EL today, then you have something like is_el_valid already.

We can't put into forkchoice without the EL returning at least SYNCING or ACCEPTED, so that we need at least a return from the EL before even trying to hit forkchoice. My understanding of Matt's mod to avoid RLP is that either we will need the full sidecar before calling newPayload or we will need an extra call to send the blob_versioned_hashes after we have verified the KZG commitments against the extra field in the sidecar.

@lightclient
Copy link
Member

It doesn't necessarily need to be a part of the full sidecar. I think it could also be an extension on the p2p beacon block. You would pass those blob_versioned_hashes to the EL and get back SYNCING or ACCEPTED (or INVALID if they don't match up), then you'd keep them around on the CL until you receive the full sidecar and can actually validate the blobs.

@djrtwo
Copy link
Contributor

djrtwo commented May 10, 2023

Sorry, I misunderstood when you meant on putting into forkchoice.

I didn't see that @lightclient wanted to extend the blob sidecar with the version hashes. I would put these in the beacon block or execution payload so they are delivered at the same time as the block and can be verified whenever doing any block verifications. The data in a sidecar should not impact the ability to do any state transition computations.

Comment on lines +658 to +660
def get_blob_versioned_hashes(signed_blob_tx_rlp: bytes) -> Sequence[VersionedHash]:
decoded_tx = rlp.decode(signed_blob_tx_rlp, SignedBlobTransaction)
return [VersionedHash(x) for x in decoded_tx.blob_versioned_hashes]
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to use a "neutral" encoding here, that does not add RLP to the CL? It seems like the wrong direction to go with.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://eips.ethereum.org/EIPS/eip-6404

If going with (current) EIP-6404, this function would actually still provide SSZ, as EIP-6404 converts all transactions to a common SSZ representation as part of the block production, regardless of what their original representation used to be.

@djrtwo
Copy link
Contributor

djrtwo commented May 11, 2023

As discussed with @lightclient, the TX version hashes are already "committed to" in the beaconblock due to the explicit list of kzg_commitments along with the function to convert -- kzg_commitment_to_versioned_hash().

Thus the above proposal can be to just compute the list of versioned hashes from the kzg_commitments and then pass them along to the EL via the engine API with an engine-API validation condition to be that this list matches those in the blob TXs

Note, the one downside is that this validation only happens when CL and EL are communicating synchronously and not when CL is optimistic nor when EL is syncing, due to not having this info otherwise available on EL. This is a marginal degredation in the optimistic/syncing security, but I'm not really seeing how it becomes a place of actual exploit. Curious other's input

The alternative would be to move the commitments into the EL such that it just does this vlaidation without CL at all. That said, it breaks the attempted firewall between the CL DA crypto and the EL (although it is already broken in the mempool)

CC: @potuz, @lightclient, @mkalinin

@g11tech
Copy link
Contributor

g11tech commented May 11, 2023

just to add : can we prefix block commitments with the version byte so CL doesn't has to make any assumption to compute versioned hashes

@djrtwo
Copy link
Contributor

djrtwo commented May 11, 2023

just to add : can we prefix block commitments with the version byte so CL doesn't has to make any assumption to compute versioned hashes

I'm not sure this is beneficial. The version commitments are to make it so that the EL doesn't have to care about which commitment/cryptography the CL is using for the blob data

@g11tech
Copy link
Contributor

g11tech commented May 11, 2023

just to add : can we prefix block commitments with the version byte so CL doesn't has to make any assumption to compute versioned hashes

I'm not sure this is beneficial. The version commitments are to make it so that the EL doesn't have to care about which commitment/cryptography the CL is using for the blob data

may be i am missing something here: so CL can independently decide to use a commitment scheme (and derive from blob data) while the original txs were submitted using a different commitment/cryptography?

Also if EL is agnostic about the commitment version, so basically the calldata to the tx will the one making clear to EL which versioned point evaluation pre-compile to use?

OR the cryptography version is decided by hardfork (coordinated on both CL and EL)

@hwwhww
Copy link
Contributor Author

hwwhww commented May 12, 2023

@djrtwo @lightclient

pass them along to the EL via the engine API with an engine-API validation condition to be that this list matches those in the blob TXs

Do you suggest creating a new Engine API interface or re-using the engine_newPayload interface?

Having two API calls within a state transition seems to be a suboptimal design. However, extending engine_newPayload by adding parameters doesn't seem quite fitting, given the term "new_payload".

@lightclient
Copy link
Member

I think the idea is to extend engine_newPayload.

@mkalinin
Copy link
Collaborator

mkalinin commented May 12, 2023

Thus the above proposal can be to just compute the list of versioned hashes from the kzg_commitments and then pass them along to the EL via the engine API with an engine-API validation condition to be that this list matches those in the blob TXs

I don't see any issues with this approach as this check isn't a part of EL block validity conditions. EL should run this check in a similar way as it validates block hash today and respond immediately if it fails.

We have two options: either extend ExecutionPayloadV3 with the new field or define ExecutionPayloadBundleV1 with payload and versionedHashes fields. I am in favour of the former as it makes a clear distinction between fields that are matching on EL block fields and those that are not.

UPD:
The third option is to have hashes as a separate parameter, which satisfies clarity without introducing new data structure.

@hwwhww
Copy link
Contributor Author

hwwhww commented May 15, 2023

@mkalinin

We have two options: either extend ExecutionPayloadV3 with the new field or define ExecutionPayloadBundleV1 with payload and versionedHashes fields. I am in favour of the former as it makes a clear distinction between fields that are matching on EL block fields and those that are not.

did you mean the "latter", which I think has a more clear name?

@mkalinin
Copy link
Collaborator

@mkalinin

We have two options: either extend ExecutionPayloadV3 with the new field or define ExecutionPayloadBundleV1 with payload and versionedHashes fields. I am in favour of the former as it makes a clear distinction between fields that are matching on EL block fields and those that are not.

did you mean the "latter", which I think has a more clear name?

Sorry for confusion, I am more in favour of the option with the hashes as a separate parameter

@mkalinin
Copy link
Collaborator

I've opened a PR with the way it would look like in Engine API ethereum/execution-apis#407

@hwwhww
Copy link
Contributor Author

hwwhww commented May 25, 2023

closing via #3359

@hwwhww hwwhww closed this May 25, 2023
@hwwhww hwwhww deleted the rlp-blob-tx branch May 25, 2023 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deneb was called: eip-4844 DO NOT MERGE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants