-
Notifications
You must be signed in to change notification settings - Fork 621
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
Restore receipts after recent fix #4248
Conversation
5b10e61
to
c116d83
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.
Please add a description to the PR on what this PR does and what the test plan is
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
// True iff the current block is the first one in the chain with the current version | ||
pub is_first_block_of_version: bool, | ||
// True iff the current block containing chunk for some specific shard is the first one in the | ||
// chain with the current version |
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.
This seems to be only true if first epoch of the current version have block with chunk (and also currently only for shard 0)
@@ -272,15 +282,29 @@ lazy_static_include::lazy_static_include_bytes! { | |||
MAINNET_STORAGE_USAGE_DELTA => "res/storage_usage_delta.json", | |||
} | |||
|
|||
#[cfg(feature = "protocol_feature_restore_receipts_after_fix")] | |||
lazy_static_include::lazy_static_include_bytes! { |
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.
Maybe swap this with next constant so that things related to single migration is consecutive
chain/chain/src/migrations.rs
Outdated
runtime_adapter.get_epoch_protocol_version(&prev_epoch_id)?; | ||
Ok(protocol_version != prev_epoch_protocol_version) | ||
} | ||
Err(_) => Ok(true), |
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.
This looks very suspicious to me. Have you accounted for all possible ways an attacker can maliciously send you some block that such get_prev_epoch_id_from_prev_block
returns an error?
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've tried to handle error coming from KeyValueRuntime.get_prev_epoch_id_from_prev_block
but now found that my implementation was incorrect. Fixed
chain/chain/src/migrations.rs
Outdated
Ok(chain_store.get_epoch_id_of_last_block_with_chunk(prev_block_hash, shard_id)? | ||
!= runtime_adapter.get_epoch_id_from_prev_block(prev_block_hash)?) |
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.
Please use separate variables to avoid very long expressions like this
@Longarithm @bowenwang1996 Will the restored receipts be stored in the storage (e.g. accessible via P.S. The context for this question is that we need to handle these on Indexer Framework side, and we recently bumped into local receipts that were delayed and had to hotfix the behavior in #4316. /cc @khorolets |
No. We only store receipts that are present in some chunk and here it is a special case. |
Result of applying receipts to current mainnet state: Commit used for testing: 6d2530b Logs:
"Processed: 390, Delayed: 0" shows that all restored receipts were applied, and exit code 0 shows that no issues were happened. Some research of accounts balance.
But receipts in considered block also affect this account. There are two such ones, and
Same thing was checked for two previous blocks. |
This was one-off thing we needed a while ago. We no longer use it, so there's no need to support it. The code can always be restored from the git history. cc near#4248
This was one-off thing we needed a while ago. We no longer use it, so there's no need to support it. The code can always be restored from the git history. cc #4248
Currently rebased on #4274 to reuse
MigrationData
.This PR has two goals:
Test plan
Check that
restored-receipts-verifier -> test_checking_differences
process_blocks -> test_restoring_receipts_mainnet