Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

fix(concurrency): add assert message to the declared_contracts assertion in validate_reads #1981

Conversation

OriStarkware
Copy link
Contributor

@OriStarkware OriStarkware commented Jun 13, 2024

…reads


This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.42%. Comparing base (6e06e79) to head (613320f).
Report is 1 commits behind head on main.

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

@OriStarkware OriStarkware force-pushed the ori/starknet/concurrency/versioned_state/add_test_validate_reads_assertion branch 2 times, most recently from e1cc317 to 04b37a1 Compare June 13, 2024 14:04
Copy link
Collaborator

@barak-b-starkware barak-b-starkware left a 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));

Copy link
Collaborator

@noaov1 noaov1 left a 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"

@OriStarkware OriStarkware force-pushed the ori/starknet/concurrency/versioned_state/add_test_validate_reads_assertion branch 3 times, most recently from c86f60c to bf08db9 Compare June 16, 2024 10:20
@OriStarkware OriStarkware changed the title test(concurrency): test the declared_contracts assertion in validate_… fix(concurrency): add assert message to the declared_contracts assertion in validate_reads Jun 16, 2024
Copy link
Contributor Author

@OriStarkware OriStarkware left a 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.

Copy link
Contributor Author

@OriStarkware OriStarkware left a 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 and is_declared == false)

After discussion, we decided that this is an unnecessary test and we should not add it.

@OriStarkware OriStarkware force-pushed the ori/starknet/concurrency/versioned_state/add_test_validate_reads_assertion branch from bf08db9 to 613320f Compare June 16, 2024 10:23
Copy link
Contributor Author

@OriStarkware OriStarkware left a 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)

Copy link
Collaborator

@noaov1 noaov1 left a 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)

Copy link
Collaborator

@barak-b-starkware barak-b-starkware left a 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Yoni-Starkware)

@OriStarkware OriStarkware merged commit b898b1c into main Jun 16, 2024
9 checks passed
@OriStarkware OriStarkware deleted the ori/starknet/concurrency/versioned_state/add_test_validate_reads_assertion branch June 16, 2024 14:43
gswirski pushed a commit to reilabs/blockifier that referenced this pull request Jun 26, 2024
# 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants