-
Notifications
You must be signed in to change notification settings - Fork 129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add basic equivocation detection pipeline schema #2338
Conversation
Extract basic parts of SubstrateFinalitySyncPipeline into SubstrateFinalityPipeline
d2f2f37
to
f09f758
Compare
|
||
/// Finality engine. | ||
type FinalityEngine: Engine<Self::SourceChain>; | ||
pub trait SubstrateFinalitySyncPipeline: SubstrateFinalityPipeline { | ||
/// How submit finality proof call is built? | ||
type SubmitFinalityProofCallBuilder: SubmitFinalityProofCallBuilder<Self>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why SubmitFinalityProofCallBuilder
was not moved to SubstrateFialityPipeline
?
I could see SubstrateFinalityPipeline
as a abstract "holder of data" / "configuration" and SubstrateFinalitySyncPipeline
adding some behavior (also adds [async_trait]
).
if we move SubmitFinalityProofCallBuilder
to SubstrateFinalityPipeline
,
then SubstrateFinalitySyncPipeline
could be more like GuardedSubstrateFinalityPipeline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bellow I see:
pub type SubstrateFinalityProof<P> = <<P as SubstrateFinalityPipeline>::FinalityEngine as Engine<
<P as SubstrateFinalityPipeline>::SourceChain,
>>::FinalityProof;
so the proof is related to the SubstrateFinalityPipeline
, so SubmitFinalityProofCallBuilder
should be placed maybe there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to have a SubstrateFinalityPipeline
that contains only some basic finality logic that will be shared by the sync and equivocation implementations. SubmitFinalityProofCallBuilder
is specific to the finality sync and not needed for equivocation and that's why I kept it in SubstrateFinalitySyncPipeline
.
I wasn't sure about the relay guards logic. It only validates the target chain runtime version which seems related to finality sync. Because for finality sync we submit transactions to the target. But for equivocation we'll submit transactions to the source. We can have 2 methods, one for adding target relay guards, and one for source. Or we can keep it like this and maybe add a different function for adding source relay guards to the equivocation detection pipeline. I'm not sure. But we can do this in the future when we have more context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a vacuum the changes in this PR aren't incorrect, but I'm curious to see where this is going, aka how these primitives will be actually used.
) -> CallOf<P::SourceChain>; | ||
} | ||
|
||
/// Building the `report_equivocation` call when having direct access to the target chain runtime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most likely this will not live on a relay chain node, so direct access will not be available.
Instead this will have to be done through a signed extrinsic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. We'll also have to add an indirect version of this later. But we'll need to either create mocks or generate the runtimes schema from metadata. So for the moment I added only this direct version to be able to use it on millau and rialto hopefully. And then we'll add what's missing.
* Move finality Engine to finality_base folder * Define SubstrateFinalityPipeline Extract basic parts of SubstrateFinalitySyncPipeline into SubstrateFinalityPipeline * Add equivocation detection pipeline * Fix comment
This pull request has been mentioned on Polkadot Forum. There might be relevant details there: https://forum.polkadot.network/t/polkadot-kusama-bridge/2971/5 |
…aritytech#2341) * Move finality Engine to finality_base folder * Define SubstrateFinalityPipeline Extract basic parts of SubstrateFinalitySyncPipeline into SubstrateFinalityPipeline * Add equivocation detection pipeline * Fix comment
…aritytech#2341) * Move finality Engine to finality_base folder * Define SubstrateFinalityPipeline Extract basic parts of SubstrateFinalitySyncPipeline into SubstrateFinalityPipeline * Add equivocation detection pipeline * Fix comment
* Move finality Engine to finality_base folder * Define SubstrateFinalityPipeline Extract basic parts of SubstrateFinalitySyncPipeline into SubstrateFinalityPipeline * Add equivocation detection pipeline * Fix comment
Related to #2400
report_equivocation
call builder