[VID] Vid Vote Logic in Consensus Task

[dailinsubjam]

Closes issue: #2067

This PR:

• Update vote logic by updating vote_if_able() so that replicas don’t vote until they’ve seen a VIDShare(event VIDDisperseRecv in implementation).
• Get rid of VIDVote, VIDCert, VIDVoteSend/Recv, VIDCertSend/Recv, VIDCertificate, VIDVoteCollection.
• Update test_vid_task.

This PR does not:

• Deliver different shares to different storage nodes, for this issue VIDDisperseRecv contains ALL shares.

Key places to review:

• task-impls/src/consensus.rs: added vid_shares to save received vid shares for each view, added one more condition in vote_if_albe(), dealt with HotShotEvent::VidDisperseRecv.

How to test this PR:

just async_std test_vid_task
just async_std test_success

Comments requested:

• Currently both consensus task and vid task will deal with HotShotEvent::VidDisperseRecv, and there is a high overlapping of codes in validating the received shares. We have to keep the one in consensus task to determine when can vote, but do we still want to keep the one in vid task?

[elliedavidson]

Could we update the consensus_task unit tests to also test for receiving VID shares? I don't think they test that now, and after this change we'll want them to test that.

Really great work overall! Thank you turning this PR around so fast!

[elliedavidson]

Now that we have this event handler in the consensus task, we can remove it entirely from the vid task.

[dailinsubjam]

The only thing remained is this comment at the end of original VidDisperseRecv handler:

// Record the block we have promised to make available.
// TODO https://github.com/EspressoSystems/HotShot/issues/1692
// consensus.saved_payloads.insert(proposal.data.block_payload);

Do we still want to keep #1692 as a comment in the new handler for VidDisperseRecv? cc @ggutoski

[ggutoski]

I say yes but will defer to others.

[shenkeyao]

Looks good! Just a question about the endpoint.

[dailinsubjam]

Could we update the consensus_task unit tests to also test for receiving VID shares? I don't think they test that now, and after this change we'll want them to test that.

@elliedavidson Good point! I know test_success also test this, but consensus_task doesn't. I took a look and found that test input & output are also relevant to the event TransactionsSequenced and BlockReady, I have to take some time to see how to sequence them and how they change the view number. How about we create an issue for this? We can also discuss this in the next meeting.

[elliedavidson]

Could we update the consensus_task unit tests to also test for receiving VID shares? I don't think they test that now, and after this change we'll want them to test that.

@elliedavidson Good point! I know test_success also test this, but consensus_task doesn't. I took a look and found that test input & output are also relevant to the event TransactionsSequenced and BlockReady, I have to take some time to see how to sequence them and how they change the view number. How about we create an issue for this? We can also discuss this in the next meeting.

Yes, sounds good! This can be done in a separate issue. Let's get this merged first.

[elliedavidson]

Let's try updating the view timeout for the half_f failures timeout like we did for the merge yesterday.

[dailinsubjam]

Could we update the consensus_task unit tests to also test for receiving VID shares? I don't think they test that now, and after this change we'll want them to test that.

Created the issue here: #2148