feat: include genesis block's commitment in Status#1181
feat: include genesis block's commitment in Status#1181bobbinth merged 7 commits into0xMiden:nextfrom
Conversation
igamigo
left a comment
igamigo
left a comment
There was a problem hiding this comment.
Looks good in terms of code, but I think we should look into caching the genesis commitment. Generally, this commitment will be decided at the start of the chain and would never change, so going to the store every time seems unnecessary.
When the Rpc component starts serving (ie, on Rpc::serve), we are already retrieving the genesis commitment to setup the accept header tonic layer, so maybe we could take the chance and somehow set this metadata for the status endpoint to use later.
Would it be acceptable to save it in a new field in |
| pub fn set_genesis_commitment(&self, commitment: Word) -> anyhow::Result<()> { | ||
| self.genesis_commitment | ||
| .set(commitment) | ||
| .map_err(|_| anyhow::anyhow!("Genesis commitment already set"))?; | ||
| Ok(()) |
There was a problem hiding this comment.
Should this be set on the struct initialization? cc @igamigo
There was a problem hiding this comment.
I think ideally you would set this on construction, but I'm not sure this is doable right now without changing the semantics a bit. Basically you would need to change new to be async and also it would not return until the store was up and running (so that get_genesis_header_with_retry returns).
The current way is not as clean and adds some (very manageable) complexity, but can't think of a better way without refactoring other sections too much.
igamigo
left a comment
igamigo
left a comment
There was a problem hiding this comment.
LGTM. As mentioned in the comments, not sure if there is a better way to set this up without refactoring some other pieces of code, but overall the added complexity seems low enough that this is a good tradeoff (and we could add a TODO or an issue to revisit this). Would be nice to see if @sergerad has any opinions on this as well.
| pub fn set_genesis_commitment(&self, commitment: Word) -> anyhow::Result<()> { | ||
| self.genesis_commitment | ||
| .set(commitment) | ||
| .map_err(|_| anyhow::anyhow!("Genesis commitment already set"))?; |
There was a problem hiding this comment.
| .map_err(|_| anyhow::anyhow!("Genesis commitment already set"))?; | |
| .map_err(|_| anyhow::anyhow!("genesis commitment already set"))?; |
|
|
||
| let genesis_commitment = self.genesis_commitment.get().ok_or({ | ||
| tracing::warn!(target: COMPONENT, "Failed to fetch genesis header"); | ||
| Status::internal("failed to fetch genesis header") |
There was a problem hiding this comment.
| Status::internal("failed to fetch genesis header") | |
| Status::failed_precondition("genesis header was not set") |
| pub fn set_genesis_commitment(&self, commitment: Word) -> anyhow::Result<()> { | ||
| self.genesis_commitment | ||
| .set(commitment) | ||
| .map_err(|_| anyhow::anyhow!("Genesis commitment already set"))?; | ||
| Ok(()) |
There was a problem hiding this comment.
I think ideally you would set this on construction, but I'm not sure this is doable right now without changing the semantics a bit. Basically you would need to change new to be async and also it would not return until the store was up and running (so that get_genesis_header_with_retry returns).
The current way is not as clean and adds some (very manageable) complexity, but can't think of a better way without refactoring other sections too much.
| } | ||
| } | ||
|
|
||
| pub fn set_genesis_commitment(&self, commitment: Word) -> anyhow::Result<()> { |
There was a problem hiding this comment.
Let's add some doc comments to mention what this function does and why it's needed.
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a couple of small comments/questions inline.
varun-doshi
force-pushed
the
varun/status
branch
from
aa0af0e to
fb028a5
Compare
Mirko-von-Leipzig
left a comment
Mirko-von-Leipzig
left a comment
There was a problem hiding this comment.
Its a bit hacky but works for now.
I think we take this as is, and then refactor it later such that the store and block-producer clients are created in the serve function. This can be trivially done once we have a general way to auto-retry tonic requests e.g. store.get_genesis_header().retry_on_transport_error().await.
| pub struct RpcService { | ||
| store: StoreRpcClient, | ||
| block_producer: Option<BlockProducerClient>, | ||
| genesis_commitment: Word, |
There was a problem hiding this comment.
I made a PR to your branch to move this to option also
genesis commitment option
6 participants
Fixes #1088