Optimized Delegated Byzantine Fault Tolerance (ODBFT) - Part I: commit phase + regeneration strategy + minnor message p2p route optimizing#426
Conversation
|
Hi @erikzhang, I saw that you self-requested review. |
| /// <summary> | ||
| /// Speaker PrepareRequest Payload | ||
| /// </summary> | ||
| public ConsensusPayload PreparePayload; |
There was a problem hiding this comment.
Why not just use PrepareRequest?
public PrepareRequest Request;There was a problem hiding this comment.
You check everything!! eahuehauea
Could be, @erikzhang, I think that we do not use the Payload itself, you are right. It is worth changing and verifying this possibility for reducing the size of this Payload.
But I would need to double check the logic for the Regeneration (as well as when we indirectly receive this Payload OnPrepareResponse), because maybe we need some verification regarding view_number, etc...
If we decide to change this I personally would prefer to merge and then we perform this optimization until we are sure about all consequences.
Sorry for the bad design =/
There was a problem hiding this comment.
You can get the view_number from the ConsensusPayload with PrepareResponse itself.
I need the code to be merged into the master branch to be well designed. If necessary, perhaps this pr will need to be split into multiple smaller prs to submit.
There was a problem hiding this comment.
Yes, you are right, view_number can be obtained.
Furthermore, the other data there can also be modified, because the signature currently does not cover that variables.
The idea is, should we change it now or wait for a next PR?
There was a problem hiding this comment.
I started a draft, @erikzhang.
It is already compiling without errors but with some problems on replicating the original payload for getting the hash: https://github.com/igormcoelho/neo/tree/erik-optimizing-consensuspayload-to-prepmessageonly
Commits:
igormcoelho@4d22638
igormcoelho@3d3c087
There was a problem hiding this comment.
Please considering that PreparePayload has more info than PrepareRequest, such as PrevHash, BlockIndex, TimeStamp. Question is that do we need to verify those info when we consider the requests are IDENTICAL?
There was a problem hiding this comment.
Hi @wanglongfei88, in principle, values should be identical because they are obtained from the PrepareRequest Payload at OnPrepareRequest.
Thus, it should be easy to reproduce those values.
But I do not know what is happening in igormcoelho#4, should be some minor detail.
There was a problem hiding this comment.
@erikzhang, I am still having problems with igormcoelho#4
It is a good idea and should work, but need some adjustments.
Do you think we should try to merge it now or it can be done after this one?
| /// <summary> | ||
| /// Prepare Request, PreparePayload, payload signature | ||
| /// </summary> | ||
| public byte[] ResponseSignature; |
There was a problem hiding this comment.
Since every ConsensusPayload has a signature, do we still need a ResponseSignature?
neo/neo/Network/P2P/Payloads/ConsensusPayload.cs
Lines 12 to 20 in 8310035
There was a problem hiding this comment.
You want to optimize everything, Erik! aheuaheuahueauheauea
You are right again, I think that we can use this and it is a nice way to save space! Nice catch.
But the logic would be a little different, because nowadays we use the ResponseSignature representing a simple way of signing the PrepareRequest. We would need to perform some minnor adjusts.
Again, I personally prefer to merge as it is (because it is quite of well tested) and then we perform this optimization.
But fell free to change anything you want, Erik. It is up to you. If you decide that it is better to do it now count with all of us for testing it.
In this new commits here https://github.com/igormcoelho/neo/tree/optimized-dbft-part-ii we updated it to All Known PrepRequest Signatures. I also prefer to keep the current template because of this possibility, we think that it is worth to send all known signatures, as we discussed in that recently opened issue.
If you also think that it is worth we can think about merging it now (the change has no big consequences and was already tested).
There was a problem hiding this comment.
You don't need to save all the ResponseSignatures. You can relay the received PrepareResponse message again when needed.
There was a problem hiding this comment.
In the current code we just relay one signature per time.
In the PART-II PR we are investigating this idea of sharing all ResponseSignatures, it is is not merged here yet. I has some advantages in some cases:
BreceivesPrepRequestfromAand sendResponseCreceivesResponse_B(indirectly getsPrepRequest_A) and sendResponseDonly getsResponse_Cand it will contain all 3 signatures.
Since the PrepRequest message is shared in the Response payload we can easily check all Backup signatures.
But this is a case to be investigated.
There was a problem hiding this comment.
The logic also applies to the FinalSignature of the header, once we receive OnCommitAgreement we can save all of them and share them when we relay our commit agreement.
It looks like a small speed up but I think it has potential benefits when the nodes becomes bigger with more nodes, it can help communication considerably.
There was a problem hiding this comment.
ResponseSignatures and PreparePayload are essential for current logic of Regeneration which makes sure other nodes (disconnected or with low network) will be able to move on to commit phase if there are other nodes are already locked in commit phase.
Without regeneration, it will be difficult to reach consensus with some nodes are locked in commit phase, and other nodes are not in commit phase which will request change view without success.
I hope my comment makes sense and be helpful.
There was a problem hiding this comment.
If a node enters the commit phase, it should lock its state and no longer change view. If only 2 nodes enter the locked state, then the other 5 nodes can change view and generate blocks. If there are 3 nodes entering the locked state, then the remaining 4 nodes cannot change view, they must reach a consensus in the current view.
There was a problem hiding this comment.
A node can only enter commit phase with M valid signatures of the PrepareReq Message, even the Primary.
There was a problem hiding this comment.
After receiving those M valid SignedPaylods, in the worst case M - f_{fault} = f_1 will be locked.
The rest f_{fault} + f_2 = 2f = M will ask for change_view and the Healthy/NoByzantine nodes f_2 will join the other f_1 by receiving their OnRegeneration Payload.
|f_{fault}| = |f_1| = |f_2| = 1/3 N
Safe limit = M = 2/3N
f_{fault} is the set of nodes that are Byzantine or Fault nodes (natural network fails).
There was a problem hiding this comment.
@erikzhang, can/should I merge igormcoelho#5?
| /// <summary> | ||
| /// Final block signature | ||
| /// </summary> | ||
| public byte[] FinalSignature; |
There was a problem hiding this comment.
the primary should not send FinalSignature at the beginning. It should be sent in the Commit phase just like backups.
There was a problem hiding this comment.
Hi @erikzhang,
In the first draft it was done as you suggested. Later we updated it in order to send the FinalSignature at the beginning, as a speed-up.
In the current NEO code (without this PR) it is done like this, Primary sends its complete signature since the beginning.
In term of logic, it makes sense because the primary is committed since the beginning (he is the one that proposed the block and it seams normal to have its signature).
With this we gain one signature that we already known.
As far as we analyzed there are no negative aspects.
There was a problem hiding this comment.
A node that has sent a FinalSignature is not allowed to change view. This implementation violates the design I proposed earlier.
There was a problem hiding this comment.
In fact, there is other way of looking...aehauheau
A node that sees the commitment of the majority (has M partial signatures`) is not allowed to change view. 💃 :D
I think it is better to send it earlier, Erik, we gain one signature (nodes can relay a block faster by knowing it in advance). The Speaker will not send it wrong because if he send it wrong everyone will know that he is Byzantine/Malicious/Fault.
But we also check it before creating the block:
neo/neo/Consensus/ConsensusContext.cs
Lines 104 to 111 in 451e2a7
There was a problem hiding this comment.
In addition, in the new design every Primary should propose it own set of transactions (we removed SignatureSent flag). Thus, it is almost impossible that someone cheat the system by storing this FinalSignature.
There was a problem hiding this comment.
The core idea is that primary is not unconditionally accept its own proposal. The criteria for his approval are the same as the backups, that is, there are more than M consensus nodes to accept the proposal. PrepareRequest and PrepareResponse are the initial judgments of the node on the proposal, and FinalSignature is the ultimate judgment of the entire consensus team on the proposal.
There was a problem hiding this comment.
I agree, @erikzhang, the description is precise.
The only point we modified was the FinalSignature_{primary}.
FinalSignature_{primary} being sent in the beginning does not mean that Primary accepted his own proposal.
Primary will just enter commit phase equally as other Backups, when M valid approvals are achieved.
There was a problem hiding this comment.
In this sense, the FinalSignature_{primary} sent in advance is not used for any decision making, it is a just a speed up (in case of lack of agreement it will be reset:
neo/neo/Consensus/ConsensusContext.cs
Lines 78 to 92 in 451198a
Primary will still lock its change view only after M SignedPayloads are known (when it really enter commit phase).
There was a problem hiding this comment.
send FinalSignature_{primary}, in advance, or not send, this is the question. ahueahueahueahueaea
There was a problem hiding this comment.
Do we have a reached a decision about this? :D
Or should we really remove the FinalSignature_{primary} from being sent in advance?
|
There are several places that need to be modified. In order to save time, I will edit directly on this PR. |
|
Please, do not destroy anything... aheuahueaaehuaheua I am kidding, Erik. Fell free to edit. You are the master on this. |
|
I tried to remove |
|
Ok, @erikzhang. Let's do it this way. |
d0889c2 to
7930fd4
Compare
* Initial commit for contribution number 1. * Update api.md Filipino Translation for api.md * Update whitepaper.md - Cordeta From Line 1 to 61 A total of 1112 words. (Source codes and variables were not counted) * Update whitepaper.md changed all "alaala to memorya" few touches * Update Whitepaper.md Minor touches : Gayun din to Gayundin * Update whitepaper.md Review Changes modified review changes * * Updated commit for contribution 1. * Update api.md Updated review * Updated files for contribution no. 1 * Update for contribution #1. * Update api.md Fixed * Update api.md Fixed #2 * Update setup.md Filipino Translation for setup.md * Update exchange.md Filipino Transalation * Update api.md Updated changes * Fixes for contribution #1. * update exchange.md * Update setup.md * Update gui.md Filipino translation. Ready for review * whitepaper.md [FULL TAGALOG TRANSLATION][FOR REVIEW] From line 63 to 113 A total of 454 translated words * Update GUI.md I fixed the errors. * Update whitepaper.md Changes based from severinolorillajr * Initial TL translations for white-paper.md * Partial TL translations for white-paper.md - 1,176 words * NEO Filipino Localization #2 (part 1) * NEO Filipino Localization #2 (part 2) * Updated TL translations for white-paper.md based on review * NEO Filipino Localization #2 (Fix based on reviews)
Last update: 21th November
Optimized Delegated Byzantine Fault Tolerance (ODBFT)
Part II, III and IV will be dealt in different Pull Requests.
FillingContextPart I: Highlights of the proposal (which is a consolidation of previous knowledge discussed in other issues and PRs)
Mpartials signatures should be obtained before sending a complete signature of thecontext.makeHeader;PrepareRequest Payload. One peculiarity is that the Speaker/Primary signs thePrepareRequest Payload_1with an empty vector of his signature (as one could expected), while theBackupssigns the completePrepareRequest Payload_Completethat contains Speaker signature of the aforementionedPrepareRequest Payload_1;PrepareRequest Payloadsince everyPrepareResponsePayload is now containing thePrepareRequest Payload. Thus, any CN that receives thePrepareResponse Payload(even without receiving thePrepareRequest) can validate both of them together if his localPrepareRequest Payloadis stillnull(for this purpose, the partial signature is important for ensuring that a third-entity does not modify primary payload content);CommitAgreementis activated) we can lock Change View. The reasoning is thatMnodes already showed their agreement with the proposed Block.Mpartial signatures are achieved, a given node signed the complete header and relay to the network.OnRegenerationis called every time that a node asks forChangeView, it contains a Payload with all Partial Signatures (SignedPayloads) plus the originalPrepareRequestPayload. It calls aMakeRegenarationevent that will be spread throughout the network. That node (that asked forchangeview) will be able to verify the OnRegeneration and even reduce its View number if it sees that Commit phase was already been achieved in a lower view.Part I main contributors: @erikzhang, @shargon, @igormcoelho, @vncoelho, @belane, @wanglongfei88, @edwardz246003
Analogy of the proposal
An analogy of the proposal is the indefatigable miners problem (defined here):