rusk
rusk copied to clipboard
dusk-network
Refactor Agreement message handling
I think the logic behind the Agreement phase should be refactored, since we don't really need to "collect" Agreement messages anymore. We should accept a block at the end of the second Reduction, where we collect votes. If an Agreement message is received, it should be processed similarly to a Block message (since an Agreement is sufficient to accept a block). But this is better addressed in a future PR
Originally posted by @fed-franz in https://github.com/dusk-network/rusk/pull/1156#pullrequestreview-1750158192
From https://github.com/dusk-network/rusk/pull/1156#issuecomment-1827755597 :
On collecting an agreement message the Agreement loop currently does a few things:
- Check if the agreement is valid
- If so, broadcast it
- Try to fetch from LocalDB the candidate block that belongs to this agreement.
- If candidate not found, it tries to request it from the network. Waits for a response or timeout.
- Once the candidate is found/arrives, it builds the winner block and broadcasts it
On refactoring the agreement loop, we need to take these points into account.
@goshawk-3 @herr-seppia
IMO, we should handle Agreement messages with a dedicated handler, like we do for Block messages.
That is, instead of having an Agreement loop, I would have an on_event handler that implements the current logic and additionally stops the consensus loop if the Agreement is for the current candidate block (similarly to what we did when receiving an AggrAgreement).
What you're proposing is what we already have with so called Agreement Loop.
Agreement loop has collect_inbound_msg (behaves as on_event) handler that waits for an Agreement that can originate from either the network or the local Consensus. Once the agreement is collected/handled correctly, Agreement loop returns the winner block and terminates the consensus task. Actually AggrAgreement (already deprecated) and Agreement handlers are pretty similar now.
Maybe we can change naming to avoid confusion.
@fed-franz
The main change I would do, a part from change naming, would be to implement the handler not as a consensus step. The only real steps we have are Block Generation and Reductions. At the end of the second Reduction, if we have quorum on both votes, we accept the block (and broadcast an Agreement message). On the other hand, if we receive an Agreement for a known candidate block, we should accept that block (and restart consensus). If we don't know the candidate block, we ask for it to the peer who sent the Agreement message, which should send us the full block (if it broadcasted the Agreement it should have accepted the full block. When receiving the full block, we treat it like a normal Block message (triggering accept or sync).
I can see the essential logic is the same as now, but I would still refactor its code to better reflect the new design.
The main change I would do, a part from change naming, would be to implement the handler not as a consensus step.
Current implementation has an agreement_msg handler that is NOT a consensus step.
If we don't know the candidate block, we ask for it to the peer who sent the Agreement message, which should send us the full block
The peer that sends/(re)-broadcast the Agreement message, may also lack the candidate block.
Current implementation has an agreement_msg handler that is NOT a consensus step. And yet it is implemented as if it were a step (it's in
step.rs, it implementsrun, it's handled by the acceptor, etc..).
Again, I'm not saying the logic is wrong. It's just that you can clearly see it follows the previous design, where Agreement was a phase of the consensus.
What I'd like to see if a handler like the one used for Block messages (ie, an on_event handler in fsm or wherever seems appropriate).
The peer that sends/(re)-broadcast the Agreement message, may also lack the candidate block. Than we can ask all peers. Or simply store the Agreement and wait for the candidate to be received.