Use maps instead of slices in state diff struct #1466

joshklop · 2023-11-26T03:01:19Z

Makes it much easier to build a state diff incrementally during execution.

joshklop · 2023-11-26T03:04:45Z

Because we (1) loop over maps that have a felt.Felt as a key and (2) take the address of those felt loop variables, golangci-lint complains about memory aliasing.

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 · 2023-11-26T03:08:07Z

Codecov Report

Attention: 51 lines in your changes are missing coverage. Please review.

Comparison is base (acc9ac0) 72.90% compared to head (1f341bf) 72.87%.

Files	Patch %	Lines
core/state.go	61.53%	9 Missing and 6 partials ⚠️
adapters/core2p2p/state.go	0.00%	10 Missing ⚠️
migration/migration.go	81.25%	6 Missing and 3 partials ⚠️
core/state_update.go	84.09%	7 Missing ⚠️
p2p/starknet/block_bodies.go	0.00%	7 Missing ⚠️
sync/sync.go	50.00%	1 Missing and 1 partial ⚠️
core/contract.go	75.00%	0 Missing and 1 partial ⚠️

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.
📢 Have feedback on the report? Share it here.

omerfirmak · 2023-11-27T07:40:23Z

Because we (1) loop over maps that have

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

joshklop · 2023-11-27T13:02:50Z

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?

omerfirmak · 2023-11-27T15:26:46Z

play tricks with the encoder

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.

joshklop · 2023-11-27T15:33:08Z

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 · 2023-11-28T04:10:46Z

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.

% ./build/juno --http --network goerli --db-path ~/downloads/juno_goerli 
2023/11/27 22:07:46 maxprocs: Leaving GOMAXPROCS=12: CPU quota undefined

       _                    
      | |                   
      | |_   _ _ __   ___   
  _   | | | | | '_ \ / _ \  
 | |__| | |_| | | | | (_) |  
  \____/ \__,_|_| |_|\___/ v0.7.7-rc1-2-g2944a392

Juno is a Go implementation of a Starknet full-node client created by Nethermind.

22:07:47.388 27/11/2023 -06:00	WARN	node/node.go:184	Ethereum node address not found; will not verify against L1
22:07:47.426 27/11/2023 -06:00	INFO	migration/migration.go:98	Applying database migration	{"stage": "15/15"}
22:11:33.971 27/11/2023 -06:00	ERROR	node/node.go:245	Error while closing the DB	{"err": "leaked iterators: 5", "errVerbose": "leaked iterators: 5\n(1) attached stack trace\n  -- stack trace:\n  | github.com/cockroachdb/pebble.(*tableCacheContainer).close\n  | \t/home/user/.go/pkg/mod/github.com/cockroachdb/[email protected]/table_cache.go:121\n  | github.com/cockroachdb/pebble.(*DB).Close\n  | \t/home/user/.go/pkg/mod/github.com/cockroachdb/[email protected]/db.go:1457\n  | github.com/NethermindEth/juno/db/pebble.(*DB).Close\n  | \t/home/user/repos/juno/db/pebble/db.go:96\n  | github.com/NethermindEth/juno/node.(*Node).Run.func1\n  | \t/home/user/repos/juno/node/node.go:244\n  | runtime.gopanic\n  | \t/usr/lib/go/src/runtime/panic.go:914\n  | github.com/NethermindEth/juno/db.discardTxnOnPanic\n  | \t/home/user/repos/juno/db/db.go:116\n  | runtime.gopanic\n  | \t/usr/lib/go/src/runtime/panic.go:914\n  | github.com/cockroachdb/pebble.(*Batch).grow\n  | \t/home/user/.go/pkg/mod/github.com/cockroachdb/[email protected]/batch.go:1237\n  | github.com/cockroachdb/pebble.(*Batch).prepareDeferredKeyValueRecord\n  | \t/home/user/.go/pkg/mod/github.com/cockroachdb/[email protected]/batch.go:527\n  | github.com/cockroachdb/pebble.(*Batch).SetDeferred\n  | \t/home/user/.go/pkg/mod/github.com/cockroachdb/[email protected]/batch.go:623\n  | github.com/cockroachdb/pebble.(*Batch).Set\n  | \t/home/user/.go/pkg/mod/github.com/cockroachdb/[email protected]/batch.go:605\n  | github.com/NethermindEth/juno/db/pebble.(*Transaction).Set\n  | \t/home/user/repos/juno/db/pebble/transaction.go:68\n  | github.com/NethermindEth/juno/migration.changeStateDiffStruct\n  | \t/home/user/repos/juno/migration/migration.go:627\n  | github.com/NethermindEth/juno/migration.(*BucketMigrator).Migrate\n  | \t/home/user/repos/juno/migration/bucket_migrator.go:102\n  | github.com/NethermindEth/juno/migration.migrateIfNeeded.func1\n  | \t/home/user/repos/juno/migration/migration.go:106\n  | github.com/NethermindEth/juno/db.Update\n  | \t/home/user/repos/juno/db/db.go:104\n  | github.com/NethermindEth/juno/db/pebble.(*DB).Update\n  | \t/home/user/repos/juno/db/pebble/db.go:106\n  | github.com/NethermindEth/juno/migration.migrateIfNeeded\n  | \t/home/user/repos/juno/migration/migration.go:105\n  | github.com/NethermindEth/juno/migration.MigrateIfNeeded\n  | \t/home/user/repos/juno/migration/migration.go:69\n  | github.com/NethermindEth/juno/node.(*Node).Run\n  | \t/home/user/repos/juno/node/node.go:262\n  | main.main.func2\n  | \t/home/user/repos/juno/cmd/juno/juno.go:142\n  | github.com/spf13/cobra.(*Command).execute\n  | \t/home/user/.go/pkg/mod/github.com/spf13/[email protected]/command.go:872\n  | github.com/spf13/cobra.(*Command).ExecuteC\n  | \t/home/user/.go/pkg/mod/github.com/spf13/[email protected]/command.go:990\n  | github.com/spf13/cobra.(*Command).Execute\n  | \t/home/user/.go/pkg/mod/github.com/spf13/[email protected]/command.go:918\n  | github.com/spf13/cobra.(*Command).ExecuteContext\n  | \t/home/user/.go/pkg/mod/github.com/spf13/[email protected]/command.go:911\n  | main.main\n  | \t/home/user/repos/juno/cmd/juno/juno.go:146\n  | runtime.main\n  | \t/usr/lib/go/src/runtime/proc.go:267\n  | runtime.goexit\n  | \t/usr/lib/go/src/runtime/asm_amd64.s:1650\nWraps: (2) leaked iterators: 5\nError types: (1) *withstack.withStack (2) *errutil.leafError"}
github.com/NethermindEth/juno/node.(*Node).Run.func1
	/home/user/repos/juno/node/node.go:245
