Move proof generation to the type system level #8185
Conversation
bkchr
added
A0-please_review
| @@ -91,7 +117,7 @@ impl<A, B, C> ProposerFactory<A, B, C> { | |||
| } | |||
| } | |||
|
|
|||
| impl<B, Block, C, A> ProposerFactory<A, B, C> | |||
| impl<B, Block, C, A, RP> ProposerFactory<A, B, C, RP> | |||
There was a problem hiding this comment.
nitpick I guess this is named RP from RecordProof but probably PR would be better (ProofRecording).
There was a problem hiding this comment.
Ha! Before I renamed the trait over the lifetime of this pr :D
Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>
| /// required. The only two implementations of this trait are [`DisableProofRecording`] and | ||
| /// [`EnableProofRecording`]. | ||
| /// | ||
| /// This trait is sealed and can not be implemented outside of this crate! |
There was a problem hiding this comment.
Would allowing external implementation be an issue?
There was a problem hiding this comment.
For now I don't see any reason to allow this to change from the outside. Maybe in the future when there is a reason to change the proof type.
There was a problem hiding this comment.
I was a bit curious, but it is harmless anyway.
| @@ -170,7 +170,6 @@ impl core::Benchmark for ConstructionBenchmark { | |||
| inherent_data_providers.create_inherent_data().expect("Create inherent data failed"), | |||
| Default::default(), | |||
| std::time::Duration::from_secs(20), | |||
| RecordProof::Yes, | |||
There was a problem hiding this comment.
Does it switch the bench to not recording (shouldn't we use proposerfactory::with_proof_recording)?
There was a problem hiding this comment.
There is no reason this ever used proof recording.
There was a problem hiding this comment.
I don't know, maybe this was using proof recording on purpose to have worst case scenario bench, but I really don't know.
| @@ -611,7 +610,6 @@ mod tests { | |||
| inherent_data, | |||
| digest, | |||
| std::time::Duration::from_secs(1), | |||
| RecordProof::Yes, | |||
There was a problem hiding this comment.
Same, with_proof_recording for proposer factory?
|
bot merge |
|
Trying merge. |
ghost
merged commit * Companion for Substrate #8185 paritytech/substrate#8185 * "Update Substrate" Co-authored-by: parity-processbot <>
* Start * Finish!!!! * Update client/basic-authorship/src/basic_authorship.rs Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> * Review comments Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>
* Companion for Substrate #8185 paritytech/substrate#8185 * "Update Substrate" Co-authored-by: parity-processbot <>
* Start * Finish!!!! * Update client/basic-authorship/src/basic_authorship.rs Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> * Review comments Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>
This pr changes the way proof generation can be requested when building a block with
Proposer. Instead of passingRecordProof::Yesto every call ofpropose, this pr changes it to either always record the proof or never for a specific implementation ofProposer. Up to now there is was no middle way of "now we need a proof" and "now we don't need a proof". It was always either one of this.The main advantage of moving this to the type system level is that we can write code
Proposer<B, ProofRecording = EnableProofRecording>. Meaning we will only accept proposer implementations that will generate a proof. This is quite useful for Cumulus and for people who will be using Cumulus, to make it complain at compile time that the proposer is not correct.The basic authorship
ProposerFactoryandProposerare now written in a way to make them usable for proof recording or for no proof recording.polkadot companion: paritytech/polkadot#2507