Flatten memberships into a single type #3867
Conversation
tbro
force-pushed
the
tb/pos/flatten-members
branch
4 times, most recently
from
0d9e849
to
859aa47
Compare
tbro
force-pushed
the
tb/pos/flatten-members
branch
from
859aa47
to
c67fb3a
Compare
tbro
force-pushed
the
tb/pos/flatten-members
branch
2 times, most recently
from
dbd49cf
to
d4815f9
Compare
tbro
force-pushed
the
tb/pos/flatten-members
branch
from
d4815f9
to
572e6a2
Compare
Delete Memberships and replace functionality. Add some methods to `Membership` trait to deal w/ collapsing into one type both kinds of memberships (stake and DA). * avoid passing membership into `is_valid_cert * for DA, avoid proxying threshold through `Threshold` trait * remove `Topic` param from `Membership::new * Split cert impls by marker (#3891) * add membership methods to Cert trait * remove non-existent tests from justfile
tbro
force-pushed
the
tb/pos/flatten-members
branch
from
0e1c8e9
to
3483bfd
Compare
We can keep the old name where we only have one membership type to keep the diff smaller.
| @@ -124,8 +125,8 @@ impl<TYPES: NodeType, V: Versions> ProposalDependencyHandle<TYPES, V> { | |||
| if let HotShotEvent::HighQcRecv(qc, _sender) = event.as_ref() { | |||
| if qc | |||
| .is_valid_cert( | |||
| self.quorum_membership.as_ref(), | |||
| TYPES::Epoch::new(0), | |||
| self.quorum_membership.stake_table(TYPES::Epoch::new(0)), | |||
There was a problem hiding this comment.
we can use epoch 0 for now (since QC doesn't have an epoch number yet), but we should make an issue to come back and take the epoch number from the QC after #3893
can you add an issue to the board for this line?
| >( | ||
| data: DATAType, | ||
| membership: &TYPES::Membership, | ||
| da_membership: &TYPES::Membership, |
There was a problem hiding this comment.
I think this should take the stake table + threshold directly (and therefore build_assembled_sig should too)
this isn't used in the integration tests like libp2p::libp2p_network though, so shouldn't have anything to do with the failure
|
|
||
| let da_data = DaData { | ||
| payload_commit: da_payload_commitment, | ||
| }; | ||
|
|
||
| build_cert::<TYPES, V, DaData, DaVote<TYPES>, DaCertificate<TYPES>>( | ||
| da_data, | ||
| da_membership, | ||
| membership, |
There was a problem hiding this comment.
this should pass the da stake table + threshold directly
There was a problem hiding this comment.
Will we always just pass in DA stake table and success_threshold? If so it isn't clear to me why we need a generic CERT.
| &self, | ||
| stake_table: Vec<<TYPES::SignatureKey as SignatureKey>::StakeTableEntry>, |
There was a problem hiding this comment.
not sure if this is immediately fixable (might require epoch numbers in the certs, which is being worked on separately), but I think is_valid_cert should actually just take the Memberships directly, rather than the stake table + threshold
we always know which stake table/threshold to use for each cert
can you make an issue for this and add it to the board
There was a problem hiding this comment.
Before this PR that was the logic. We changed it b/c is_valid_cert can't be generic of Membership b/c we have to call specific functions such stake_table or da_stake_table. This PR addresses this by passing in the stake table.
Always use quorum for leader selection
Needed for #3726
This PR:
Removes
Membershipsstruct and updatesMembershiptrait to support DA and Quorum w/ a single interface. Essentially, instead of havingqourom_membershipandcommittee_membership(DA) fields in most places we now have a singlemembershipfield. In some places, where only one of the membership types was previously used, the previous name is preserved. To obtain the correct data, we now have to call the correct method:membership.leader()for quorummembership.da_leader()for da.This PR does not:
Shouldn't change behavior. It is only preparation for EspressoSystems/espresso-sequencer#2237
Key places to review:
The trait is used extensively and there are changes across many files.
crates/types/src/traits/election.rsSimpleCertimpls incrates/types/src/simple_certificate.rs