runtime.gopanic
	/usr/lib/go/src/runtime/panic.go:914
github.com/NethermindEth/juno/db.discardTxnOnPanic
	/home/user/repos/juno/db/db.go:116
runtime.gopanic
	/usr/lib/go/src/runtime/panic.go:914
github.com/cockroachdb/pebble.(*Batch).grow
	/home/user/.go/pkg/mod/github.com/cockroachdb/[email protected]/batch.go:1237
github.com/cockroachdb/pebble.(*Batch).prepareDeferredKeyValueRecord
	/home/user/.go/pkg/mod/github.com/cockroachdb/[email protected]/batch.go:527
github.com/cockroachdb/pebble.(*Batch).SetDeferred
	/home/user/.go/pkg/mod/github.com/cockroachdb/[email protected]/batch.go:623
github.com/cockroachdb/pebble.(*Batch).Set
	/home/user/.go/pkg/mod/github.com/cockroachdb/[email protected]/batch.go:605
github.com/NethermindEth/juno/db/pebble.(*Transaction).Set
	/home/user/repos/juno/db/pebble/transaction.go:68
github.com/NethermindEth/juno/migration.changeStateDiffStruct
	/home/user/repos/juno/migration/migration.go:627
github.com/NethermindEth/juno/migration.(*BucketMigrator).Migrate
	/home/user/repos/juno/migration/bucket_migrator.go:102
github.com/NethermindEth/juno/migration.migrateIfNeeded.func1
	/home/user/repos/juno/migration/migration.go:106
github.com/NethermindEth/juno/db.Update
	/home/user/repos/juno/db/db.go:104
github.com/NethermindEth/juno/db/pebble.(*DB).Update
	/home/user/repos/juno/db/pebble/db.go:106
github.com/NethermindEth/juno/migration.migrateIfNeeded
	/home/user/repos/juno/migration/migration.go:105
github.com/NethermindEth/juno/migration.MigrateIfNeeded
	/home/user/repos/juno/migration/migration.go:69
github.com/NethermindEth/juno/node.(*Node).Run
	/home/user/repos/juno/node/node.go:262
main.main.func2
	/home/user/repos/juno/cmd/juno/juno.go:142
github.com/spf13/cobra.(*Command).execute
	/home/user/.go/pkg/mod/github.com/spf13/[email protected]/command.go:872
github.com/spf13/cobra.(*Command).ExecuteC
	/home/user/.go/pkg/mod/github.com/spf13/[email protected]/command.go:990
github.com/spf13/cobra.(*Command).Execute
	/home/user/.go/pkg/mod/github.com/spf13/[email protected]/command.go:918
