Use maps instead of slices in state diff struct #1466
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joshklop
force-pushed
the
joshklop/statediff
branch
from
3b275c1
to
2fe819a
Compare
|
Because we (1) loop over maps that have a I attempted to remove all cases of harmful memory aliasing by copying the value, e.g., // addr is a felt.Felt, classHash is a *felt.Felt
for addr, classHash := range something {
addrCopy := addr
// do things
} |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1466 +/- ##
==========================================
- Coverage 72.90% 72.87% -0.04%
==========================================
Files 96 96
Lines 9948 9982 +34
==========================================
+ Hits 7253 7274 +21
- Misses 2157 2169 +12
- Partials 538 539 +1 ☔ View full report in Codecov by Sentry. |
joshklop
force-pushed
the
joshklop/statediff
branch
2 times, most recently
from
b1d69e4
to
4971ec3
Compare
I don't think there is a reason to do that. and I am sorry to let you know that this will need a migration :D |
Ahhhh yes that is unfortunate. I wonder if we can play tricks with the encoder to put that off to regenesis? |
You will still have to write an adapter from old type to new type to be able to do that. That is most of the code you will need to write for the migration anyways. |
Yeah, I was thinking the migration might take too long. ~1M blocks on testnets is a lot... but now that I think about it, the migration shouldn't be too bad. |
joshklop
force-pushed
the
joshklop/statediff
branch
2 times, most recently
from
71c2463
to
2944a39
Compare
|
Got this error while trying to migrate the goerli snapshot. I'm planning to look into it tomorrow, marking this PR as a draft in the meantime. |
joshklop
force-pushed
the
joshklop/statediff
branch
from
2944a39
to
87ca2e1
Compare
|
Lowered the batch size from 1M to 100K (500K was also too large). It's not panicing, but the goerli migration hasn't finished and it started over an hour ago :/ |
Might be out of scope for this PR but if you add concurrency to |
joshklop
force-pushed
the
joshklop/statediff
branch
from
87ca2e1
to
359ba0c
Compare
|
I tried to parallelize the bucket migrator without much success because we need to synchronize db writes #1497. In this PR, I modified the migration process to allow a single migration to be parallelized across multiple db transactions. This assumes the transactions modify disjoint sets of keys, which is safe in our case. Migrating the integration snapshot took around 2 minutes, while the goerli snapshot took 1-2 hours. |
This is way too risky of an assumption to make. Might not hold in the near future, that will make us revert all the work we have done here. What is worse is, we might not recognize that it doesnt hold anymore and corrupt users' databases. |
We're only making the assumption about a single migration, which will only run on databases with a known schema version. Can you elaborate on why the assumption is risky? |
joshklop
force-pushed
the
joshklop/statediff
branch
3 times, most recently
from
03fd6fe
to
fa2f245
Compare
omerfirmak
force-pushed
the
joshklop/statediff
branch
from
694706e
to
6212c90
Compare
joshklop
force-pushed
the
joshklop/statediff
branch
from
e04d232
to
a663601
Compare
mainnet migration fails with 100k
omerfirmak
approved these changes
Dec 8, 2023
omerfirmak
approved these changes
Dec 8, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Makes it much easier to build a state diff incrementally during execution.