Catchpoints: Add the kv hashes into the trie#4812
Conversation
jannotti
changed the title
Codecov Report
@@ Coverage Diff @@
## master #4812 +/- ##
==========================================
- Coverage 54.69% 54.67% -0.03%
==========================================
Files 416 416
Lines 53596 53622 +26
==========================================
+ Hits 29316 29319 +3
- Misses 21851 21875 +24
+ Partials 2429 2428 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| // Now add the kvstore hashes | ||
| // CR Question: Does this neeed ordering? I don't see why it would, but | ||
| // the account iterator is explicitly ordered for some reason. | ||
| kvs, err := tx.QueryContext(ctx, "SELECT key, value FROM kvstore") |
There was a problem hiding this comment.
makeOrderedAccountsIter reads ordered, shouldn't this do the same? IDK
There was a problem hiding this comment.
See the discussion in slack. @cce tracked down why the accounts were ordered - it had to do with ordering the hashes so that it was faster to insert them into the trie. But, given that this code only runs one time, in an uncommon situation (a node was running without catchpoints, then turns them on) I definitely don't think it's worth the effort and complexity to create a temp table, then grab the hashes in order, just to build the tree faster.
There was a problem hiding this comment.
orderdAccountsIter was added in #1697, there is a nice writeup about it
3 participants
When building the catchpoint merkle trie from scratch, need to include the kv hashes too.