github.com/spf13/cobra.(*Command).ExecuteContext
	/home/user/.go/pkg/mod/github.com/spf13/[email protected]/command.go:911
main.main
	/home/user/repos/juno/cmd/juno/juno.go:146
runtime.main
	/usr/lib/go/src/runtime/proc.go:267
panic: pebble: batch too large: >= 4.0 G [recovered]
	panic: pebble: batch too large: >= 4.0 G

goroutine 1 [running]:
github.com/NethermindEth/juno/db.discardTxnOnPanic({0x32a2540, 0xc00033af00})
	/home/user/repos/juno/db/db.go:116 +0xa5
panic({0x2cb6780?, 0xc00055f200?})
	/usr/lib/go/src/runtime/panic.go:914 +0x21f
github.com/cockroachdb/pebble.(*Batch).grow(0xc000680000?, 0x526618?)
	/home/user/.go/pkg/mod/github.com/cockroachdb/[email protected]/batch.go:1237 +0x12d
github.com/cockroachdb/pebble.(*Batch).prepareDeferredKeyValueRecord(0xc0001621c0, 0x9, 0x802e, 0x1)
	/home/user/.go/pkg/mod/github.com/cockroachdb/[email protected]/batch.go:527 +0x86
github.com/cockroachdb/pebble.(*Batch).SetDeferred(...)
	/home/user/.go/pkg/mod/github.com/cockroachdb/[email protected]/batch.go:623
github.com/cockroachdb/pebble.(*Batch).Set(0xc0001621c0, {0xc00f43a006, 0x9, 0x40?}, {0xc00f494000, 0x802e, 0xc083a97b40?}, 0x7f35865f25b8?)
	/home/user/.go/pkg/mod/github.com/cockroachdb/[email protected]/batch.go:605 +0x3b
github.com/NethermindEth/juno/db/pebble.(*Transaction).Set(0xc00033af00, {0xc00f43a006, 0x9, 0x9}, {0xc00f494000, 0x802e, 0x802e})
	/home/user/repos/juno/db/pebble/transaction.go:68 +0x14b
github.com/NethermindEth/juno/migration.changeStateDiffStruct({0x32a2540, 0xc00033af00}, {0xc00f43a006, 0x9, 0x9}, {0xc00f454000, 0x9287, 0x9287}, 0x2?)
	/home/user/repos/juno/migration/migration.go:627 +0x78b
github.com/NethermindEth/juno/migration.(*BucketMigrator).Migrate(0xc000643f40, {0x4b9a25?, 0x28?}, {0x32a2540, 0xc00033af00}, 0xc000e0fa18?)
	/home/user/repos/juno/migration/bucket_migrator.go:102 +0x28e
github.com/NethermindEth/juno/migration.migrateIfNeeded.func1({0x32a2540, 0xc00033af00})
	/home/user/repos/juno/migration/migration.go:106 +0x5f
github.com/NethermindEth/juno/db.Update({0x32a0138?, 0xc000638300?}, 0xc000280980)
	/home/user/repos/juno/db/db.go:104 +0x9e
github.com/NethermindEth/juno/db/pebble.(*DB).Update(0xc000025279?, 0x2f66585?)
	/home/user/repos/juno/db/pebble/db.go:106 +0x25
github.com/NethermindEth/juno/migration.migrateIfNeeded({0x329a5d8?, 0xc0006440a0}, {0x32a0138, 0xc000638300}, 0x2, {0x329a798, 0xc000574050}, {0x4549da0, 0xf, 0xf})
	/home/user/repos/juno/migration/migration.go:105 +0x35d
github.com/NethermindEth/juno/migration.MigrateIfNeeded(...)
	/home/user/repos/juno/migration/migration.go:69
github.com/NethermindEth/juno/node.(*Node).Run(0xc0001ffbc0, {0x329a5d8, 0xc0006440a0})
	/home/user/repos/juno/node/node.go:262 +0x409
main.main.func2(0xc0004b8000, {0x2e07bf5?, 0x7?, 0x2dd7ebf?})
	/home/user/repos/juno/cmd/juno/juno.go:142 +0xbb
github.com/spf13/cobra.(*Command).execute(0xc0004b8000, {0xc000146130, 0x5, 0x5})
	/home/user/.go/pkg/mod/github.com/spf13/[email protected]/command.go:872 +0x6aa
github.com/spf13/cobra.(*Command).ExecuteC(0xc0004b8000)
	/home/user/.go/pkg/mod/github.com/spf13/[email protected]/command.go:990 +0x38d
github.com/spf13/cobra.(*Command).Execute(...)
	/home/user/.go/pkg/mod/github.com/spf13/[email protected]/command.go:918
