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.

Sign up for GitHub

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

Jump to bottom

TreeFromProof test hardening #356

Merged
merged 5 commits into from
Jan 18, 2024
Merged

TreeFromProof test hardening #356

merged 5 commits into from
Jan 18, 2024

Conversation

jsign
Copy link
Collaborator

@jsign jsign commented May 18, 2023 •
edited
Loading

This PR adds more test coverage for PreStateTreeFromProof(...).

The diff is very messy, so I recommend some "split view" maybe.

jsign
jsign commented May 18, 2023
View reviewed changes
proof_ipa.go Outdated Show resolved Hide resolved
proof_ipa.go Outdated Show resolved Hide resolved
tree.go Outdated Show resolved Hide resolved
jsign
jsign commented May 19, 2023
View reviewed changes
tree_test.go Outdated Show resolved Hide resolved
gballet
gballet reviewed Jun 5, 2023
View reviewed changes
Copy link
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

Thanks for looking at that. Indeed a lot of stuff has fossilized and should be cleaned up. I will change the interface to MakeVerkleMultiProof

proof_ipa.go Outdated Show resolved Hide resolved
tree.go Outdated Show resolved Hide resolved
proof_test.go Outdated Show resolved Hide resolved
proof_test.go Outdated Show resolved Hide resolved
proof_test.go Outdated
droot, err := TreeFromProof(dproof, root.Commit())
if err != nil {
t.Fatal(err)
func TestStatelessDeserializeMissingChildNode(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

that's nice, but there should be another one in which oneKeyTest is the missing key, so that we can check this use case is also working.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, it's named TestStatelessDeserializeAbsentValueInExistingLeafNode(...).

@gballet
Copy link
Member

gballet commented Jun 5, 2023

@jsign right, the reason keyvals is passed, is because I used to make the difference between "previous" values (held by keyvals) and "current values" (held by the tree). This isn't something that we are doing at the moment, but it will come back since we want to put the post state in the tree. We can figure out the proper way to do this when this happens.

@gballet gballet mentioned this pull request Jun 5, 2023
Merged
@jsign
proof: add extra test and fix lints
0a3b83a 
Signed-off-by: Ignacio Hagopian <[email protected]>
@jsign
proof: test simplification
fbb0559 
Signed-off-by: Ignacio Hagopian <[email protected]>
jsign added 2 commits January 17, 2024 15:36
@jsign
fixes
a5e5c11 
Signed-off-by: Ignacio Hagopian <[email protected]>
@jsign
readd tests
a5bb701 
Signed-off-by: Ignacio Hagopian <[email protected]>
jsign
jsign commented Jan 17, 2024
View reviewed changes
proof_test.go
}
}

func TestProofOfPresenceWithEmptyValue(t *testing.T) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is now TestStatelessDeserializeAbsentValueInExistingLeafNode(...)

proof_test.go
@@ -729,125 +729,211 @@ func TestVerkleProofMarshalUnmarshalJSON(t *testing.T) {
func TestStatelessDeserialize(t *testing.T) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Most of the tests now use shared code which allows them to be described more shortly.

proof_test.go
}

func TestStatelessDeserializeMissingChildNode(t *testing.T) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed to TestStatelessDeserializeMissingLeafNode

proof_test.go
}

func TestStatelessDeserializeDepth2(t *testing.T) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not lost, the test is still here but the diff is confusing.

proof_test.go
@@ -988,87 +1074,6 @@ func TestProofVerificationWithPostState(t *testing.T) { // skipcq: GO-R1005
})
}
}
func TestProofOfAbsenceBorderCase(t *testing.T) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not deleted, it's now in L813.

proof_test.go
}
}

func TestProofOfAbsenceBorderCaseReversed(t *testing.T) {
Copy link
Collaborator Author

@jsign jsign Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not deleted, it's now in L826.

@jsign jsign requested a review from gballet January 17, 2024 21:45
@jsign jsign marked this pull request as ready for review January 17, 2024 21:45
gballet
gballet approved these changes Jan 18, 2024
View reviewed changes
Copy link
Member

@gballet gballet 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 for rebasing.

@gballet gballet merged commit 371161b into master Jan 18, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gballet gballet gballet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jsign @gballet