Add VidDisperseShare data type
#12
Conversation
src/consensus.rs
Outdated
| @@ -41,7 +42,7 @@ pub struct Consensus<TYPES: NodeType> { | |||
| /// In the future we will need a different struct similar to VidDisperse except | |||
| /// it stores only one share. | |||
| /// TODO <https://github.com/EspressoSystems/HotShot/issues/1732> | |||
| pub vid_shares: BTreeMap<TYPES::Time, Proposal<TYPES, VidDisperse<TYPES>>>, | |||
| pub vid_shares: BTreeMap<TYPES::Time, Proposal<TYPES, VidDisperseShare<TYPES>>>, | |||
There was a problem hiding this comment.
I think we want to keep this as the full disperse. The libp2p request response protocol dictates that the leader and DA committee should store all of the shares so they can send one share to anyone who asks.
There was a problem hiding this comment.
Aah, yes. True. But it is a bit tricky because I don't think we can recreate Proposal<VidDisperse> out of many received Proposal<VidDisperseShare>. I think so because Proposal<VidDisperse> cannot be signed at this point.
I came up with the following to solve the problem and keep the full disperse:
pub vid_shares: BTreeMap<TYPES::Time, HashMap<TYPES::SignatureKey, Proposal<TYPES, VidDisperseShare<TYPES>>>>
This way all the received Proposal<VidDisperseShare> are preserved and can be easily extracted by view and recipient key.
I hope this is acceptable.
There was a problem hiding this comment.
hmm we probably need to resolve this issue some other way. For now this is totally fine. I'm pretty sure the signature is over the vid_commitment only so it should be valid for any Share or Disperse but we can refine after your solution goes unblocks the hotshot changes.
pub vid_shares: BTreeMap<TYPES::Time, HashMap<TYPES::SignatureKey, Proposal<TYPES, VidDisperseShare<TYPES>>>>, so let's just go with this
There was a problem hiding this comment.
Ok, so it's fine to take the signature from one of the Proposal<VidDisperseShare> and put it in Proposal<VidDisperse>, is that correct?
As a separate note: there must be something off with my changes because the tests are failing. I'm now fighting with test_success to check what is the reason.
There was a problem hiding this comment.
I believe it should be ok so long as the same VID commitment is put in both Proposals
There was a problem hiding this comment.
Let me know if you need help debugging the tests
src/event.rs
Outdated
| @@ -31,7 +33,7 @@ pub struct LeafInfo<TYPES: NodeType> { | |||
| /// Optional application-specific state delta. | |||
| pub delta: Option<Arc<<<TYPES as NodeType>::ValidatedState as ValidatedState<TYPES>>::Delta>>, | |||
| /// Optional VID disperse data. | |||
| pub vid: Option<VidDisperse<TYPES>>, | |||
| pub vid: Option<VidDisperseShare<TYPES>>, | |||
There was a problem hiding this comment.
This is a tricky one, I think this changes seems more correct, but maybe the sequencer would want more than one share, if we had it. We should check what they do with the VidDisperse now. I think they just get their share and this change will simplify their code
There was a problem hiding this comment.
I've looked into espresso-sequencer repo but I couldn't find the correct place to check. Could you point it to me?
For the moment I reverted it to:
pub vid: Option<VidDisperse<TYPES>>
There was a problem hiding this comment.
I'm not sure, we can just ask them though
No description provided.