github.com/spf13/cobra.(*Command).ExecuteContext(...)
	/home/user/.go/pkg/mod/github.com/spf13/[email protected]/command.go:911
main.main()
	/home/user/repos/juno/cmd/juno/juno.go:146 +0x17d

joshklop · 2023-11-29T04:47:05Z

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 :/

omerfirmak · 2023-11-29T15:05:20Z

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 BucketMigrator, it should speed things up quite a lot.

joshklop · 2023-12-02T13:50:23Z

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.

omerfirmak · 2023-12-05T09:13:14Z

This assumes the transactions modify disjoint sets of keys, which is safe in our case.

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.

joshklop · 2023-12-05T20:22:21Z

This is way too risky of an assumption to make.

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?

…igration

…counterpart

mainnet migration fails with 100k

joshklop force-pushed the joshklop/statediff branch from 3b275c1 to 2fe819a Compare November 26, 2023 03:01

joshklop linked an issue Nov 26, 2023 that may be closed by this pull request

Update core.StateDiffs.StorageDiffs type #1437

Closed

joshklop force-pushed the joshklop/statediff branch 2 times, most recently from b1d69e4 to 4971ec3 Compare November 26, 2023 19:21

joshklop force-pushed the joshklop/statediff branch 2 times, most recently from 71c2463 to 2944a39 Compare November 28, 2023 01:00

joshklop marked this pull request as draft November 28, 2023 04:10

joshklop force-pushed the joshklop/statediff branch from 2944a39 to 87ca2e1 Compare November 29, 2023 04:46

joshklop force-pushed the joshklop/statediff branch from 87ca2e1 to 359ba0c Compare December 2, 2023 13:41

omerfirmak mentioned this pull request Dec 6, 2023

Add block builder state implementation #1526

Closed

joshklop force-pushed the joshklop/statediff branch 3 times, most recently from 03fd6fe to fa2f245 Compare December 6, 2023 21:36

joshklop and others added 4 commits December 6, 2023 15:42

Use maps instead of slices in state diff struct

fd6673c

Add state diff migration

fa2f245

Fix trying to fetch Cairo1 classes by their compiled class hash

aec0513

Make extra sure no values are getting misplaced/shuffled during the m…

6212c90

…igration

omerfirmak force-pushed the joshklop/statediff branch from 694706e to 6212c90 Compare December 7, 2023 10:46

joshklop requested a review from omerfirmak December 7, 2023 12:05

joshklop marked this pull request as ready for review December 7, 2023 12:05

stop duplicating local loop variable when it is safe

5cc262d

joshklop force-pushed the joshklop/statediff branch from e04d232 to a663601 Compare December 8, 2023 03:34

omerfirmak and others added 3 commits December 7, 2023 21:39

somewhat align rpc state update types with the changes in their core …

3661f98

…counterpart

Add Clone() method to felt.Felt struct

a663601

Reduce batch size of migration

1f341bf

mainnet migration fails with 100k

omerfirmak approved these changes Dec 8, 2023

View reviewed changes

omerfirmak merged commit d3b73d1 into main Dec 8, 2023
9 of 10 checks passed

omerfirmak deleted the joshklop/statediff branch December 8, 2023 08:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use maps instead of slices in state diff struct #1466

Use maps instead of slices in state diff struct #1466

joshklop commented Nov 26, 2023

joshklop commented Nov 26, 2023 •

edited

Loading

codecov bot commented Nov 26, 2023 •

edited

Loading

omerfirmak commented Nov 27, 2023 •

edited

Loading

joshklop commented Nov 27, 2023

omerfirmak commented Nov 27, 2023

joshklop commented Nov 27, 2023

joshklop commented Nov 28, 2023

joshklop commented Nov 29, 2023

omerfirmak commented Nov 29, 2023

joshklop commented Dec 2, 2023

omerfirmak commented Dec 5, 2023

joshklop commented Dec 5, 2023

Use maps instead of slices in state diff struct #1466

Use maps instead of slices in state diff struct #1466

Conversation

joshklop commented Nov 26, 2023

joshklop commented Nov 26, 2023 • edited Loading

codecov bot commented Nov 26, 2023 • edited Loading

Codecov Report

omerfirmak commented Nov 27, 2023 • edited Loading

joshklop commented Nov 27, 2023

omerfirmak commented Nov 27, 2023

joshklop commented Nov 27, 2023

joshklop commented Nov 28, 2023

joshklop commented Nov 29, 2023

omerfirmak commented Nov 29, 2023

joshklop commented Dec 2, 2023

omerfirmak commented Dec 5, 2023

joshklop commented Dec 5, 2023

joshklop commented Nov 26, 2023 •

edited

Loading

codecov bot commented Nov 26, 2023 •

edited

Loading

omerfirmak commented Nov 27, 2023 •

edited

Loading