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

feat: ensures consistent namespace hash format for all tree node parameters in VerifyInclusion function #176

Merged
merged 26 commits into from
Apr 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
6b0ecaf
checks the exact size of the node
staheri14 Apr 10, 2023
a17fc23
revises tests descriptions
staheri14 Apr 11, 2023
14746d1
revises func description
staheri14 Apr 11, 2023
9cde761
refactors tests
staheri14 Apr 11, 2023
e0766e3
reorganizes helper functions and adds their descriptions
staheri14 Apr 11, 2023
422368f
adds more comments
staheri14 Apr 11, 2023
05017fc
resolves a gofumpt issue
staheri14 Apr 11, 2023
9b27e87
verifies root and proof.nodes validity at the beginning of verifyincl…
staheri14 Apr 11, 2023
ca7baf8
Merge branch 'master' into check-node-exact-length-in-hasher
staheri14 Apr 12, 2023
30abad6
Merge branch 'master' into verify-node-format-before-proof-verification
staheri14 Apr 12, 2023
7506e19
Merge branch 'check-node-exact-length-in-hasher' into verify-node-for…
staheri14 Apr 12, 2023
bbf7613
developes test for node format verification in the Verifyinclusion
staheri14 Apr 12, 2023
847c6ad
verifies that the node size is exactly the same as the expected size
staheri14 Apr 12, 2023
b021016
Merge branch 'check-node-exact-length-in-hasher' into verify-node-for…
staheri14 Apr 12, 2023
3e871b8
Merge branch 'master' into performs-arguments-consistency-check-to-ve…
staheri14 Apr 12, 2023
6bf186f
Merge branch 'master' into check-node-exact-length-in-hasher
staheri14 Apr 12, 2023
0844d82
explains that VerifyInclusion does not verify proof completeness
staheri14 Apr 13, 2023
f5427e5
adds a test case for node length > the expected size
staheri14 Apr 13, 2023
76b27b4
revises the error message
staheri14 Apr 13, 2023
dd6a640
revises test names
staheri14 Apr 13, 2023
428744d
Merge branch 'check-node-exact-length-in-hasher' into performs-argume…
staheri14 Apr 13, 2023
782b3f3
fixes gofumpt issues
staheri14 Apr 13, 2023
b5f617e
revises comments
staheri14 Apr 13, 2023
8628f5c
refactors tests
staheri14 Apr 13, 2023
91b96f7
Merge branch 'master' into verify-node-format-before-proof-verification
staheri14 Apr 13, 2023
bcd45c3
resolves test failure
staheri14 Apr 13, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions proof.go
Original file line number Diff line number Diff line change
Expand Up @@ -302,9 +302,22 @@ func (proof Proof) verifyLeafHashes(nth *Hasher, verifyCompleteness bool, nID na
// and the provided proof to regenerate and compare the root. Note that the leavesWithoutNamespace data should not contain the prefixed namespace, unlike the tree.Push method,
// which takes prefixed data. All leaves implicitly have the same namespace ID:
// `nid`.
// VerifyInclusion does not verify the completeness of the proof, so it's possible for leavesWithoutNamespace to be a subset of the leaves in the tree that have the namespace ID nid.
func (proof Proof) VerifyInclusion(h hash.Hash, nid namespace.ID, leavesWithoutNamespace [][]byte, root []byte) bool {
nth := NewNmtHasher(h, nid.Size(), proof.isMaxNamespaceIDIgnored)

// perform some consistency checks:
// check that the root is valid w.r.t the NMT hasher
if err := nth.ValidateNodeFormat(root); err != nil {
return false
}
// check that all the proof.nodes are valid w.r.t the NMT hasher
for _, node := range proof.nodes {
if err := nth.ValidateNodeFormat(node); err != nil {
return false
}
}
Comment on lines +309 to +319
Copy link
Member

Choose a reason for hiding this comment

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

[non-blocking question]
similar to what we have discussed with needing to throw and error after the proof was verified if the namepsaces are not in order, do we need to do the same here?

Copy link
Collaborator Author

@staheri14 staheri14 Apr 14, 2023

Choose a reason for hiding this comment

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

I believe the issue you are referring to is #97. Currently, if namespaces are not in order, the verification result is false without providing any error. However, we can choose to return a proper error message to indicate the root cause of verification failure, if this information would be useful for the caller. However, my current understanding is that returning an error from proof verification methods such as VerifyInclusion may not address the issue in question, as the problem is related to verifying that shares of a block are not out of order. To achieve this, the full node must construct NMTs from the block data and identify out-of-order leaves during tree construction.

Does this address your question?

Copy link
Member

Choose a reason for hiding this comment

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

To achieve this, the full node must construct NMTs from the block data and identify out-of-order leaves during tree construction.

ahh I See. consensus full nodes having the ability to generate otherwise valid proofs makes sense, but that's not possible, so there's no point.

do we need to enable non-consensus full nodes to have this ability if we want to protect against this in a trust minimized way then?

Copy link
Collaborator Author

@staheri14 staheri14 Apr 17, 2023

Choose a reason for hiding this comment

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

do we need to enable non-consensus full nodes to have this ability if we want to protect against this in a trust minimized way then?

I guess you are suggesting returning an error message from the VerirfyInclusion method to indicate whether leaves were out of order so that the light node can use that information to craft a bad encoding proof and propagate it in the network (or just consume it locally).
Returning an error message is possible, but it may not provide a definitive conclusion, as in the current implementation of VerirfyInclusion (and other similar verification methods available), as soon as a violation is detected e.g., nodes are out of order, the verification stops and the false result is returned. This means that the root does not get calculated from the given proof and leaves to see whether it matches the expected root (from the block header) or not. As such, even if an error gets returned, it would not be conclusive. If were to enable such feature, we need to modify the verification process to always calculate the root. Please let me know if this is a desired feature and I can track it in a separate GH issue.

Copy link
Member

@evan-forbes evan-forbes Apr 17, 2023

Choose a reason for hiding this comment

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

Please let me know if this is a desired feature and I can track it in a separate GH issue.

I think we need more discussion around this and to walk through the entire flow. this convo is definitely not blocking merging.

I'm not entirely sure that we have to worry about it, since we this is the equivalent of 2/3s signing over an invalid data root. Typically, full nodes would simply reject that block, which is what would happen now, in our case though, this could result in light clients being tricked in very specific scenarios. It seems like something that we would want to fix to have trust minimized exclusion proofs. meaning that when a consensus or full node returns all the data in a namespace along with a prove of inclusion and completeness, then light clients can rely on the completeness without making an honest majority assumption or running a consensus node.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we need more discussion around this and to walk through the entire flow.

I will make a note to discuss this further in our next call.


// add namespace to all the leaves
hashes := make([][]byte, len(leavesWithoutNamespace))
for i, d := range leavesWithoutNamespace {
Expand Down
50 changes: 30 additions & 20 deletions proof_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package nmt
import (
"bytes"
"crypto/sha256"
"hash"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -256,42 +257,51 @@ func TestVerifyLeafHashes_Err(t *testing.T) {
}

func TestVerifyInclusion_False(t *testing.T) {
// create a sample tree
nmt := exampleNMT(2, 1, 2, 3, 4, 5, 6, 7, 8)
hasher := nmt.treeHasher
root, err := nmt.Root()
hasher := sha256.New()

// create a sample tree with namespace ID size of 1
nmt1 := exampleNMT(1, 1, 2, 3, 4, 5, 6, 7, 8)
root1, err := nmt1.Root()
require.NoError(t, err)
nid4_1 := namespace.ID{4}
proof4_1, err := nmt1.ProveRange(3, 4) // leaf at index 3 has namespace ID 4
require.NoError(t, err)
leaf4_1 := nmt1.leaves[3][nmt1.NamespaceSize():]

// create nmt proof for namespace ID 4
nID4 := namespace.ID{4, 4}
proof4, err := nmt.ProveNamespace(nID4)
// create a sample tree with namespace ID size of 2
nmt2 := exampleNMT(2, 1, 2, 3, 4, 5, 6, 7, 8)
root2, err := nmt2.Root()
require.NoError(t, err)
// proof4 is the inclusion proof for the leaf at index 3
leaf4WithoutNamespace := nmt.leaves[3][nmt.NamespaceSize():] // the VerifyInclusion function expects the leaf without the namespace ID, that's why we cut the namespace ID from the leaf.
nid4_2 := namespace.ID{4, 4}
proof4_2, err := nmt2.ProveRange(3, 4) // leaf at index 3 has namespace ID 4
require.NoError(t, err)
leaf4_2 := nmt2.leaves[3][nmt2.NamespaceSize():]

// corrupt the last node in the proof4.nodes, it resides on the right side of the proof4.end index.
// this test scenario makes the VerifyInclusion fail when constructing the tree root from the
// computed subtree root and the proof.nodes on the right side of the proof.end index.
proof4.nodes[2] = proof4.nodes[2][:nmt.NamespaceSize()-1]
require.Equal(t, leaf4_2, leaf4_1)
leaf := leaf4_1

// create nmt
type args struct {
Hasher *Hasher
nID namespace.ID
leafHashes [][]byte
root []byte
hasher hash.Hash
nID namespace.ID
leavesWithoutNamespace [][]byte
root []byte
}
tests := []struct {
name string
proof Proof
args args
result bool
}{
{" wrong proof.nodes: the last node has an incorrect format", proof4, args{hasher, nID4, [][]byte{leaf4WithoutNamespace}, root}, false},
{"nID size of proof < nID size of VerifyInclusion's nmt hasher", proof4_1, args{hasher, nid4_2, [][]byte{leaf}, root2}, false},
{"nID size of proof > nID size of VerifyInclusion's nmt hasher", proof4_2, args{hasher, nid4_1, [][]byte{leaf}, root1}, false},
{"nID size of root < nID size of VerifyInclusion's nmt hasher", proof4_2, args{hasher, nid4_2, [][]byte{leaf}, root1}, false},
{"nID size of root > nID size of VerifyInclusion's nmt hasher", proof4_1, args{hasher, nid4_1, [][]byte{leaf}, root2}, false},
{"nID size of proof and root < nID size of VerifyInclusion's nmt hasher", proof4_1, args{hasher, nid4_2, [][]byte{leaf}, root1}, false},
{"nID size of proof and root > nID size of VerifyInclusion's nmt hasher", proof4_2, args{hasher, nid4_1, [][]byte{leaf}, root2}, false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := tt.proof.VerifyInclusion(tt.args.Hasher, tt.args.nID, tt.args.leafHashes, tt.args.root)
got := tt.proof.VerifyInclusion(tt.args.hasher, tt.args.nID, tt.args.leavesWithoutNamespace, tt.args.root)
assert.Equal(t, tt.result, got)
})
}
Expand Down