-
Notifications
You must be signed in to change notification settings - Fork 587
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
fix(runtime): disallow deletion of account with large state #4116
Conversation
51ab12c
to
be0504c
Compare
"num_block_producer_seats": 50, | ||
"protocol_version": 43, | ||
"genesis_time": "1970-01-01T00:00:00.000000000Z", | ||
"chain_id": "sample", |
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.
trailing ws?
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.
hmm this file is auto generated
if current_protocol_version | ||
>= PROTOCOL_FEATURES_TO_VERSION_MAPPING[&ProtocolFeature::DeleteActionRestriction] |
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.
unrelated to PR, but feels like we can have a nicer API: current_protocol_version.has_feature(ProtocolFeature::DeleteActionRestriction)
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.
That is a good idea!
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
@@ -458,7 +457,7 @@ pub(crate) fn action_delete_account( | |||
// account storage usage should be larger than code size | |||
let code_len = code.code.len() as u64; | |||
debug_assert!(account_storage_usage > code_len); |
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.
We don't run debug_assert
in prod, right? So it's only for tests
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.
yes
For account whose state is large, deleting them is not cheap and this PR prevents accounts with more than 10kb of state (excluding contracts) from being deleted. Contract developers should develop their own logic to delete part of the state if needed. Test plan --------- * `test_delete_account_too_large` * `test_delete_account_with_contract_and_small_state` * `test_delete_account_with_contract_and_large_state` * nayduck
For account whose state is large, deleting them is not cheap and this PR prevents accounts with more than 10kb of state (excluding contracts) from being deleted. Contract developers should develop their own logic to delete part of the state if needed.
Test plan
test_delete_account_too_large
test_delete_account_with_contract_and_small_state
test_delete_account_with_contract_and_large_state