-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(blockifier): implement the allocated keys logic #2150
Conversation
Artifacts upload triggered. View details here |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2150 +/- ##
===========================================
+ Coverage 40.10% 68.78% +28.67%
===========================================
Files 26 108 +82
Lines 1895 13957 +12062
Branches 1895 13957 +12062
===========================================
+ Hits 760 9600 +8840
- Misses 1100 3946 +2846
- Partials 35 411 +376 ☔ View full report in Codecov by Sentry. |
34effde
to
c25c984
Compare
9a2703b
to
447c9de
Compare
Artifacts upload triggered. View details here |
c25c984
to
63e3201
Compare
447c9de
to
785b347
Compare
Artifacts upload triggered. View details here |
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 3 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs)
a discussion (no related file):
add some test scenarios?
some cases to consider:
- validate 0->7, execute 7->0, no charge
- validate 0->7, execute 7->8, a single charge
- tx0 0->7, tx1 7->8, tx2 8->0, tx3 0->7: only txs 0 and 3 are charged
crates/blockifier/src/state/cached_state.rs
line 694 at r1 (raw file):
self.0.remove(k); } });
I think this is clearer (non blocking though)
Suggestion:
for key, value in state_change.state_maps.storage.iter() {
if value == &Felt::ZERO {
self.0.remove(key);
} else {
self.insert(key);
}
}
crates/blockifier/src/state/cached_state.rs
line 689 at r3 (raw file):
pub fn extend(&mut self, state_change: &StateChanges) { self.0.extend(&state_change.allocated_keys.0); // Remove keys that are set back to zero.
Suggestion:
// Remove keys that are set back to zero.
// Ignore all but storage entry allocations - newly allocated contract addresses and
// class hashes are paid for separately.
// TODO: Re-price declare, deploy_account and deploy syscalls.
63e3201
to
2d9b007
Compare
785b347
to
6fc7042
Compare
Artifacts upload triggered. View details here |
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 r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs)
crates/blockifier/src/state/cached_state_test.rs
line 457 at r4 (raw file):
let merged_changes = StateChanges::merge(state_changes); assert_ne!(merged_changes.allocated_keys.is_empty(), charged); }
I prefer a test with account txs running - to check specifically the flow between validate and execute, and between consecutive txs.
the result of this test is tightly coupled to how you use the transactional state, no guarantee here that you "mimic" the real behavior 1:1
Code quote:
#[rstest]
#[case(false, vec![felt!("0x0")], false)]
#[case(true, vec![felt!("0x7")], true)]
#[case(false, vec![felt!("0x7")], false)]
#[case(true, vec![felt!("0x7"), felt!("0x0")], false)]
#[case(false, vec![felt!("0x0"), felt!("0x8")], true)]
#[case(false, vec![felt!("0x0"), felt!("0x8"), felt!("0x0")], false)]
fn test_allocated_keys_update(
#[case] is_base_empty: bool,
#[case] storage_updates: Vec<Felt>,
#[case] charged: bool,
) {
let contract_address = contract_address!(CONTRACT_ADDRESS);
let storage_key = StorageKey::from(0x10_u16);
// Set initial state
let mut state: CachedState<DictStateReader> = CachedState::default();
if !is_base_empty {
state.set_storage_at(contract_address, storage_key, felt!("0x1")).unwrap();
}
let mut state_changes = vec![];
for value in storage_updates {
// In the end of the previous loop, state has moved into the transactional state.
let mut transactional_state = TransactionalState::create_transactional(&mut state);
// Update state and collect the state changes.
transactional_state.set_storage_at(contract_address, storage_key, value).unwrap();
state_changes.push(transactional_state.get_actual_state_changes().unwrap());
transactional_state.commit();
}
let merged_changes = StateChanges::merge(state_changes);
assert_ne!(merged_changes.allocated_keys.is_empty(), charged);
}
2d9b007
to
2e91cee
Compare
6fc7042
to
0410cf2
Compare
Artifacts upload triggered. View details here |
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 r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs)
2e91cee
to
13f1440
Compare
0410cf2
to
81ae09a
Compare
bbaf49e
to
842b806
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: 1 of 5 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware and @nimrod-starkware)
a discussion (no related file):
Previously, dorimedini-starkware wrote…
add some test scenarios?
some cases to consider:
- validate 0->7, execute 7->0, no charge
- validate 0->7, execute 7->8, a single charge
- tx0 0->7, tx1 7->8, tx2 8->0, tx3 0->7: only txs 0 and 3 are charged
Done.
crates/blockifier/src/state/cached_state.rs
line 694 at r1 (raw file):
Previously, dorimedini-starkware wrote…
I think this is clearer (non blocking though)
This is not the same logic.
I insert the given allocated keys and remove the zero storage values.
crates/blockifier/src/state/cached_state.rs
line 689 at r3 (raw file):
pub fn extend(&mut self, state_change: &StateChanges) { self.0.extend(&state_change.allocated_keys.0); // Remove keys that are set back to zero.
All changes here are only about storage allocations; we didn't mention anything else.
Put this TODO on Monday?
crates/blockifier/src/transaction/account_transactions_test.rs
line 1398 at r6 (raw file):
..Default::default() }, n_allocated_keys: 2,
@dorimedini-starkware
I'm unsure why the expected value is 2 (and not 3).
Code quote:
n_allocated_keys: 2,
842b806
to
137af70
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: 1 of 5 files reviewed, 9 unresolved discussions (waiting on @dorimedini-starkware, @nimrod-starkware, and @yoavGrs)
crates/blockifier/src/transaction/account_transactions_test.rs
line 1487 at r8 (raw file):
..Default::default() }, // The recipient storage add
Fix
Code quote:
/ The recipient storage add
crates/blockifier/src/state/cached_state.rs
line 684 at r8 (raw file):
#[cfg_attr(any(feature = "testing", test), derive(Clone))] #[derive(Debug, Default, Eq, PartialEq)] pub struct AllocatedKeys(HashSet<StorageEntry>);
Add a short docstring
Code quote:
pub struct AllocatedKeys(HashSet<StorageEntry>);
crates/blockifier/src/state/cached_state.rs
line 688 at r8 (raw file):
impl AllocatedKeys { /// Extend the set of allocated keys with the allocated_keys of the given state changes. /// Remove storage keys that are set back to zero.
Suggestion:
/// Extends the set of allocated keys with the allocated_keys of the given state changes.
/// Removes storage keys that are set back to zero.
crates/blockifier/src/state/cached_state.rs
line 707 at r8 (raw file):
} /// Collect entries that turn zero -> nonzero.
Suggestion:
/// Collects entries that turn zero -> nonzero.
crates/blockifier/src/state/cached_state.rs
line 724 at r8 (raw file):
} }) .collect(),
Suggestion:
updated_storage
.iter()
.filter_map(|(k, v)| {
let base_value = base_storage.get(k).unwrap_or(Felt::ZERO);
if *v != Felt::ZERO && base_value == Felt::ZERO)
{
Some(*k)
} else {
None
}
})
.collect(),
137af70
to
05dbcf7
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: 1 of 5 files reviewed, 9 unresolved discussions (waiting on @dorimedini-starkware and @nimrod-starkware)
crates/blockifier/src/state/cached_state_test.rs
line 457 at r4 (raw file):
Previously, dorimedini-starkware wrote…
I prefer a test with account txs running - to check specifically the flow between validate and execute, and between consecutive txs.
the result of this test is tightly coupled to how you use the transactional state, no guarantee here that you "mimic" the real behavior 1:1
I added test_allocated_keys_two_transactions
that copied the behavior of validate and execute (but still, it's not guaranteed).
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 4 files at r6, 1 of 1 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: 4 of 5 files reviewed, 11 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs)
crates/blockifier/src/state/cached_state.rs
line 694 at r1 (raw file):
Previously, yoavGrs wrote…
This is not the same logic.
I insert the given allocated keys and remove the zero storage values.
I meant for key in allocated_keys: if state_maps[k] != 0: insert(key)
crates/blockifier/src/state/cached_state.rs
line 689 at r3 (raw file):
Previously, yoavGrs wrote…
All changes here are only about storage allocations; we didn't mention anything else.
Put this TODO on Monday?
The Ignore all but ...
comment is just for clarity: addresses and class hashes are allocations, but they are not counted here, because here we are counting for fee charge and these special allocations are charged for separately.
If you think it's redundant, I'll unblock.
Yes, the TODO can (should) be on Monday, can you add it?
crates/blockifier/src/state/cached_state_test.rs
line 457 at r4 (raw file):
Previously, yoavGrs wrote…
I added
test_allocated_keys_two_transactions
that copied the behavior of validate and execute (but still, it's not guaranteed).
why not use account transaction testing framework?
crates/blockifier/src/transaction/account_transactions_test.rs
line 1398 at r6 (raw file):
Previously, yoavGrs wrote…
@dorimedini-starkware
I'm unsure why the expected value is 2 (and not 3).
what are the 3 allocations you expect?
try injecting prints or something to see which key isn't included in your AllocatedKeys object
crates/blockifier/src/state/cached_state_test.rs
line 455 at r9 (raw file):
// Test that `allocated_keys` collects zero -> nonzero updates, where the TransactionalState holds // the previous state as base.
you should not be testing the TransactionalState, I think you should be testing account tx behavior (higher level) - how this "squash" is implenented may be unrelated to the TransactionalState object
Suggestion:
/// Test that allocations in validate and execute phases are properly squashed.
crates/blockifier/src/state/cached_state_test.rs
line 460 at r9 (raw file):
#[case(true, felt!("0x0"), felt!("0x8"), true)] #[case(true, felt!("0x7"), felt!("0x7"), true)] #[case(false, felt!("0x0"), felt!("0x8"), true)]
why would we expect an allocation in this case? this is the 8->0->8 case;
if this is a bug please mark this case with a TODO
Code quote:
#[case(false, felt!("0x0"), felt!("0x8"), true)]
crates/blockifier/src/state/cached_state_test.rs
line 465 at r9 (raw file):
#[case] is_base_empty: bool, #[case] first_value: Felt, #[case] second_value: Felt,
Suggestion:
#[case] validate_value: Felt,
#[case] execute_value: Felt,
crates/blockifier/src/state/cached_state_test.rs
line 484 at r9 (raw file):
let second_state_changes = second_state.get_actual_state_changes().unwrap(); let merged_changes = StateChanges::merge(vec![first_state_changes, second_state_changes]);
same remark as with the other test - use account transaction testing framework, so you don't have to mimic usage of TransactionalState
Code quote:
let contract_address = contract_address!(CONTRACT_ADDRESS);
let storage_key = StorageKey::from(0x10_u16);
// Set initial state
let mut state: CachedState<DictStateReader> = CachedState::default();
if !is_base_empty {
state.set_storage_at(contract_address, storage_key, felt!("0x1")).unwrap();
}
let mut first_state = TransactionalState::create_transactional(&mut state);
first_state.set_storage_at(contract_address, storage_key, first_value).unwrap();
let first_state_changes = first_state.get_actual_state_changes().unwrap();
let mut second_state = TransactionalState::create_transactional(&mut first_state);
second_state.set_storage_at(contract_address, storage_key, second_value).unwrap();
let second_state_changes = second_state.get_actual_state_changes().unwrap();
let merged_changes = StateChanges::merge(vec![first_state_changes, second_state_changes]);
2c19800
to
9a147b1
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 1 of 1 files at r7.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs)
05dbcf7
to
db3a6a1
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: all files reviewed, 10 unresolved discussions (waiting on @dorimedini-starkware, @nimrod-starkware, and @Yoni-Starkware)
crates/blockifier/src/state/cached_state.rs
line 694 at r1 (raw file):
Previously, dorimedini-starkware wrote…
I meant
for key in allocated_keys: if state_maps[k] != 0: insert(key)
In my version, the function removes existing allocated keys (not only from the given `allocated keys), that turn to zero in the given storage.
crates/blockifier/src/state/cached_state.rs
line 689 at r3 (raw file):
Previously, dorimedini-starkware wrote…
The
Ignore all but ...
comment is just for clarity: addresses and class hashes are allocations, but they are not counted here, because here we are counting for fee charge and these special allocations are charged for separately.
If you think it's redundant, I'll unblock.
Yes, the TODO can (should) be on Monday, can you add it?
Done, in the class docstring and in Monday.
crates/blockifier/src/state/cached_state.rs
line 684 at r8 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Add a short docstring
Done.
crates/blockifier/src/state/cached_state_test.rs
line 457 at r4 (raw file):
Previously, dorimedini-starkware wrote…
why not use account transaction testing framework?
I will add one in a separate PR.
This requires defining a new test contract.
crates/blockifier/src/state/cached_state_test.rs
line 455 at r9 (raw file):
Previously, dorimedini-starkware wrote…
you should not be testing the TransactionalState, I think you should be testing account tx behavior (higher level) - how this "squash" is implenented may be unrelated to the TransactionalState object
Done.
crates/blockifier/src/state/cached_state_test.rs
line 460 at r9 (raw file):
Previously, dorimedini-starkware wrote…
why would we expect an allocation in this case? this is the 8->0->8 case;
if this is a bug please mark this case with a TODO
Done.
crates/blockifier/src/transaction/account_transactions_test.rs
line 1398 at r6 (raw file):
Previously, dorimedini-starkware wrote…
what are the 3 allocations you expect?
try injecting prints or something to see which key isn't included in your AllocatedKeys object
I find it.
crates/blockifier/src/transaction/account_transactions_test.rs
line 1487 at r8 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Fix
Done.
crates/blockifier/src/state/cached_state_test.rs
line 465 at r9 (raw file):
#[case] is_base_empty: bool, #[case] first_value: Felt, #[case] second_value: Felt,
Done.
crates/blockifier/src/state/cached_state.rs
line 688 at r8 (raw file):
impl AllocatedKeys { /// Extend the set of allocated keys with the allocated_keys of the given state changes. /// Remove storage keys that are set back to zero.
Done.
crates/blockifier/src/state/cached_state.rs
line 707 at r8 (raw file):
} /// Collect entries that turn zero -> nonzero.
Done.
9a147b1
to
8533938
Compare
db3a6a1
to
5717e9c
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 4 of 8 files at r11, all commit messages.
Reviewable status: 5 of 9 files reviewed, 10 unresolved discussions (waiting on @nimrod-starkware, @yoavGrs, and @Yoni-Starkware)
crates/blockifier/src/state/cached_state.rs
line 694 at r1 (raw file):
Previously, yoavGrs wrote…
In my version, the function removes existing allocated keys (not only from the given `allocated keys), that turn to zero in the given storage.
not sure i understand.. consider:
for (k, v) in state_change.state_maps.storage {
if v == 0 { self.remove(k) } else { self.insert(k) }
}
what will be the difference?
- allocated keys that are not part of
state_change.state_maps.storage
? i.e. class hashes can be in allocated_keys but are not in storage - ... anything else?
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 4 of 8 files at r11.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @nimrod-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.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @nimrod-starkware and @Yoni-Starkware)
crates/blockifier/src/state/cached_state.rs
line 694 at r1 (raw file):
Previously, dorimedini-starkware wrote…
not sure i understand.. consider:
for (k, v) in state_change.state_maps.storage { if v == 0 { self.remove(k) } else { self.insert(k) } }
what will be the difference?
- allocated keys that are not part of
state_change.state_maps.storage
? i.e. class hashes can be in allocated_keys but are not in storage- ... anything else?
You will count a key that was updated but not allocated.
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: all files reviewed, 4 unresolved discussions (waiting on @nimrod-starkware and @Yoni-Starkware)
crates/blockifier/src/state/cached_state.rs
line 694 at r1 (raw file):
Previously, yoavGrs wrote…
You will count a key that was updated but not allocated.
ah, right! nice
5717e9c
to
115054e
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 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @nimrod-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 all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware)
No description provided.