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

Update EIP-4844: Specify precompile input's z and y to be encoded as big endian #7020

Merged
merged 4 commits into from
Jun 7, 2023

Conversation

g11tech
Copy link
Contributor

@g11tech g11tech commented May 12, 2023

Following a discussion ACD on may 11, it was decided to specify z and y in pre-compile input to be big endian so that there is minimal confusion on EL which primarily encodes bytes as big endian.

Ref: https://discord.com/channels/595666850260713488/745077610685661265/1106250714415648849

Corresponding PR in the consensus specs detailing polynomial commitments:

@g11tech g11tech requested a review from eth-bot as a code owner May 12, 2023 13:10
@github-actions github-actions bot added c-update Modifies an existing proposal s-review This EIP is in Review t-core labels May 12, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented May 12, 2023

✅ All reviewers have approved.

@lightclient
Copy link
Member

lightclient commented May 15, 2023

So it turns out that the blake2 (EIP-152) precompile accepts little endian encoding, so maybe we're being too pedantic?

@g11tech g11tech force-pushed the bigendian-precompile-input branch from be7d957 to 9537907 Compare May 15, 2023 13:19
EIPS/eip-4844.md Outdated
@@ -64,7 +64,7 @@ Compared to full data sharding, this EIP has a reduced cap on the number of thes
| Type | Base type | Additional checks |
| - | - | - |
| `BLSFieldElement` | `uint256` | `x < BLS_MODULUS` |
| `Blob` | `Vector[BLSFieldElement, FIELD_ELEMENTS_PER_BLOB]` | |
| `Blob` | `Vector[BLSFieldElement, FIELD_ELEMENTS_PER_BLOB]` | Each field element encoded as padded 32 bytes big endian |

Choose a reason for hiding this comment

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

the way forward here would be the same as any other BLS data: treat it as a black-boxed array of bytes and specify the serialization elsewhere (rather than bending the ssz interpretation etc)

Copy link
Contributor Author

@g11tech g11tech May 17, 2023

Choose a reason for hiding this comment

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

fair point, actually its uint256 list/array so so talking about big endian encoding is irrelevant.

and with now the rlp serialization, it will anyway serialize it big endian for EL layer

have remove the extra meta text

Choose a reason for hiding this comment

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

well, BLSFieldElement in that world needs to be Bytes32 - uint256 would imply a SSZ "number" which is little endian.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's indeed how it's defined in consensus-specs: https://github.com/ethereum/consensus-specs/blob/dev/specs/deneb/polynomial-commitments.md#custom-types

This might be one of the desync issues between EIP and consensus-specs.

Am I right that the Blob struct is currently unused in the EIP?

Copy link
Contributor Author

@g11tech g11tech May 24, 2023

Choose a reason for hiding this comment

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

Blob right now in EIP (mostly an EL specification) is only used for network tx format correctness verification via kzg libs and so is the case in CL.

even for DAS underlying libs could take an input and give erasure code blobs as output
for Custody game, i don't see why CLs need to interpret blob data as BlsFieldElements for generating merkle proofs, so again their its just blob data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, BLSFieldElement in that world needs to be Bytes32 - uint256 would imply a SSZ "number" which is little endian.

Well Uint256 does imply a number which is the intend as its a point on the BLS curve, but i don't think it implies encoding format case in point the below lines from the EIP which were anyway previously the part of EIP.

    # Return FIELD_ELEMENTS_PER_BLOB and BLS_MODULUS as padded 32 bytes big endian values
    return Bytes(U256(FIELD_ELEMENTS_PER_BLOB).to_be_bytes32() + U256(BLS_MODULUS).to_be_bytes32())

But anyway I think this extra clarification I added in a separate PR takes care of any confusion:
https://github.com/ethereum/EIPs/pull/7038/files#diff-c72ce77807acfea9e9693b7999f247841df938c9645be37013903805fa8b0c09R283

Choose a reason for hiding this comment

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

Copy link
Contributor

@hwwhww hwwhww Jun 7, 2023

Choose a reason for hiding this comment

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

Probably not in this PR, but can we use CL format ByteVector[BYTES_PER_FIELD_ELEMENT * FIELD_ELEMENTS_PER_BLOB] for Blob here? ByteVector (just bytes) allows you not to go deep into SSZ in EL than Vector.

EIPS/eip-4844.md Outdated Show resolved Hide resolved
@g11tech g11tech force-pushed the bigendian-precompile-input branch from d39051e to 1d20295 Compare June 7, 2023 08:39
Copy link
Contributor

@asn-d6 asn-d6 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@eth-bot eth-bot enabled auto-merge (squash) June 7, 2023 10:01
Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

@eth-bot eth-bot merged commit 3ed2402 into ethereum:master Jun 7, 2023
10 checks passed
yperbasis added a commit to ledgerwatch/erigon-lib that referenced this pull request Jun 7, 2023
yperbasis added a commit to ledgerwatch/erigon that referenced this pull request Jun 7, 2023
calmbeing pushed a commit to calmbeing/bsc-erigon-lib that referenced this pull request Jul 10, 2023
calmbeing pushed a commit to calmbeing/bsc-erigon-lib that referenced this pull request Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-review Waiting on author to review c-update Modifies an existing proposal s-review This EIP is in Review t-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants