-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use PrepareRequest
?
public PrepareRequest Request;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
public class ConsensusPayload : IInventory | |
{ | |
public uint Version; | |
public UInt256 PrevHash; | |
public uint BlockIndex; | |
public ushort ValidatorIndex; | |
public uint Timestamp; | |
public byte[] Data; | |
public Witness Witness; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to save all the ResponseSignature
s. You can relay the received PrepareResponse
message again when needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
B
receivesPrepRequest
fromA
and sendResponse
C
receivesResponse_B
(indirectly getsPrepRequest_A
) and sendResponse
D
only getsResponse_C
and 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erikzhang, can/should I merge igormcoelho#5?
/// <summary> | ||
/// Final block signature | ||
/// </summary> | ||
public byte[] FinalSignature; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
if (PrimaryIndex == i && !Crypto.Default.VerifySignature(block.GetHashData(), FinalSignatures[i], Validators[i].EncodePoint(false))) | |
{ | |
// CheckFinalSignatures...It looks like that Primary tried to cheat providing wrong final signature! His header signature will not be considered | |
if ((FinalSignatures.Count(p => p != null) - 1) >= M) | |
continue; | |
else | |
return null; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
public void ChangeView(byte view_number) | |
{ | |
ViewNumber = view_number; | |
PrimaryIndex = GetPrimaryIndex(view_number); | |
PreparePayload = null; | |
TransactionHashes = null; | |
SignedPayloads = new byte[Validators.Length][]; | |
FinalSignatures = new byte[Validators.Length][]; | |
if (MyIndex >= 0) | |
{ | |
ExpectedView[MyIndex] = view_number; | |
} | |
_header = null; | |
} |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
send FinalSignature_{primary}
, in advance, or not send, this is the question. ahueahueahueahueaea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
FillingContext
Part I: Highlights of the proposal (which is a consolidation of previous knowledge discussed in other issues and PRs)
M
partials signatures should be obtained before sending a complete signature of thecontext.makeHeader
;PrepareRequest Payload
. One peculiarity is that the Speaker/Primary signs thePrepareRequest Payload_1
with an empty vector of his signature (as one could expected), while theBackups
signs the completePrepareRequest Payload_Complete
that contains Speaker signature of the aforementionedPrepareRequest Payload_1
;PrepareRequest Payload
since everyPrepareResponse
Payload 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 Payload
is stillnull
(for this purpose, the partial signature is important for ensuring that a third-entity does not modify primary payload content);CommitAgreement
is activated) we can lock Change View. The reasoning is thatM
nodes already showed their agreement with the proposed Block.M
partial signatures are achieved, a given node signed the complete header and relay to the network.OnRegeneration
is called every time that a node asks forChangeView
, it contains a Payload with all Partial Signatures (SignedPayloads
) plus the originalPrepareRequestPayload
. It calls aMakeRegenaration
event 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):