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

[wip] SSZ-optimize proof (type 2) #443

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

gballet
Copy link
Member

@gballet gballet commented Jul 19, 2024

This is the first suggestion by Péter, it replaces the proposed suffix diff format with:

type StemStateDiff struct {
	Stem [StemSize]byte `json:"stem"`
	UpdatedSuffixes []byte   `json:"updatedsuffixes"`
	UpdatedCurrent  [][]byte `json:"updatedcurrent"`
	UpdatedNew      [][]byte `json:"updatednew"`
	ReadSuffixes []byte   `json:"readsuffixes"`
	ReadCurrent  [][]byte `json:"readcurrent"`

	InsertedSuffixes []byte   `json:"insertedsuffixes"`
	InsertedNew      [][]byte `json:"insertednew"`

	UntouchedSuffixes []byte `json:"untouchedsuffixes"`
}

The goal is to not use any Optional/null at all.

@gballet gballet requested a review from jsign July 19, 2024 07:56
Comment on lines -92 to 99
type SuffixStateDiffs []SuffixStateDiff
UpdatedSuffixes []byte `json:"updatedsuffixes"`
UpdatedCurrent [][]byte `json:"updatedcurrent"`
UpdatedNew [][]byte `json:"updatednew"`

type StemStateDiff struct {
Stem [StemSize]byte `json:"stem"`
SuffixDiffs SuffixStateDiffs `json:"suffixDiffs"`
ReadSuffixes []byte `json:"readsuffixes"`
ReadCurrent [][]byte `json:"readcurrent"`

InsertedSuffixes []byte `json:"insertedsuffixes"`
InsertedNew [][]byte `json:"insertednew"`

UntouchedSuffixes []byte `json:"untouchedsuffixes"`
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: the json tags must be camel case, as usual.

Comment on lines +119 to +120
if len(sd[i].UpdatedCurrent[j]) == 0 {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: remove empty if. Same applies in below cases.

if len(sd[i].UpdatedCurrent[j]) == 0 {

} else {
copy(ret[i].UpdatedCurrent[j], sd[i].UpdatedCurrent[j])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comments as mentioned in this other PR. Same applies in below cases in this method.

switch {
case isInsertion(preLen, postLen):
stemdiff.InsertedSuffixes = append(stemdiff.InsertedSuffixes, key[StemSize])
if postLen == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you meant != 32?

If we're in an isInsertion then postLen can't be 0.

}
case isRead(preLen, postLen):
stemdiff.ReadSuffixes = append(stemdiff.ReadSuffixes, key[StemSize])
if preLen == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto.

}
case isUpdate(preLen, postLen):
stemdiff.UpdatedSuffixes = append(stemdiff.UpdatedSuffixes, key[StemSize])
if preLen == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto.

copy(aligned[:preLen], proof.PreValues[i])
stemdiff.UpdatedCurrent = append(stemdiff.UpdatedCurrent, aligned[:])
}
if postLen == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto.

@@ -83,17 +82,20 @@ type Proof struct {
PostValues [][]byte
}

type SuffixStateDiff struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something we should remember, this kind of new format requires more integrity checks to see if same suffixes aren't claimed to be new, updated and/or untouched at the same time.

for _, suffixdiff := range stemstatediff.SuffixDiffs {
if /* len(suffixdiff.NewValue) > 0 - this only works for a slice */ suffixdiff.NewValue != nil {
for i, suffix := range stemstatediff.InsertedSuffixes {
if /* len(suffixdiff.NewValue) > 0 - this only works for a slice */ stemstatediff.InsertedNew[i] != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit lost here. Shouldn't stemstatediff.InsertedNew items always be not nil?
If was an inserted value, we need a value.

}
}
for i, suffix := range stemstatediff.UpdatedSuffixes {
if /* len(suffixdiff.NewValue) > 0 - this only works for a slice */ stemstatediff.UpdatedNew[i] != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants