Conversation
…ferred-block-proving
sergerad
requested review from
Mirko-von-Leipzig,
bobbinth and
drahnr
and removed request for
Mirko-von-Leipzig
| signature BLOB NOT NULL, | ||
| commitment BLOB NOT NULL, | ||
| proving_inputs BLOB, -- Serialized BlockProofRequest needed for deferred proving. NULL for genesis block. | ||
| block_proof BLOB, -- NULL means the block has not yet been proven |
There was a problem hiding this comment.
Maybe fine for now, but eventually, we probably shouldn't store block proofs in the database as each of them could be close to 200KB.
There was a problem hiding this comment.
Do we want to store each proof as a single file?
| block_header BLOB NOT NULL, | ||
| signature BLOB NOT NULL, | ||
| commitment BLOB NOT NULL, | ||
| proving_inputs BLOB, -- Serialized BlockProofRequest needed for deferred proving. NULL for genesis block. |
There was a problem hiding this comment.
What's the reason for storing proving inputs in the database?
There was a problem hiding this comment.
Iiuc conceptually, we retrieve all un-proven blocks from DB, and to request a proof we require the inputs. The column becomes uniform source for inputs when it comes to proofing for both in-flight and proving on-startup. It also ensure we persist the inputs/don't lose any information on crash / host reboot / etc.
If retrieving/creating a proof fails repeatedly, we might want a mitigation to get stuck in trying to proof a failing one. (I'll get back to this).
There was a problem hiding this comment.
Yes the proof scheduler needs to gather proving inputs on startup / in loop when proving committed blocks.
Do we want to factor this differently so that the proof scheduler has to construct the proving inputs itself? @bobbinth
| BlockRange block_range = 1; | ||
|
|
||
| // The finality level to use when clamping the upper bound of the block range. | ||
| // | ||
| // When set to FINALITY_COMMITTED (default), the upper bound is clamped to the chain tip. | ||
| // When set to FINALITY_PROVEN, the upper bound is clamped to the latest proven block. | ||
| Finality finality = 2; |
There was a problem hiding this comment.
block_range and finality could be set inconsistently, right? For example, if the last committed block is 100, and last proven block is 105, the request could be from = 90, to = 103, finality = committed and this could create some ambiguity (e.g., what takes precedence - block_to or finality?).
A different approach could be to make it so that we can either specify block_to or the finality value - but not both.
There was a problem hiding this comment.
if the last committed block is 100, and last proven block is 105
A proven block could never be higher than the latest committed block.
A different approach could be to make it so that we can either specify block_to or the finality value - but not both.
I think the main use case is precisely to be able to get a range of blocks with a specified finality.
I think the endpoint should either error out or provide best effort results for a range that can't be satisfied whether its a committed or proven finality request.
| 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
block_proof BLOBandproving_inputs BLOBcolumns to theblock_headerstable.insert_block_proof,select_block_proof,select_unproven_blocks(excludes genesis block 0),select_block_proving_inputs, 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.SyncChainMmr: Added aFinalityenum (COMMITTED,PROVEN) to the protobuf and afinalityfield onSyncChainMmrRequest.apply_blockquery: IntroducedApplyBlockDatastruct to replace the 7-parameter function signature.