-
Notifications
You must be signed in to change notification settings - Fork 107
fix(concurrency): add assert message to the declared_contracts assertion in validate_reads #1981
fix(concurrency): add assert message to the declared_contracts assertion in validate_reads #1981
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1981 +/- ##
=======================================
Coverage 78.42% 78.42%
=======================================
Files 62 62
Lines 8913 8913
Branches 8913 8913
=======================================
Hits 6990 6990
Misses 1476 1476
Partials 447 447 ☔ View full report in Codecov by Sentry. |
e1cc317
to
04b37a1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1, @OriStarkware, and @Yoni-Starkware)
crates/blockifier/src/concurrency/versioned_state.rs
line 125 at r2 (raw file):
is_declared, self.compiled_contract_classes.read(tx_index + 1, class_hash).is_some(), "The declared_contracts mapping should match the compiled_contract_classes mapping"
(Point at the end)
Suggestion:
"The declared_contracts mapping should match the compiled_contract_classes mapping."
crates/blockifier/src/concurrency/versioned_state_test.rs
line 340 at r2 (raw file):
let version_state_proxy = safe_versioned_state.pin_version(0); version_state_proxy.state().declared_contracts.write(0, class_hash!(1_u8), true); assert!(!safe_versioned_state.pin_version(1).validate_reads(&state_maps_to_validate));
Add another flow in which the assertion fails in the opposite way (.is_some() == true
and is_declared == false
)
Code quote:
let state_maps_to_validate = StateMaps {
declared_contracts: HashMap::from([(class_hash!(1_u8), false)]),
..Default::default()
};
let version_state_proxy = safe_versioned_state.pin_version(0);
version_state_proxy.state().declared_contracts.write(0, class_hash!(1_u8), true);
assert!(!safe_versioned_state.pin_version(1).validate_reads(&state_maps_to_validate));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain the motivation for this test?
Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @OriStarkware and @Yoni-Starkware)
crates/blockifier/src/concurrency/versioned_state.rs
line 125 at r2 (raw file):
is_declared, self.compiled_contract_classes.read(tx_index + 1, class_hash).is_some(), "The declared_contracts mapping should match the compiled_contract_classes mapping"
So it won't contaminate the search
Suggestion:
"The declared contracts mapping should match the compiled contract classes mapping"
c86f60c
to
bf08db9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @barak-b-starkware, @noaov1, and @Yoni-Starkware)
crates/blockifier/src/concurrency/versioned_state.rs
line 125 at r2 (raw file):
Previously, barak-b-starkware wrote…
(Point at the end)
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @barak-b-starkware, @noaov1, and @Yoni-Starkware)
crates/blockifier/src/concurrency/versioned_state_test.rs
line 340 at r2 (raw file):
Previously, barak-b-starkware wrote…
Add another flow in which the assertion fails in the opposite way (
.is_some() == true
andis_declared == false
)
After discussion, we decided that this is an unnecessary test and we should not add it.
…ion in validate_reads
bf08db9
to
613320f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am adding a new assert message. The PR initially added a test for the assert, but after discussion we decided that it is an unnecessary test, so I deleted it.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @barak-b-starkware, @noaov1, and @Yoni-Starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @barak-b-starkware and @Yoni-Starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yoni-Starkware)
# Description l1 -> l2 messaging should be using a different hash computation, but instead we were using the hash computation for l2 -> l1 message. this pr adds a new `compute_l1_to_l2_message_hash` function for computing the hash of a l1 -> l2 message. ref : https://docs.starknet.io/documentation/architecture_and_concepts/Network_Architecture/messaging-mechanism/#l1-l2-messages ## Related issue <!-- Please link related issues: Fixes #<issue_number> More info: https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword --> ## Tests <!-- Please refer to the CONTRIBUTING.md file to know more about the testing process. Ensure you've tested at least the package you're modifying if running all the tests consumes too much memory on your system. --> - [ ] Yes - [ ] No, because they aren't needed - [ ] No, because I need help ## Added to documentation? <!-- If the changes are small, code comments are enough, otherwise, the documentation is needed. It may be a README.md file added to your module/package, a DojoBook PR or both. --> - [ ] README.md - [ ] [Dojo Book](https://github.com/dojoengine/book) - [x] No documentation needed ## Checklist - [x] I've formatted my code (`scripts/prettier.sh`, `scripts/rust_fmt.sh`, `scripts/cairo_fmt.sh`) - [x] I've linted my code (`scripts/clippy.sh`, `scripts/docs.sh`) - [x] I've commented my code - [ ] I've requested a review after addressing the comments
…reads
This change is