refactor: prepare shard update impl for stateless validation #10187
Conversation
| prev_block_header: &BlockHeader, | ||
| chunk: &ShardChunk, | ||
| ) -> Result<(), Error> { | ||
| let protocol_version = |
There was a problem hiding this comment.
please change the order of those ifs to have validate_transactions_order first in order to preserve the behaviour of the original code to avoid accidental protocol change:
There was a problem hiding this comment.
I think that's not a protocol change - if there is an error, block with chunk is not saved on chain. But I swapped order by accident anyway
chain/chain/src/update_shard.rs
Outdated
| /// - queries to epoch manager | ||
| /// - allowing contracts to get current chain data | ||
| #[derive(Clone, Debug)] | ||
| pub struct BlockContext { |
There was a problem hiding this comment.
let's make all structs s here pub(crate) instead of pub
|
|
||
| #[derive(Debug)] | ||
| pub struct SameHeightResult { | ||
| pub(crate) shard_uid: ShardUId, |
There was a problem hiding this comment.
I guest we can just make it pub if SameHeightResult is pub(crate)
There was a problem hiding this comment.
pub(crate) worked for Context-s, but didn't work for ApplyChunkResult. It is actually leaked to other crates in BlockCatchUpRequest
It doesn't look right, but it is outside of scope of the PR I think
chain/chain/src/update_shard.rs
Outdated
| } | ||
|
|
||
| /// Reason to update a shard when new block appears on chain. | ||
| pub enum ShardUpdateReason { |
There was a problem hiding this comment.
nit: I suggest just calling it ShardUpdate
There was a problem hiding this comment.
Discussed offline. Can choose ShardUpdateMode and rename ApplyChunksMode in chain.rs later
| // Before `FixApplyChunks` feature, gas price was taken from current | ||
| // block by mistake. Preserve it for backwards compatibility. | ||
| let gas_price = if !is_new_chunk | ||
| && protocol_version < ProtocolFeature::FixApplyChunks.protocol_version() | ||
| { | ||
| block_header.next_gas_price() | ||
| } else { | ||
| prev_block_header.next_gas_price() | ||
| }; |
There was a problem hiding this comment.
nit: Can you move it to a helper method?
There was a problem hiding this comment.
Move what exactly? :) The logic for getting gas price is very simple, the whole method is quite small.
chain/chain/src/update_shard.rs
Outdated
| shard_info.shard_uid, | ||
| runtime, | ||
| epoch_manager, | ||
| split_state_roots.unwrap(), |
There was a problem hiding this comment.
I suppose this is safe dynamically but it still looks a bit scary. Can you make the split_state_roots part of the reason where it can be already checked to be not none and typed as such?
There was a problem hiding this comment.
It makes Reason struct more ugly... But that's fine for now
This is just a refactor, which shouldn't change any behaviour of the code but prepares it for stateless validation.
We came up with idea that logic contained in
get_apply_chunk_job_*method should not depend on Chain that heavily. Here I extract all logic where Chain is required toget_apply_chunk_jobonly. All remaining logic is moved toupdate_shard.rs. For better readability, I introduce more updates:ShardUpdateReasonenum we only update validator accounts or process split state info.BlockContextandShardUpdateReasoncontents which are specific for all shard updates. Now it should be much more clear which data is necessary. Note that receipts are passed only when new chunk appears.chain.rs size is reduced by 250 lines which is good I think.
In the follow-up PRs you can see more refactoring (but less invasive) and introducing of "shadow" jobs for stateless validation on nightly, which will in fact call
apply_old_chunkN times and thenapply_new_chunk1 time. It was impossible to do using previous API.Nayduck https://nayduck.near.org/#/run/3276