catchpoints: skip first account hash if ExpectingMoreEntries is true#4666
catchpoints: skip first account hash if ExpectingMoreEntries is true#4666cce wants to merge 1 commit into
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4666 +/- ##
=======================================
Coverage 54.38% 54.39%
=======================================
Files 403 403
Lines 51928 51930 +2
=======================================
+ Hits 28243 28245 +2
+ Misses 21308 21305 -3
- Partials 2377 2380 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| for _, hash := range balance.accountHashes { | ||
| for i, balance := range bals { | ||
| for j, hash := range balance.accountHashes { | ||
| if expectingMoreEntries[i] && j == 0 { |
There was a problem hiding this comment.
two questions:
- expectingMoreEntries set to true for the first chunk, and it is not in the beginning of a chunk, so why j == 0?
- Totals calculation - the original issue also reports off by one expected vs processed, I guess this is not addressed by this fix, please confirm.
There was a problem hiding this comment.
j == 0 is because the first account hash in the list is the account's own and the rest are for resources. It would be more clear to split accountHashes into separate "accountHash" and "resourceHashes" fields though. And also add expectingMoreEntries to the normalizedAccountBalances type perhaps..
There was a problem hiding this comment.
for total chunks, @jannotti was telling me he noticed the current code cannot know how many chunks there are but that he modified it in the boxes PR
There was a problem hiding this comment.
I guess a better fix might be to add this directly to prepareNormalizedBalancesV6 to prevent the account's hash from ending up in accountHashes altogether, instead of skipping it later like I'm doing here
There was a problem hiding this comment.
thank you for the j==0 explanation.
RE totals - I'm taking about number of accounts, not chunks:
Catchpoint total accounts: 15251246
Catchpoint accounts processed: 15251247
|
Closing in favor of #4668. |
2 participants
Summary
When an account is split over multiple catchpoint file chunks, we attempt to add its account hash to the catchpoint merkle trie twice, yielding a database error when processing the catchpoint. This attempts to fix that issue.
Test Plan
Needs testing