Trie mutex refactoring #4984
Trie mutex refactoring #4984
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## rc/v1.5.0 #4984 +/- ##
============================================
Coverage ? 70.64%
============================================
Files ? 666
Lines ? 86482
Branches ? 0
============================================
Hits ? 61098
Misses ? 20800
Partials ? 4584 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
trie/patriciaMerkleTrie_test.go
Outdated
| _, _, _ = tr.GetSerializedNodes([]byte("dog"), 1024) | ||
| wgEnd.Done() | ||
| fmt.Println("finished") |
There was a problem hiding this comment.
so we don't want to check the error here?
There was a problem hiding this comment.
forgotten prints :(
No, not quite, the storer is a stub anyway. GetSerializedNodes will error as it won't find some hashes, GetSerializedNode would have returned what the mock storer returned
iulianpascalau
dismissed stale reviews from BeniaminDrasovean and ssd04
via
adeea2e
| @@ -301,6 +295,9 @@ func (tr *patriciaMerkleTrie) recreate(root []byte, tsm common.StorageManager) ( | |||
|
|
|||
| // String outputs a graphical view of the trie. Mainly used in tests/debugging | |||
| func (tr *patriciaMerkleTrie) String() string { | |||
| tr.mutOperation.Lock() | |||
There was a problem hiding this comment.
why do we need here Lock?
you can use RLock
same for Get(), GetObsoleteHashes(), VerifyProof(), GetNumNodes(), GetOldRoot()
There was a problem hiding this comment.
Unfortunately, we need the Lock/Unlock pair here and on all methods that are doing only get operations.
Reason: if the trie is collapsed/partially collapsed (as what happens after each Commit call) the nodes will hold only the collapsed version of the nodes on the last stored level. Then, at each traversing, the trie might try to fetch a collapsed node that will trigger the DB loading and altering of its containing pointers. So, because of the usage of the resolveCollapsed function, we can not use RLock/RUnlock functions. The concurrency test might not fail because is not big enough, but otherwise, it will fail on concurrent operations. Changed the test to have only one level in memory so it will fail if we change the Lock/Unlock to RLock/RUnlock.
iulianpascalau
dismissed stale reviews from ssd04 and BeniaminDrasovean
via
64d8b26
There was a problem hiding this comment.
@@ Log scanner @@
trie-mutex-refactoring
================================================================================
- Known Warnings 9
- New Warnings 0
- Known Errors 0
- New Errors 0
- Panics 0
================================================================================ - block hash does not match 10283
- miniblocks does not match 0
- miniblock hash does not match 0
- receipts hash missmatch 2
- wrong nonce in block 3761
================================================================================ - No jailed nodes on the thestnet
================================================================================
iulianpascalau
added
type:bug
Reasoning behind the pull request
Proposed changes
Testing procedure
WARNlogs messages whenever the node is closing.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?