Merkle Proof API refactor #3453
Conversation
# Conflicts: # api/groups/proofGroup.go # api/groups/proofGroup_test.go # facade/nodeFacade.go
api/groups/proofGroup.go
Outdated
| } | ||
|
|
||
| c.JSON( | ||
| http.StatusOK, | ||
| shared.GenericAPIResponse{ | ||
| Data: gin.H{"proof": hexProof}, | ||
| Data: gin.H{ | ||
| "mainProof": mainProofHex, |
There was a problem hiding this comment.
Should have been:
{
"data": {
"proofs": {
"mainProof": ..
....
}
}
....
you placed the attributes right under the data field which doesn't follow the format of other endpoints
facade/nodeFacade.go
Outdated
| } | ||
|
|
||
| // GetProofDataTrie returns the Merkle Proof for the given address, and another Merkle Proof | ||
| // for the given key, if it is present in the dataTrie |
There was a problem hiding this comment.
"if it is present in the dataTrie" -> "if it exists in the dataTrie"
facade/nodeFacade.go
Outdated
| func (nf *nodeFacade) GetProof(rootHash string, address string) ([][]byte, error) { | ||
| rootHashBytes, err := hex.DecodeString(rootHash) | ||
| func (nf *nodeFacade) GetProof(rootHash string, address string) (*shared.GetProofResponse, error) { | ||
| proof, value, err := nf.getProof(rootHash, address) |
There was a problem hiding this comment.
previously, nodeFacade was only a middleware between the API and node.go. you introduced some logic here and also a marshalizer, that already exists in node.go.
Maybe move the logic there?
facade/nodeFacade.go
Outdated
| addressBytes, err := nf.DecodeAddressPubkey(address) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| proof, err := trie.GetProof(addressBytes) | ||
| account, err := nf.accountsState.LoadAccount(addressBytes) |
There was a problem hiding this comment.
the effect of "changing the trie data" while this function executes is more obvious in this case.
facade/nodeFacade.go
Outdated
|
|
||
| // GetProofDataTrie returns the Merkle Proof for the given address, and another Merkle Proof | ||
| // for the given key, if it is present in the dataTrie | ||
| func (nf *nodeFacade) GetProofDataTrie(rootHash string, address string, key string) (*shared.GetProofResponse, *shared.GetProofResponse, error) { |
There was a problem hiding this comment.
Let's add an integration test with one node that demonstrates that proof operations work with the verify proof operations.
# Conflicts: # facade/initial/initialNodeFacade.go
Codecov Report
@@ Coverage Diff @@
## development #3453 +/- ##
===============================================
- Coverage 73.89% 73.87% -0.02%
===============================================
Files 581 582 +1
Lines 74278 74441 +163
===============================================
+ Hits 54889 54996 +107
- Misses 15002 15044 +42
- Partials 4387 4401 +14
Continue to review full report at Codecov.
|
node/node.go
Outdated
| return false, err | ||
| } | ||
|
|
||
| return trie.VerifyProof(key, proof) |
There was a problem hiding this comment.
It now has become obvious that you actually need all the trie nodes to verify a proof. This should have been a stateless function.
There was a problem hiding this comment.
Removed. This will be added on erdgo.
| return nil, nil, fmt.Errorf("the address does not belong to a user account") | ||
| } | ||
| dataTrieRootHash := userAccount.GetRootHash() | ||
|
|
There was a problem hiding this comment.
remove empty line here and add it on L1345
| @@ -602,15 +602,11 @@ func (tr *patriciaMerkleTrie) GetProof(key []byte) ([][]byte, []byte, error) { | |||
| } | |||
|
|
|||
| // VerifyProof verifies the given Merkle proof | |||
| func (tr *patriciaMerkleTrie) VerifyProof(key []byte, proof [][]byte) (bool, error) { | |||
| func (tr *patriciaMerkleTrie) VerifyProof(rootHash []byte, key []byte, proof [][]byte) (bool, error) { | |||
There was a problem hiding this comment.
Why do we need to provide a roothash here? I thought this process is stateless and it can work with an empty roothash.
BeniaminDrasovean
dismissed stale reviews from bogdan-rosianu and iulianpascalau
via
dbf66f9
BeniaminDrasovean
dismissed stale reviews from bogdan-rosianu and iulianpascalau
via
0ecd6ab
# Conflicts: # facade/interface.go
…Network/elrond-go into merkle-proof-api-refactor
Added a new endpoint that returns the proof for an account, and the proof for a given key from the data trie, all in a single call.
For tessting, check that the following endpoints work on a node:
"/proof/address/:address"
"/proof/root-hash/:roothash/address/:address"
"/proof/root-hash/:roothash/address/:address/key/:key"
"/proof/verify"