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

Fix(multiproofs verification): don't skip visiting intermediate hashes #173

Merged
merged 4 commits into from
Jul 24, 2024

Conversation

thedevbirb
Copy link
Contributor

@thedevbirb thedevbirb commented Jul 23, 2024

This PR fixes the logic of the function VerifyMultiproof which doesn't visit the intermediate hashes calculated while traversing the tree, resulting in the error proof is missing required nodes when verifying Merkle Multiproofs.

In more detail, what I've noticed is that when adding the intermediate generalised indices in the keys array to be visited later, they are added to the end of the array but the latter is not sorted again, which mean those indices will actually never be read. This results in skipping some required steps of calculating intermediate hashes leading to a failure of the verification.

fastssz/proof.go

Lines 80 to 91 in c98805c

left, hasLeft := db[(k|1)^1]
right, hasRight := db[k|1]
if !hasRight || !hasLeft {
return false, fmt.Errorf("proof is missing required nodes, either %d or %d", (k|1)^1, k|1)
}
copy(tmp[:32], left[:])
copy(tmp[32:], right[:])
db[getParent(k)] = hashFn(tmp)
keys = append(keys, getParent(k))
pos++

The solution proposed avoids a 1-line change to add sorting after every insertion due to its inefficiency but relies instead on a auxiliary array to keep track of these additional indices

@ferranbt
Copy link
Owner

ferranbt commented Jul 23, 2024

Thank you for the PR!

Two things:

  • Are the module updates necessary. CI is also failing with dependency issues.
  • Can you also modify this multi proof test?

"math/rand"
"testing"
"time"

"github.com/attestantio/go-eth2-client/spec/bellatrix"
Copy link
Owner

Choose a reason for hiding this comment

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

Can you use the types in the spec-tests folder?

@thedevbirb
Copy link
Contributor Author

Thanks a lot for the review! I hope I have addressed it correctly

@ferranbt ferranbt merged commit 31cd371 into ferranbt:main Jul 24, 2024
1 check passed
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