feat(rpc): expose protocol config over the gRPC APIs - #2596
Conversation
Mirko-von-Leipzig
left a comment
There was a problem hiding this comment.
Only a partial review
3a0711e to
e532549
Compare
1eb94a1 to
d19628c
Compare
d19628c to
ddab236
Compare
ddab236 to
4bdd1b5
Compare
| let (start, _) = view | ||
| .get_block_header(Some(current_client_block_height), false) | ||
| .await | ||
| .map_err(super::get_block_header_error_to_status)?; | ||
| let start = | ||
| start.ok_or_else(|| Status::internal("starting block header is missing"))?; | ||
| start.protocol_config_commitment() != block_header.protocol_config_commitment() |
There was a problem hiding this comment.
We could optimise this if we also store the block at which a protocol config changes with the protocol config data?
There was a problem hiding this comment.
How would that help us? I mean, that'd still mean that you'd need to make a DB query that checks is there were changes in the (start, tip) range?
There was a problem hiding this comment.
Yes but you wouldn't have to fetch and decode the entire header and calculate the commitment.
There was a problem hiding this comment.
Oh, it's not actually calculating anything (the protocol config commitment is a field in the header). You have to decode that though, but that should be fairly cheap?
There was a problem hiding this comment.
I think this just seemed easier to do just this range search:
super::load_protocol_config(&view, current_client_block_height..=block_header.range).awaitThere was a problem hiding this comment.
I suggested adding a block number tracking here as well; but that means changing PK to composite (or to block number).
There was a problem hiding this comment.
If if not strictly required, I think it would be useful to have the block number in this table - would allow us to easily inspect all protocol configurations as they evolve over time. I'm assuming that adding this field is very easy - if not, then we can come back to it later.
| let commitment = header.protocol_config_commitment(); | ||
| if let Some(config) = protocol_config.as_ref() { | ||
| let calculated = config.to_commitment(); | ||
| if calculated != commitment { | ||
| return Err(crate::errors::DatabaseError::ProtocolConfigCommitmentMismatch { | ||
| expected: commitment, | ||
| calculated, | ||
| } | ||
| .into()); | ||
| } | ||
| } | ||
| let stored = self.db.select_protocol_config_by_commitment(commitment).await?; | ||
| if stored.is_none() && protocol_config.is_none() { | ||
| return Err(crate::errors::DatabaseError::ProtocolConfigNotFound(commitment).into()); | ||
| } | ||
| let new_protocol_config = if stored.is_none() { protocol_config } else { None }; |
There was a problem hiding this comment.
Took me a while to understand why this looked more complicated, but I think its because we avoid re-storing a config if we already have it. e.g. A, B then A again, the last A doesn't get stored.
I don't think that's really worth optimising for, we can just insert or ignore, or if we go with my suggestion to also add the block number, then we have to insert regardless.
There was a problem hiding this comment.
I'd like to avoid adding the block number unless there's a really good reason to do so, so we should maybe just do an "insert or ignore" here?
There was a problem hiding this comment.
One downside is that if we just "insert or ignore" then we don't actually ensure that the commitment in the block header that's being applied is present (is either already in the DB or being added just now).
3851287 to
fc34817
Compare
b302641 to
fc34817
Compare
fc34817 to
fe71f00
Compare
fe71f00 to
ea9a8e9
Compare
ea9a8e9 to
d42a4d5
Compare
d42a4d5 to
c8806dd
Compare
Co-authored-by: Mirko <48352201+Mirko-von-Leipzig@users.noreply.github.com>
Co-authored-by: Mirko <48352201+Mirko-von-Leipzig@users.noreply.github.com>
Co-authored-by: Mirko <48352201+Mirko-von-Leipzig@users.noreply.github.com>
Co-authored-by: Mirko <48352201+Mirko-von-Leipzig@users.noreply.github.com>
Co-authored-by: Mirko <48352201+Mirko-von-Leipzig@users.noreply.github.com>
c8806dd to
090ceb0
Compare
…the commitment changed
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! This is not an exhaustive review - I largely skipped big parts of the NTX builder and the monitor changes, and focused primarily on the non-test code. I left some comments inline - the main one is about using BlockProofRequest message for validators - but maybe I just misunderstood something there.
| pub async fn sign_block( | ||
| &self, | ||
| proposed_block: &ProposedBlock, | ||
| block_inputs: &BlockInputs, | ||
| protocol_config: &ProtocolConfig, | ||
| ) -> Result<Vec<SignBlockResponse>, ValidatorError> { | ||
| let message = proto::block_proving::BlockProofRequest { | ||
| protocol_config: Some(protocol_config.into()), | ||
| batches: proposed_block.batches().as_slice().iter().map(Into::into).collect(), | ||
| block_inputs: Some(block_inputs.into()), | ||
| timestamp: proposed_block.timestamp(), | ||
| next_validator_config: Some(proposed_block.next_validator_config().into()), | ||
| next_protocol_config: proposed_block.next_protocol_config().map(Into::into), | ||
| }; |
There was a problem hiding this comment.
Not from this PR, but why are we sending BlockProofRequest to the validators? Is it just for convenience because BlockProofRequest message and the message we want to send to the validator have the same "shape"? If so, I'd consider still using two different messages because maybe they'll evolve in different was in the future (could be done in a different PR).
| // The active configuration for signing. May be omitted if the validator already stores it. | ||
| // Proving-only callers may omit this field. | ||
| optional protocol_config.ProtocolConfig protocol_config = 6; |
There was a problem hiding this comment.
Probably related to the above comment: I didn't fully understand the purpose of this field - is this something like:
- When
BlockProofRequestis sent to a prover, we don't need this field. - When
BlockProofRequestis sent to a validator and the validator already has the config, we don't need it. - When
BlockProofRequestis sent to a validator, and if the validator doesn't already have this config, we do need it.
I guess the question is: how do we know if the validator has or doesn't have the config?
| /// Supply the active configuration if its commitment is not yet stored. The configuration | ||
| /// must match the block header. New configurations are committed with the block. |
There was a problem hiding this comment.
This will also error-out if protocol_config is not provided and we can't find the config corresponding to the signed_block in the database, right? If so, I'd mention it.
Summary
GetBlockHeaderByNumber,SyncChainMmr, and public/validator block subscriptions.--fee-faucet-idoptions.Changelog
Stack created with GitHub Stacks CLI • Give Feedback 💬