Implement OutOfRangeTx variant of Invalid bundle fraud proof #2039
Conversation
ParthDesai
requested review from
vedhavyas,
NingLin-P,
nazar-pc and
rg3l3dr
as code owners
|
I interrupted CI, Windows job was running for 4 hours 51 minutes, looks like it got stuck there. UPD: Restarted, looks like issues with networking tests: #2045 |
crates/sp-domains/src/fraud_proof.rs
Outdated
| pub consensus_block_hash: Hash, | ||
| pub parent_domain_block_hash: DomainHash, |
There was a problem hiding this comment.
Not sure if we need these 2 fields since bad_receipt already contains them.
| MissingInvalidBundleEntry( | ||
| MissingInvalidBundleEntryFraudProof<Number, Hash, DomainNumber, DomainHash>, | ||
| ), | ||
| ValidAsInvalid(ValidAsInvalidBundleEntryFraudProof), |
There was a problem hiding this comment.
The word "missing" implies that invalid bundle is missing from somewhere, but we don't have the context about "somewhere" here, would be great to avoid using it. FYI #1995 uses TrueInvalid and FalseInvalid to replace these 2 items.
| RuntimeApiLight::new(self.executor.clone(), domain_runtime_code.into()); | ||
|
|
||
| match additional_data { | ||
| MissingBundleAdditionalData::OutOfRangeTx { extrinsic_index } => { |
There was a problem hiding this comment.
Would be great to move the following to a dedicated function, non-blocker.
| Block, | ||
| ParentChainBlock, | ||
| >(&*self.client, consensus_block_hash)? | ||
| let local_receipt = crate::aux_schema::load_execution_receipt::<_, Block, CBlock>( |
There was a problem hiding this comment.
Why change ParentChainBlock to CBlock? although it is also correct.
| let tx_range = self | ||
| .consensus_client | ||
| .runtime_api() | ||
| .domain_tx_range(*consensus_block_hash, *domain_id)?; |
There was a problem hiding this comment.
Seems we are using a different consensus_block_hash for this runtime API:
- In the block-preprocessor, we are using the consensus block that derives the under constructing domain block
- In the domain-bundle-proposer, we are using the best consensus block hash
- Here, we are using the consensus block hash of the ER
Although it is okay since dynamic tx-range will be deprecated, plz add some comments to explain why it is okay and won't lead to non-deterministic results and TODO such that when dynamic tx-range is deprecated we should use a constant tx-range value.
| .vrf_hash(), | ||
| ); | ||
|
|
||
| let parent_domain_block_hash = { |
There was a problem hiding this comment.
is_within_tx_range is a stateless api so it is okay to use the default value of domain_block_hash cc @vedhavyas
| pub opaque_bundle_with_proof: | ||
| OpaqueBundleWithProof<Number, Hash, DomainNumber, DomainHash, Balance>, |
There was a problem hiding this comment.
With #1995 we don't need to include the whole bundle in the fraud proof, plz take a look at the PR description.
crates/sp-domains/src/fraud_proof.rs
Outdated
| sp_api::decl_runtime_apis! { | ||
| /// API related to execution receipts stored in runtime | ||
| pub trait ExecutionReceiptApi<DomainNumber: Encode + Decode, DomainHash: Encode + Decode> { | ||
| fn get_execution_receipt_by_hash(hash: ReceiptHash) -> Option<ExecutionReceipt<NumberFor<Block>, Block::Hash, DomainNumber, DomainHash, Balance>>; | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this runtime api can fit into DomainsApi and perhaps just get_execution_receipt or execution_receipt cc @vedhavyas
| if maybe_target_invalid_bundle.is_some() { | ||
| let target_invalid_bundle = maybe_target_invalid_bundle | ||
| .expect("already checked for None in if condition above; qed"); |
There was a problem hiding this comment.
if let Some(target_invalid_bundle) = maybe_target_invalid_bundle {
...
}
crates/sp-domains/src/fraud_proof.rs
Outdated
| consensus_block_incl_er, | ||
| consensus_block_incl_bundle, |
There was a problem hiding this comment.
The fraud proof is targetting an invalid ER, consensus_block_incl_bundle should be ER::consensus_block_hash and why do we need consensus_block_incl_er?
There was a problem hiding this comment.
I think it is okay to use the best consensus hash to get the bad receipt, because the fraud proof is included in the next consensus block that builds on top of the best consensus block so the fraud proof should be verified against the best consensus block, if the bad receipt don't exist here which means the bad receipt is already pruned or is in other forks, in both cases the fraud proof should be considered as invalid.
|
Closing in favor of #2136 |
Description
This PR implements OutOfRangeTx fraud proof as one variant of Invalid bundle fraud proof. It introduces generation and verification of it.
Commits are organized as follows:
1st commit introduces a rough version of the generation and verification in which we do not take into account the bad receipt.
2nd commit adds runtime api to get the execution receipt from the hash (this is used to get the bad receipt and its parent receipt in the verification).
3rd commit utilises the runtime api introduced in 2nd commit to query the bad receipt and its parent for subsequent verification.
4th commit integrates the verifier in the subspace service.
5th commit remove todo that was in place for returning bad receipt hash.
6th commit adds different consensus block hash containing ER and bundle.
7th commit adds test for the generation and verification + some changes in test runtime.
8th commit merges main into this branch.
Code contributor checklist: