Add "rust proof format" Deserialization #175
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
jwasinger
force-pushed
the
serialize-rust-proof-format-deserialize
branch
from
5056015 to
61bb52a
Compare
jwasinger
force-pushed
the
serialize-rust-proof-format-deserialize
branch
from
fc7a2bb to
3633d99
Compare
gballet
reviewed
Jan 14, 2022
Comment on lines
+150
to
+158
| poaStems = make([][]byte, numPoaStems) | ||
| for i := 0; i < int(numPoaStems); i++ { | ||
| var poaStem [31]byte | ||
| if err := binary.Read(reader, binary.LittleEndian, &poaStem); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| poaStems[i] = poaStem[:] | ||
| } |
There was a problem hiding this comment.
This allocate a buffer and copies the data into it, when you already have direct access to the source data.
Suggested change
| poaStems = make([][]byte, numPoaStems) | |
| for i := 0; i < int(numPoaStems); i++ { | |
| var poaStem [31]byte | |
| if err := binary.Read(reader, binary.LittleEndian, &poaStem); err != nil { | |
| return nil, err | |
| } | |
| poaStems[i] = poaStem[:] | |
| } | |
| // verify that there is enough data to read | |
| if numPoaStems * 31 + 4 <= len(proofSerialized) { | |
| return nil, errors.New("truncated proof") | |
| } | |
| poaStems = make([][]byte, numPoaStems) | |
| for i := 0; i < int(numPoaStems); i++ { | |
| poaStems[i] = poaStem[4+31*i:4+31*i+31] | |
| reader.Seek(31, io.SeekStart) | |
| } |
There was a problem hiding this comment.
Okay. If we go with this option, then the caller must make sure that the source data is not used after they call DeserializeProof(sourceData). It has potential to create some annoying bugs down the line.
But I'm fine to make it this way if you think the reduction in allocations is worth it here.
There was a problem hiding this comment.
It's only a problem if the buffer is reused to deserialize another proof afterwards, it's unlikely to happen but I guess you are right. I made a note of this in issue #176 and we will revisit it when the go-ethereum part of the code is more stable, and we know that this won't happen.
Co-authored-by: Guillaume Ballet <3272758+gballet@users.noreply.github.com>
gballet
added a commit
that referenced
this pull request
Jan 17, 2022
* rust-verkle-style proof serialization * coverage: remove an error check that can not be tested independently from another error check * Add "rust proof format" Deserialization (#175) * proof deserialization * appease the almighty linter * Update proof_test.go Co-authored-by: Guillaume Ballet <3272758+gballet@users.noreply.github.com> * fix linter issues Co-authored-by: Guillaume Ballet <3272758+gballet@users.noreply.github.com> * improve deserialization error coverage Co-authored-by: jwasinger <j-wasinger@hotmail.com>
gballet
added a commit
that referenced
this pull request
Jan 18, 2022
* change proof to the format of rust-verkle * merge the depth and presence status into a single byte (#168) * when encountering another stem in GetCommitmentsAlongPath, return it (#169) * when encountering another stem in GetCommitmentsAlongPath, return it * add a unit test for coverage * goimport * rust-verkle-style proof serialization (#173) * rust-verkle-style proof serialization * coverage: remove an error check that can not be tested independently from another error check * Add "rust proof format" Deserialization (#175) * proof deserialization * appease the almighty linter * Update proof_test.go Co-authored-by: Guillaume Ballet <3272758+gballet@users.noreply.github.com> * fix linter issues Co-authored-by: Guillaume Ballet <3272758+gballet@users.noreply.github.com> * improve deserialization error coverage Co-authored-by: jwasinger <j-wasinger@hotmail.com> Co-authored-by: jwasinger <j-wasinger@hotmail.com>
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.
No description provided.