chore: Add HashPointToBytes method
#428
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kevaundray
changed the title
Bytes method on Points with MapToScalarFieldBytesBytes method on the Point struct with MapToScalarFieldBytes
kevaundray
commented
Feb 17, 2024
tree.go
Outdated
Comment on lines
1621
to
1623
| // TODO: hash.Bytes() and MapToScalarFieldBytes(n.commitment) should give the same value | ||
| // TODO: its not clear why both values were needed before this change | ||
| // TODO: hash.Bytes should likely use BytesLE to be consistent with LE format |
There was a problem hiding this comment.
nit: remove TODO
kevaundray
commented
Feb 17, 2024
ipa.go
Outdated
| @@ -60,3 +60,9 @@ func FromBytes(fr *Fr, data []byte) { | |||
| copy(aligned[32-len(data):], data) | |||
| fr.SetBytes(aligned[:]) | |||
| } | |||
|
|
|||
| func MapToScalarFieldBytes(point *Point) [32]byte { | |||
There was a problem hiding this comment.
Whether it should go into go-ipa can be bikeshedded imo -- main goal is to update all libraries to have this breaking change
kevaundray
commented
Feb 17, 2024
kevaundray
commented
Feb 17, 2024
Signed-off-by: Ignacio Hagopian <[email protected]>
…s using MapToScalarField
kevaundray
force-pushed
the
kw/use-hash-commitment-for-root
branch
from
35d8e5f to
58815f7
Compare
kevaundray
changed the title
Bytes method on the Point struct with MapToScalarFieldBytesMapToScalarFieldBytes method
kevaundray
changed the title
MapToScalarFieldBytes methodMapToScalarFieldBytes method
|
The |
jsign
approved these changes
Feb 29, 2024
jsign
approved these changes
Feb 29, 2024
kevaundray
changed the title
MapToScalarFieldBytes methodHashPointToBytes method
This was referenced Mar 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rationale
We currently have two ways to convert a Point to 32 bytes, using the
.Bytesmethod and theMapToScalarFieldmethod. The latter requires you to serialize the scalar field, though you can think of it as 32 bytes.~~For all intents and purposes in the verkle API, it seems we only need one of these. The root node uses the former, but for other internal nodes we use the latter. This PR changes it so that the root node is not treated differently from the other internal nodes and this allows us to remove
.Bytesfrom the public API*. ~~It is still used when serializing proofs, but from the point of view of a verkle trie library, a proof is(should be) just an opaque set of bytes.
*This PR alone won't allow us to remove
.Bytes-- it is used in pedersen_hash/get_tree_key_hash. I'll make a PR to remove it from geth.Linking to crate-crypto/rust-verkle#86
EDIT:
Ignacio pointed out that Serialize/Bytes is needed because we need to deserialize the root node in the header in order to use it in a proof.