fix: Genesis(SignedBlock)

[sergerad]

Closes #1844.

[bobbinth]

Looks good! Thank you!

One thing to think through: what are the implications of this from an external user standpoint? I think there may be a couple:

• The GetBlockByNumber endpoint would return a SignedBlock for the genesis block and ProvenBlock for everything else, right? If so, we probably should make the MaybeBlock message more structured to contain this information. If that's not the case (e.g., if we always get the SignedBlock from this endpoint), we should document this explicitly and maybe add a separate endpoint for retrieving block proofs (in a different PR). Or maybe we can change this endpoint to return both the SignedBlock and the BlockProof?
• Does this impact the GetBlockHeaderByNumber in any way? I don't think it should, but let's double-check.
• Nothing else comes to mind, but maybe there is some other implication?

[sergerad]

• The GetBlockByNumber endpoint would return a SignedBlock for the genesis block and ProvenBlock for everything else, right?

SignedBlock is used so there is no inconsistency:

        let signed_block_bytes = signed_block.to_bytes();
        let store = Arc::clone(&self.block_store);
        let block_save_task = tokio::spawn(
            async move { store.save_block(block_num, &signed_block_bytes).await }.in_current_span(),
        );

maybe add a separate endpoint for retrieving block proofs (in a different PR). Or maybe we can change this endpoint to return both the SignedBlock and the BlockProof?

Would have to be Option<BlockProof> for this endpoint but I think that would make sense. Let me know if you prefer that or a separate endpoint please.

• Does this impact the GetBlockHeaderByNumber in any way? I don't think it should, but let's double-check.

I don't think there is an impact to consider here (boils down to block_headers table).

• Nothing else comes to mind, but maybe there is some other implication?

Nothing comes to mind. It would have to be a code path involving deserialization of blocks from bytes for it to not have been caught by the compiler during the type change. And I think the only relevant path in that regard is the raw block data which you raised.

[bobbinth]

SignedBlock is used so there is no inconsistency:

Ah - nice! We should then update the doc comments for MaybeBlock message (I think they are currently stale).

Would have to be Option<BlockProof> for this endpoint but I think that would make sense. Let me know if you prefer that or a separate endpoint please.

Yeah - I think adding this to MaybeBlock message makes sense. I'd do it in a separate PR though.

[SantiagoPittella]

Looks good, maybe in some other PR but the TODO in crates/store/src/server/block_producer.rs:105 can be addressed now.

                let signed_block = SignedBlock::new_unchecked(header.clone(), body, signature); // TODO(sergerad): Use `SignedBlock::new()` when available.

[Mirko-von-Leipzig]

Or maybe we can change this endpoint to return both the SignedBlock and the BlockProof?

It makes sense to me to have a separate GetBlockProof endpoint - though that's really mostly to avoid the bool: with_proof type optional dancing that one does. And then we can easily just document that the genesis block will never have a proof and will return an error if requested.