Use LocalBlockProver to build blocks
#709
Conversation
PhilippGackstatter
force-pushed
the
pgackst-migrate-block-prover
branch
from
3f89443 to
1a6ad2c
Compare
| impl BlockSummaryAndInputs { | ||
| impl BlockBatchesAndInputs { | ||
| fn inject_telemetry(&self) { | ||
| let span = Span::current(); | ||
|
|
||
| // SAFETY: We do not expect to have more than u32::MAX of any count per block. | ||
| span.set_attribute( | ||
| "block.updated_accounts.count", | ||
| i64::try_from(self.summary.updated_accounts.len()) | ||
| i64::try_from(self.inputs.account_witnesses().len()) | ||
| .expect("less than u32::MAX account updates"), | ||
| ); | ||
| span.set_attribute( | ||
| "block.output_notes.count", | ||
| i64::try_from(self.summary.output_notes.iter().fold(0, |acc, x| acc.add(x.len()))) |
There was a problem hiding this comment.
Not sure what the best approach for telemetry is. We could:
- split it in three sections (see below)
- or keep it as two sections (just block batches and input + proven block, which can include the proposed block stuff)
- or as just one on the proven block, if we don't care about the number of unauthenticated notes.
// Block Batches and Input
// This info is only available here because the proposed block doesn't track what notes were erased explicitly.
"block.unauthenticated_notes.count",
// This can be here or somewhere else.
"block.updated_accounts.count",
// Proposed Block:
// Nullifiers and output notes should be emitted when note erasure was done which is here.
"block.nullifiers.count",
"block.output_notes.count",
// We could also emit the timestamp here, looks like we don't do that anywhere yet?
// Proven Block:
// BlockHeader is only available on a proven block.
"block.hash"
"block.sub_hash"
"block.parent_hash"
"block.protocol.version"
"block.commitments.kernel"
"block.commitments.nullifier"
"block.commitments.account"
"block.commitments.chain"
"block.commitments.note"
"block.commitments.transaction"
There was a problem hiding this comment.
Its something I'm also unclear on to be honest.
I think we essentially want "as much as possible" that could be of interest in determining stats/metrics post-incident.
I think notes erased would definitely be of interest. So maybe one has to carry some metadata through the various stages or allow it to be derived e.g.
block.batches.ouput_notes.count
block.output_ntoes.count
might allow deriving erased_notes.count as the difference between the two. Though this means every consumer of traces needs to know the definitions and calculations, so maybe carrying this through is better?
There was a problem hiding this comment.
In general, emitting sooner is better so that the values are there even if a later stage fails.
There was a problem hiding this comment.
Sounds good. I pushed updates now. I also remove the previous ProvenBlock (called BuiltBlock in the meantime) in favor of a local TelemetryInjector trait. That reads cleaner and we don't have to come up with new names for proposed and proven block. Main reason for using it was because I couldn't come up with a wrapper name for proposed block 😄.
Also added erased notes as an explicit metric. Renamed block.unauthenticated_notes.count to block.erased_note_proofs.count because that's more accurate, doesn't conflict with erased notes and actually just tells us how many proofs were found in the store. So really this tells us how many notes will most likely be authenticated, not how many will be erased. So we may want yet another name here.
This is a bit inconsistent now in regards to naming between the strings and the variables, but there is an issue for it, so we might figure it out eventually (0xPolygonMiden/miden-base#1144).
PhilippGackstatter
changed the title
LocalBlockProver to build blocksLocalBlockProver to build blocks
|
|
||
| let (block_header_witness, updated_accounts) = BlockWitness::new(inputs, &batches)?; | ||
| // Question: Should we split proposing and proving in two stages for telemetry reasons? |
There was a problem hiding this comment.
I'd say yes because
- We want to separately measure them in terms of time taken
- They may exhibit weird behaviour independently, e.g. one scales poorly with nullifier count
If you think proposal will always be negligible then we can keep them together as well.
| #[instrument(target = COMPONENT, skip_all)] | ||
| fn read_block_numbers(block_numbers: &[u32]) -> BTreeSet<BlockNumber> { | ||
| block_numbers.iter().map(|raw_number| BlockNumber::from(*raw_number)).collect() | ||
| } |
There was a problem hiding this comment.
Should this be standalone?
There was a problem hiding this comment.
I put it there for consistency with the other similar functions above it.
| batch | ||
| .input_notes() | ||
| .iter() | ||
| .cloned() | ||
| .filter_map(|note| note.header().map(NoteHeader::id)) |
There was a problem hiding this comment.
Is cloning a batch pretty cheap? I think we made it so that cloning transactions is cheap (because we wrap the inner ProvenTransaction in Arc), maybe worth doing something like that for batches too?
There was a problem hiding this comment.
It's not wrapped in an Arc or anything, so cloning is a deep-clone. I don't think it's particularly useful here, since we only clone an InputNoteCommitment but it seems it could be an efficiency gain in BatchGraph::select_block where we clone all batches that are selected.
It would also get more useful when the batch gets bigger with inclusion of VerifiedTransactions in it (0xPolygonMiden/miden-base#1121).
|
BTW this PR fixes the sync (we didn't get to merge it but it's basically ready). We tested integration tests against this PR and they seem to work well! |
Thank you very much! I also did a run of the integration tests and they all passed as well, so I'll merge 👍. |
Use the
LocalBlockProverfrom 0xPolygonMiden/miden-base#1152 to build blocks.Builds on this PR in miden-base: 0xPolygonMiden/miden-base#1172.
I ran the client integration tests against this branch and they all passed.
I checked that all cases of the deleted tests in the block builder are covered by tests in miden-base.