pbss: serialized points in uncompressed form #370
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
jsign
force-pushed
the
jsign/pbssuncompressed
branch
from
dc97f00 to
5bb850f
Compare
jsign
commented
Jun 26, 2023
| internalBitlistOffset = nodeTypeOffset + nodeTypeSize | ||
| internalNodeChildrenOffset = internalBitlistOffset + bitlistSize | ||
| internalBitlistOffset = nodeTypeOffset + nodeTypeSize | ||
| internalCommitmentOffset = internalBitlistOffset + bitlistSize |
There was a problem hiding this comment.
I think this was a pending rename @gballet referred in deleted line L858.
Comment on lines
-70
to
+71
| // - Internal nodes: <nodeType><bitlist><children...> | ||
| // - Leaf nodes: <nodeType><stem><bitlist><c1comm><c2comm><children...> | ||
| // - Internal nodes: <nodeType><bitlist><commitment> | ||
| // - Leaf nodes: <nodeType><stem><bitlist><comm><c1comm><c2comm><children...> |
There was a problem hiding this comment.
Fixed this comments; this is mostly for the base branch than this PR.
| comm := n.commitment.Bytes() | ||
| // XXX rename | ||
| copy(ret[internalNodeChildrenOffset:], comm[:]) | ||
| // Note that children were already appended in ret through the children slice. |
There was a problem hiding this comment.
Removed this comment since doesn't make sense now.
| case *LeafNode: | ||
| pointsToCompress = append(pointsToCompress, n.commitment, n.c1, n.c2) | ||
| compressedPointsIdxs[n] = len(pointsToCompress) - 3 |
There was a problem hiding this comment.
@gballet, I removed this line. We don't use the serializedPointsIdxs for LeafNodes.
This allowed me to reduce the preallocated size in L1493 a bit, and avoid a bit of work here.
| func (n *InternalNode) serializeInternalWithCompressedCommitment(compressedPointsIdxs map[VerkleNode]int, compressedPoints [][32]byte) ([]byte, error) { | ||
| serialized := make([]byte, nodeTypeSize+bitlistSize+SerializedPointCompressedSize) | ||
| bitlist := serialized[internalBitlistOffset:internalNodeChildrenOffset] | ||
| func (n *InternalNode) serializeInternalWithUncompressedCommitment(pointsIdx map[VerkleNode]int, serializedPoints [][SerializedPointUncompressedSize]byte) ([]byte, error) { |
There was a problem hiding this comment.
Also tried to rename variables to avoid referring to "compressed" or "uncompressed"; and use a more general "serialized". (If it's compressed or uncompressed, should be irrelevant)
jsign
force-pushed
the
jsign/pbssuncompressed
branch
from
5bb850f to
245360e
Compare
jsign
commented
Jun 26, 2023
Comment on lines
-1596
to
+1606
| baseSize := nodeTypeSize + StemSize + bitlistSize + 3*SerializedPointCompressedSize | ||
| result := make([]byte, baseSize, baseSize+4*32) // Extra pre-allocated capacity for 4 values. | ||
| result := make([]byte, nodeTypeSize+StemSize+bitlistSize+3*SerializedPointUncompressedSize+len(children)) | ||
| result[0] = leafRLPType | ||
| copy(result[leafSteamOffset:], n.stem[:StemSize]) | ||
| copy(result[leafBitlistOffset:], bitlist[:]) | ||
| copy(result[leafCommitmentOffset:], cBytes[:]) | ||
| copy(result[leafC1CommitmentOffset:], c1Bytes[:]) | ||
| copy(result[leafC2CommitmentOffset:], c2Bytes[:]) | ||
| result = append(result, children...) | ||
| copy(result[leafChildrenOffset:], children) |
There was a problem hiding this comment.
So here's an improvement that will avoid allocations. I'm not sure why we had that speculative 4*32 when we actually know the exact size. Maybe just due to organic evolution we missed doing this.
This will avoid append to do allocations.
jsign
commented
Jun 26, 2023
| require github.com/crate-crypto/go-ipa v0.0.0-20230601170251-1830d0757c80 | ||
| require github.com/crate-crypto/go-ipa v0.0.0-20230626131944-6a9b06cf26df |
There was a problem hiding this comment.
As mentioned in the PR description, this is targeting a tentative go-ipa version that doesn't have a PR yet.
It exists mostly to assist go-verkle in this PR. We can figure out when it can make sense to make it official after the tests.
…ed form Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
jsign
force-pushed
the
jsign/pbssuncompressed
branch
from
245360e to
e96a845
Compare
gballet
approved these changes
Jun 30, 2023
jsign
added a commit
that referenced
this pull request
Jul 10, 2023
* config: rename serialized point size constant and value to uncompressed form Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * switch to uncompressed serialization Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> --------- Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
gballet
added a commit
that referenced
this pull request
Jul 10, 2023
…serialization (#354) * don't rely on the commitment in HashedNode change leaf serialization to add the leaf commitment there fix a few tests fix: incorrect size computation caused commitment overwrite fix some unit tests fix the batch unit test * declare paths for serialization * Add CommitmentBytes back in SerializedNode * fixes from replaying * fix: deepsource warnings * tree: avoid memory allocs while collecting paths (#371) Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * pbss: serialized points in uncompressed form (#370) * config: rename serialized point size constant and value to uncompressed form Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * switch to uncompressed serialization Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> --------- Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * tree: transform first layer to hashed nodes after serializing (#372) * tree: after serializing transform first layer to hashed nodes Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * tree: add comment about temporary code Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> --------- Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * tree: make HashedNode non-pointer and fieldless (#373) * tree: make HashedNode non-pointer and fieldless Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * lints Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * hashednode: add panic comment Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * hashednode: add label in toDot Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> --------- Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * rebase fix Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> --------- Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> Co-authored-by: Ignacio Hagopian <jsign.uy@gmail.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.
This PR serializes commitment points in uncompressed form.
It leverages a go-ipa tentative branch that supports uncompressed form serialization.
While doing this, I also realized some extra optimizations I'll refer to in the comments.