refactor trie root hash computation #6594
Conversation
…refactor-trie-set-hash # Conflicts: # integrationTests/state/stateTrieSync/stateTrieSync_test.go # trie/branchNode.go # trie/branchNode_test.go # trie/extensionNode_test.go # trie/patriciaMerkleTrie.go
BeniaminDrasovean
changed the base branch from
refactor-trie-mutex-usage
to
trie-concurrency-unit-tests
…o refactor-trie-set-hash
…o refactor-trie-set-hash # Conflicts: # trie/extensionNode_test.go # trie/patriciaMerkleTrie.go
…o refactor-trie-set-hash # Conflicts: # trie/baseNode.go # trie/interface.go
trie/patriciaMerkleTrie.go
Outdated
| func (tr *patriciaMerkleTrie) GetProof(key []byte, rootHash []byte) ([][]byte, []byte, error) { | ||
| trie, err := tr.Recreate(rootHash) |
There was a problem hiding this comment.
why recreating trie here? to avoid concurrent accesses?
There was a problem hiding this comment.
Yes. changed a bit and added a TODO to refactor this func in the future. We can avoid some redundant operations
| @@ -1438,7 +1438,7 @@ func (n *Node) getProof(rootHash []byte, key []byte) (*common.GetProofResponse, | |||
| return nil, err | |||
| } | |||
|
|
|||
| computedProof, value, err := tr.GetProof(key) | |||
| computedProof, value, err := tr.GetProof(key, rootHash) | |||
There was a problem hiding this comment.
do we need to recreate trie in GetProof? AccountsAdapterAPI().GetTrie(rootHash) will also recreate trie based on rootHash, right?
There was a problem hiding this comment.
Indeed. Changed inside GetProof and added a TODO there for a future improvement.
| } | ||
|
|
||
| it, err := NewDFSIterator(newTr) | ||
| it, err := NewDFSIterator(tr, rootHash) |
There was a problem hiding this comment.
do we still need critical section here?
There was a problem hiding this comment.
Yes. The idea is that this func is called on goroutines a lot (on API calls and this is also used when another node requests trie data because it is syncing). This gets a lot of data from the storage. Thus if we do not add a critical section here the processing will have delays because it will compete for storage with this function.
trie/patriciaMerkleTrie_test.go
Outdated
There was a problem hiding this comment.
maybe add more iterations in ConcurrencyOperations test; for me with more iterations it's failing with data race
There was a problem hiding this comment.
Indeed, it fails sometimes. This is because not all trie funcs have been refactored yet to allow parallel execution. Will do this in a future PR when all trie funcs have been refactored, and then it should not fail anymore.
Reasoning behind the pull request
Proposed changes
setHash,setHashConcurrentandsetRootHashfunctions from the trie. The resulting function will be able to set the hash for a specific trie node using goroutines. For example, a branch node will set the hashes of it's children on different goroutines, and after that it will compute it's own hash.Testing procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?