feat: make Consensus trait generic over block parts #12451
Conversation
| @@ -44,11 +44,11 @@ impl<'a> PostExecutionInput<'a> { | |||
|
|
|||
| /// Consensus is a protocol that chooses canonical chain. | |||
| #[auto_impl::auto_impl(&, Arc)] | |||
| pub trait Consensus: Debug + Send + Sync { | |||
| pub trait Consensus<H = Header, B = BlockBody>: Debug + Send + Sync { | |||
There was a problem hiding this comment.
| pub trait Consensus<H = Header, B = BlockBody>: Debug + Send + Sync { | |
| pub trait Consensus<N: NodePrimitives<BlockHeader = Header, BlockBody = BlockBody>>: Debug + Send + Sync { |
prefer this, and adding header and block body as ATs parallel to block in NodePrimitives. it's nice to grow the generics list by only a single element everywhere via this task of generalising the data primitives.
There was a problem hiding this comment.
imo ideally this has a single block AT bounded by
Consensus responsibility is to validate blocks so don't think it should know about entire node storage model. this would also be consistent with pool traits which only know the transaction AT
I've done this as 2 generics right now so that we can still keep dyn Consensus everywhere
does it make sense?
There was a problem hiding this comment.
why can't you keep the trait object if you add the generic?
NetworkPrimitives has header and body as ATs, and the block, so I'm not sure you're intentions are really clear?
There was a problem hiding this comment.
why can't you keep the trait object if you add the generic?
I mean that we can't keep it as is if we'd add AT because we'd need to specify Block = Block everywhere, so went with generics for now
NetworkPrimitiveshas header and body as ATs, and the block, so I'm not sure you're intentions are really clear?
sorry, not sure I follow, how are NetworkPrimitives related to consensus trait?
There was a problem hiding this comment.
yeah a single AT would be ideal, but for now we can introduce both AT just to move forward
There was a problem hiding this comment.
i
why can't you keep the trait object if you add the generic?
I mean that we can't keep it as is if we'd add AT because we'd need to specify
Block = Blockeverywhere, so went with generics for now
I see, seems to be a misunderstanding, I didn't mean adding an AT to the consensus trait
NetworkPrimitiveshas header and body as ATs, and the block, so I'm not sure your intentions are really clear?sorry, not sure I follow, how are
NetworkPrimitivesrelated to consensus trait?
if you remove the rlp encoding trait bounds from ATs on NetworkPrimitives, and just have auto trait bounds, then the associated types can easily just be the unit struct ().
by passing one type, that aggregates the data primitives as associated types, we make a framework that is extensible without breaking the public api.
in the future, and even towards the end of this sdk integration task, it may well be, that somewhere in consensus, another data primitive type is needed. then we have to change the type definition again to put another generic to the list if we do it the way you impl it now.
There was a problem hiding this comment.
by passing one type, that aggregates the data primitives as associated types, we make a framework that is extensible without breaking the public api.
in the future, and even towards the end of this sdk integration task, it may well be, that somewhere in consensus, another data primitive type is needed. then we have to change the type definition again to put another generic to the list if we do it the way you impl it now.
hmm, in scope of sdk the Consensus trait API only needs block and its components right now. If we were to add additional methods requiring something else this would anyway be a breaking change to API I think? and internally trait implementations are free to use any components
| @@ -44,11 +44,11 @@ impl<'a> PostExecutionInput<'a> { | |||
|
|
|||
| /// Consensus is a protocol that chooses canonical chain. | |||
| #[auto_impl::auto_impl(&, Arc)] | |||
| pub trait Consensus: Debug + Send + Sync { | |||
| pub trait Consensus<H = Header, B = BlockBody>: Debug + Send + Sync { | |||
There was a problem hiding this comment.
yeah a single AT would be ideal, but for now we can introduce both AT just to move forward
|
prefer if we don't create more debt for no apparent reason whatsoever @mattsse , it's a very easy change to have the one aggregate generic |
Preparation for abstraction of block for network/downloaders.
Moves
ensure_valid_body_responsefn from networking crate toConsensustrait asvalidate_body_against_header.Makes
Consensusgeneric over header and body