refactor(blockifier_reexecution): add chain ID to SerializableOfflineReexecutionData

[dorimedini-starkware]

No description provided.

[dorimedini-starkware]

• refactor(blockifier_reexecution): allow overriding chain ID explicitly #1974
• refactor(blockifier_reexecution): set chain ID by inspecting RPC URL #1973
• refactor(blockifier_reexecution): add chain ID to SerializableOfflineReexecutionData #1972 👈 (View in Graphite)
• refactor(blockifier_reexecution): add chain ID to TestStateReader #1971
• refactor(blockifier_reexecution): inject chain ID to chain info creation #1970
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[reviewable-StarkWare]

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 42.17%. Comparing base (e3165c4) to head (60702e2).
Report is 506 commits behind head on main.

Files with missing lines	Patch %	Lines
...s/blockifier_reexecution/src/state_reader/utils.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1972      +/-   ##
==========================================
+ Coverage   40.10%   42.17%   +2.06%     
==========================================
  Files          26      204     +178     
  Lines        1895    23935   +22040     
  Branches     1895    23935   +22040     
==========================================
+ Hits          760    10094    +9334     
- Misses       1100    13375   +12275     
- Partials       35      466     +431     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests
• JS Bundle Analysis - Avoid shipping oversized bundles

[aner-starkware]

Reviewable status: 0 of 221 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @dorimedini-starkware)

---

crates/blockifier_reexecution/src/main.rs line 137 at r1 (raw file):

                serializable_data_prev_block,
                serializable_data_next_block,
                chain_id: ChainId::Mainnet,

Is this really what we want? I mean, we probably want to save data for reexecution tests for Mainnet; do we also want to save and run tests for other networks; If not, then it should just be explicitly entered in the CLI, not saved in the file.

Code quote:

chain_id: ChainId::Mainnet,

[dorimedini-starkware]

Reviewable status: 0 of 221 files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @AvivYossef-starkware)

---

crates/blockifier_reexecution/src/main.rs line 137 at r1 (raw file):

Previously, aner-starkware wrote…

Is this really what we want? I mean, we probably want to save data for reexecution tests for Mainnet; do we also want to save and run tests for other networks; If not, then it should just be explicitly entered in the CLI, not saved in the file.

still relevant?
the idea is:

1. RPC tests guess the chain ID from the node URL; or, get the chain ID explicitly if you want to override
2. offline tests need to fetch the chain ID from the JSON (hence the addition of the chain ID to the JSON struct)

[aner-starkware]

crates/blockifier_reexecution/src/main.rs line 137 at r1 (raw file):

Previously, dorimedini-starkware wrote…

still relevant?
the idea is:

1. RPC tests guess the chain ID from the node URL; or, get the chain ID explicitly if you want to override
2. offline tests need to fetch the chain ID from the JSON (hence the addition of the chain ID to the JSON struct)

Are we planning to run reexecution in other networks often or in tests?

[dorimedini-starkware]

Reviewable status: 0 of 221 files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @AvivYossef-starkware)

---

crates/blockifier_reexecution/src/main.rs line 137 at r1 (raw file):

Previously, aner-starkware wrote…

Are we planning to run reexecution in other networks often or in tests?

if we see interesting edge cases pop up in other networks, yes, I would like to be prepared.
maybe it will never but used in the CI but it was already useful for me, to manually test specific blocks from testnet

[aner-starkware]

crates/blockifier_reexecution/src/main.rs line 137 at r1 (raw file):

Previously, dorimedini-starkware wrote…

if we see interesting edge cases pop up in other networks, yes, I would like to be prepared.
maybe it will never but used in the CI but it was already useful for me, to manually test specific blocks from testnet

I understand that testing in other networks is useful, but why do we need to store the network in the file?

[dorimedini-starkware]

Reviewable status: 0 of 221 files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @AvivYossef-starkware)

---

crates/blockifier_reexecution/src/main.rs line 137 at r1 (raw file):

Previously, aner-starkware wrote…

I understand that testing in other networks is useful, but why do we need to store the network in the file?

because we need the block info for offline execution, right? the chain info requires knowledge of the network (in principle - in practice I think since the fee token addresses are the same it isn't really required, but good to have this for forward compatibility). no?

[aner-starkware]

crates/blockifier_reexecution/src/main.rs line 137 at r1 (raw file):

Previously, dorimedini-starkware wrote…

because we need the block info for offline execution, right? the chain info requires knowledge of the network (in principle - in practice I think since the fee token addresses are the same it isn't really required, but good to have this for forward compatibility). no?

I don't think it's necessary to add it right to the file—this seems like something we might never really use, and if we need it, the user can add the network via CLI. Adding the network to the file means we plan to make tests that rely on it, but at the moment, there's nothing to suggest such tests will ever be needed. For all the current networks, the behaviour is the same.

[aner-starkware]

crates/blockifier_reexecution/src/main.rs line 137 at r1 (raw file):

Previously, aner-starkware wrote…

I don't think it's necessary to add it right to the file—this seems like something we might never really use, and if we need it, the user can add the network via CLI. Adding the network to the file means we plan to make tests that rely on it, but at the moment, there's nothing to suggest such tests will ever be needed. For all the current networks, the behaviour is the same.

*right now

[aner-starkware]

Reviewed all commit messages.
Reviewable status: 0 of 221 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @dorimedini-starkware)

[aner-starkware]

Reviewed 2 of 221 files at r2, 219 of 219 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @dorimedini-starkware)

[dorimedini-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @AvivYossef-starkware)

---

crates/blockifier_reexecution/src/main.rs line 137 at r1 (raw file):

Previously, aner-starkware wrote…

*right now

resolved offline

[github-actions]

Artifacts upload triggered. View details here

[aner-starkware]

Reviewable status: 115 of 221 files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)

[aner-starkware]

Reviewed 106 of 106 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)

[dorimedini-starkware]

Merge activity

• Nov 19, 10:11 AM EST: A user started a stack merge that includes this pull request via Graphite.
• Nov 19, 10:12 AM EST: A user merged this pull request with Graphite.