feat: ensures consistent namespace hash format for all tree node parameters in VerifyInclusion function #176
Conversation
Codecov Report
@@ Coverage Diff @@
## master #176 +/- ##
==========================================
- Coverage 95.76% 95.41% -0.35%
==========================================
Files 5 5
Lines 519 523 +4
==========================================
+ Hits 497 499 +2
- Misses 13 14 +1
- Partials 9 10 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
…mat-before-proof-verification
…mat-before-proof-verification
…nts-consistency-check-to-verifyinclusion
|
This PR has reduced the code coverage because it includes early consistency checks at the start of the |
| // 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 | ||
| } | ||
| } |
There was a problem hiding this comment.
[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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
morge? |
Overview
Closes #157
Closes #161
Checklist