fix(stages): add commit threshold to merkle stage v2#1656
Conversation
joshieDo
requested review from
gakonst,
onbjerg,
rakita and
rkrasiuk
as code owners
joshieDo
added
C-bug
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 @@ Coverage Diff @@
## main #1656 +/- ##
==========================================
- Coverage 72.83% 72.75% -0.08%
==========================================
Files 392 392
Lines 46923 47108 +185
==========================================
+ Hits 34175 34273 +98
- Misses 12748 12835 +87
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
@joshieDo I Resolved merge conflicts, and have tested this locally and confirmed that it addresses the OOM issue, as discussed. Elegant solution overall - nice work.
My recommendation would be that we merge this, followed by:
- Further investigation of why we're getting mismatched state roots
- Apply the same logic in the account/storage hashing stages
- Improve how the checkpointing logic is communicated back to the user, right now we don't show any notion of progress for these stages
| // if there are more blocks than threshold it is faster to rebuild the trie | ||
| let mut loader = DBTrieLoader::new(tx.deref_mut()); | ||
| loader.calculate_root().map_err(|e| StageError::Fatal(Box::new(e)))? |
There was a problem hiding this comment.
Note this new method of instantiation of the DBTrieLoader since #1515
| TrieProgress::InProgress(_) => { | ||
| // Save the loader's progress & drop it | ||
| // to allow committing to the database (otherwise we're hitting the borrow | ||
| // checker) | ||
| let progress = loader.current; | ||
| drop(loader); | ||
| tx.commit()?; | ||
| // Reinstantiate the loader from where it was left off. | ||
| loader = DBTrieLoader::new(tx.deref_mut()); | ||
| loader.current = progress; | ||
| } |
There was a problem hiding this comment.
This is unfortunately necessary, we've already mutably borrowed the transaction to update_root and we cannot mutably borrow it again to commit(). But calling loader.tx.commit() is mut self because we use the vanilla DbTxMut method, so it will consume the tx w/o re-opening.
So instead, we drop the loader, freeing up the Transaction object, commit/reopen tx, and then re-instantiate the loader from where it was left off. Should be fine.
|
|
||
| let (account_proof, storage_root) = loader | ||
| .generate_acount_proof(self.db, root, hashed_address) | ||
| .generate_acount_proof(root, hashed_address) |
There was a problem hiding this comment.
self.db is no longer needed because we pass it inside the loader
3 participants
Replaces #1594.
DBTrieLoadernow has an inner checkpoint (no commit) which writes it to a genericSyncStageProgresstable, if500_000elements have been written to trie(s).Follow-up: Make sure
HashingAccountandHashingStoragealso do not commit and use a similar checkpoint logic.