Conversation
…ferred-block-proving
sergerad
requested review from
Mirko-von-Leipzig,
bobbinth and
drahnr
and removed request for
Mirko-von-Leipzig
| store_client: &StoreClient, | ||
| data_directory: DataDirectory, | ||
| accounts_filepath: PathBuf, | ||
| signer: &EcdsaSecretKey, |
There was a problem hiding this comment.
is this always an Ecdsa key type?
There was a problem hiding this comment.
Yes. As opposed to what?
| let block_nums: Vec<i64> = | ||
| SelectDsl::select(schema::block_headers::table, schema::block_headers::block_num) | ||
| .filter(schema::block_headers::block_proof.is_null()) | ||
| .filter(schema::block_headers::block_num.gt(0i64)) |
There was a problem hiding this comment.
Q; are there proven blocks at genesis?
There was a problem hiding this comment.
Genesis block is never proven.
| // ================================================================================================ | ||
|
|
||
| /// Overall timeout for proving a single block. | ||
| const BLOCK_PROVE_TIMEOUT: Duration = Duration::from_mins(2); |
There was a problem hiding this comment.
We should probably make this a bit more relaxed, particularly for CI runs 🫠
There was a problem hiding this comment.
Bumped to 4. Leaving comment here for others to weigh in
| continue; | ||
| }, | ||
| }; | ||
|
|
There was a problem hiding this comment.
I think this is racy and we need atomicity in general. The current way of calling works. I'd appreciate a comment on why this works (we have the outer Mutex<()> to protect both)
There was a problem hiding this comment.
I have updated the logic to construct the notify before the db query. See the comment above that line. LMK if my understanding is incorrect.
| }; | ||
| futures.push_back(fut); | ||
| } | ||
| futures |
There was a problem hiding this comment.
FuturesOrdered doesn't have a concurrency execution limit, it only guarantees the results are in the input order (FIFO). So https://docs.rs/futures/latest/futures/stream/trait.StreamExt.html#method.buffered might a strictly better choice, to limit the optimistic proving.
There was a problem hiding this comment.
I went with adding a batch limit for the db query instead
drahnr
left a comment
drahnr
left a comment
There was a problem hiding this comment.
Generally like the approach/LGTM, a few edges to be smoothed
…ferred-block-proving
3 participants
Context
We are adding deferred (asynchronous) block proving for the node, as described #1592. Currently, block proving happens synchronously during
apply_block, which means block commitment is blocked until the proof is generated.Blocks will now exhibit committed (not yet proven) and proven states. A committed block is already part of the canonical chain and fully usable. Clients that require proof-level finality can opt into it via the new
finalityparameter onSyncChainMmr.Changes
proving_inputs BLOBandis_proven BOOLEANcolumns to theblock_headerstable, with partial indexes for querying unproven (is_proven = 0) and proven (is_proven = 1) blocks.BlockStore(following the existing block file pattern) rather than as BLOBs in SQLite. The DB tracks only anis_provenflag.mark_block_proven,select_unproven_blocks(excludes genesis block 0),select_block_proving_inputs(returns deserializedBlockProofRequest), andselect_latest_proven_block_num.apply_block: TheBlockProofRequestis now serialized and persisted alongside the block duringapply_block.proof_scheduler.rs) that drives deferred proving. It queries unproven blocks on startup (restart recovery), listens for new block commits viaNotify, and proves blocks concurrently usingFuturesOrderedfor FIFO completion ordering. Proofs are saved to files, then the block is marked proven in the DB.SyncChainMmr: Added aFinalityenum (COMMITTED,PROVEN) to the protobuf and afinalityfield onSyncChainMmrRequest.apply_blockquery: IntroducedApplyBlockDatastruct to replace the 7-parameter function signature.