R4R: Bugfix for multistoreproof building #2474
Conversation
abelliumnt
requested review from
cwgoes,
ebuchman and
rigelrozanski
as code owners
abelliumnt
changed the title
store/rootmultistore.go
Outdated
| @@ -295,6 +295,14 @@ func (rs *rootMultiStore) Query(req abci.RequestQuery) abci.ResponseQuery { | |||
| return res | |||
| } | |||
|
|
|||
| if len(res.Proof) == 0 { | |||
| if len(res.Value) == 0 { | |||
There was a problem hiding this comment.
Hmm do we want to require proof of exclusion here?
Codecov Report
@@ Coverage Diff @@
## develop #2474 +/- ##
===========================================
+ Coverage 60.07% 61.87% +1.79%
===========================================
Files 150 149 -1
Lines 8702 9535 +833
===========================================
+ Hits 5228 5900 +672
- Misses 3117 3217 +100
- Partials 357 418 +61 |
|
@cwgoes Besides, if the bug is very significant. If we query a empty store from a full node, the full node will encounter a panic error. For instance, now the initial |
| @@ -56,6 +61,10 @@ func VerifyMultiStoreCommitInfo(storeName string, storeInfos []storeInfo, appHas | |||
|
|
|||
| // VerifyRangeProof verify iavl RangeProof | |||
| func VerifyRangeProof(key, value []byte, substoreCommitHash []byte, rangeProof *iavl.RangeProof) error { | |||
| // Both rangeProof and substoreCommitHash are nil | |||
There was a problem hiding this comment.
Hmm when do we expect this? What if just one is nil?
There was a problem hiding this comment.
When the query store is empty, both rangeProof and substoreCommitHash will be nil.
If only rangeProof is nil, an invalid proof error will be returned.
If only substoreCommitHash, the proof verification will report error about root hash mismatching.
| @@ -295,6 +295,11 @@ func (rs *rootMultiStore) Query(req abci.RequestQuery) abci.ResponseQuery { | |||
| return res | |||
| } | |||
|
|
|||
| if len(res.Proof) == 0 && len(res.Value) != 0 { | |||
There was a problem hiding this comment.
For nil values, should we require a proof that the value is nil?
There was a problem hiding this comment.
Normally, if value is nil, we should return absence proof. But if the store is empty, both proof and value will be nil. In this case, we should return a multiStoreProof to demonstrage the store is empty.
| var rangeProof iavl.RangeProof | ||
| cdc.MustUnmarshalBinary(iavlProof, &rangeProof) | ||
| var rangeProof *iavl.RangeProof | ||
| if iavlProof != nil { |
…ugfix-for-multistoreproof
jackzampolin
changed the title
|
@jackzampolin @alexanderbez @cwgoes |
|
I do not have any :-) |
|
I thought it might be wise for @jaekwon to review w.r.t the upstream lite client proof functionality. |
|
Hi @HaoyangLiu, general merkle logic is merged in Tendermint and the SDK. I've updated the develop branch with a PR that takes this concern into account: #2685 |
Suppose a IAVL store has no value, then the
prooffield andvaluefield of query response will be nil. As a result, panic error will arise in multistoreproof building. We did encountered the panic error in our own application.