Import target block body during warp sync #12300
Conversation
dmitry-markin
added
A0-please_review
| @@ -160,6 +232,10 @@ where | |||
| phase: WarpSyncPhase::DownloadingWarpProofs, | |||
| total_bytes: self.total_proof_bytes, | |||
| }, | |||
| Phase::TargetBlock(_) => WarpSyncProgress { | |||
| phase: WarpSyncPhase::DownloadingTargetBlock, | |||
| total_bytes: self.total_proof_bytes, | |||
There was a problem hiding this comment.
Should this include an estimate/size of the target block?
There was a problem hiding this comment.
Blocks are downloaded atomically, so may be we should add the block size in bytes to Phase::State stage below when the block is already downloaded. Don't know if it makes sense, considering the block size is orders of magnitude smaller than the latest state (but this might be different in some blockchains).
| Phase::WarpProof { .. } => None, | ||
| Phase::TargetBlock(header) => { | ||
| let request = BlockRequest::<B> { | ||
| id: 0, |
There was a problem hiding this comment.
I wonder why the request ID is always zero and if that is ever going to cause issues. With a quick check the ID is only used here and it's not really used for checking anything. Maybe it can be removed since libp2p already gives unique request IDs so not sure why they're needed in the BlockRequest struct as well.
Not related to this PR since the target block is requested only once but maybe regular syncing code has an issue. Or not. I'll need to check.
There was a problem hiding this comment.
IDs were used before this protocol was moved to libp2p request-response. Now ID matching is done in libp2p and the field was left in BlockRequest and BlockResponse for compatibility I guess. We should eventually remove it.
| }, | ||
| Phase::TargetBlock(header) => | ||
| if let Some(block_header) = &block.header { | ||
| if block_header == header { |
There was a problem hiding this comment.
One thing that is very nice when writing pallet code is the ensure!() macro that allows to exit early and it makes the error handling code slightly nicer. Maybe we could introduce something like that for the networking code with the ability to adding logging message so something like this:
ensure!(
block_header == header,
TargetBlockImportResult::BadResponse,
"sync",
"Importing target block failed: different header."
);Hmm it's quite verbose if the logging is included. But that's something we could think about.
There was a problem hiding this comment.
Also not relevant to this PR but a general comment
@arkpar thanks! |
|
bot merge |
paritytech-processbot
bot
deleted the
dm-missing-block-after-warp-sync
branch
* master: [Fix] parameter_types! dead code errors (#12340) [Feature] Sequential migration execution for try-runtime (#12319) bench: Use `_` instead of `::` in auto-generated file names (#12332) Fast Unstake Pallet (#12129) Rename anonymous to pure proxy (#12283) Migrate remaining old decl_* macros to the new pallet attribute macros (#12271) pallet-utility: Disallow none origin (#12321) Make automatic storage deposits resistant against changing deposit prices (#12083) Format templates and fix `--steps` default value (#12286) Bump `wasmtime` to 1.0.0 (#12317) Introduce 'intermediate_insert' method to hide implementation details (#12215) Bound staking storage items (#12230) Use `array-bytes` for All Array/Bytes/Hex Operations (#12190) BREAKING: Rename Origin (#12258) Use temporary db for benchmarking (#12254) rpc: Implement `chainSpec` RPC API (#12261) Import target block body during warp sync (#12300) Proper naming wrt expectations (#12311) [ci] Revert cancel-pipeline job (#12309)
* Receive and import target block body * Request target block * minor: wording * Check for block body in the test * Import target block justifications * Fix: do not fail block validation if no justifications received * Fix: import target blocks without justifications Co-authored-by: arkpar <[email protected]>
Resolves #12255 implementing the first approach suggested.