-
Notifications
You must be signed in to change notification settings - Fork 143
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
Add additional BlockCommit
validation for Append()
#2541
Comments
Please help me out by having a clear view of this. Let's say someone is trying to "forge" a block of index
Am I following correctly? Footnotes
|
That's the gist of it. 😗 |
There were some changes made to the plan while implementing. The main difference being |
Related to #2504.
Premise
I think we were thinking about the issue in a wrong way. Although we have implemented
Block<T>.LastCommit
validation, we are missing the entire other half for validating theBlockCommit
as a proof that a consensus has been reached for theBlock<T>
we are trying to append. SaidBlockCommit
is different fromBlock<T>.LastCommit
as it will reference theBlock<T>
itself, not theBlock<T>
's previousBlock<T>
.Explanation
Any PBFT
Block<T>
that is not a genesisBlock<T>
should require aBlockCommit
to be present as an evidence that a consensus has been reached to include the saidBlock<T>
(albeit not included in theBlock<T>
as data) in order to be appended toBlockChain<T>
. It is important to differentiate the purpose of thisBlockCommit
fromBlock<T>.LastCommit
. IfBlock<T>
has an index ofn
, thenBlock<T>.LastCommit
with height ofn - 1
"signals" that whoever proposed theBlock<T>
had the right to proposeBlock<T>
in the sense that the validator has observed all the evidence to actually append the previousBlock<T>
of indexn - 1
. This, however, does not mean theBlock<T>
in question, of indexn
, is in any way valid, i.e. that a consensus has been reached, for it to be appended to aBlockChain<T>
. It just so happensSwarm<T>
propagates anyBlock<T>
's in its ownBlockChain<T>
.Any node observing
Block<T>
, either throughSwarm<T>
orContext
, must do its due diligence by whether it can observe/gather aBlockCommit
of heightn
that is valid. If it is throughSwarm<T>
, this can be observed/hijacked from aBlock<T>
of indexn + 1
which would have aLastCommit
field of heightn
. This means, when syncingBlock<T>
s through aSwarm<T>
, it should never be possible to completely sync up with a peer'sBlockChain<T>.Tip
, but only up to the penultimateBlock<T>
.Additionally, this is possible since for
Block<T>
of indexn
, the set of validators can be derived for heightn
as all the necessary state transitions must have taken place by having aBlockChain<T>
of heightn - 1
. This allows to verify and validate theLastCommit
of heightn
, i.e. the one attached to aBlock<T>
of indexn + 1
.Remedy
A starting point would be to add an additional parameter for
BlockChain<T>.Append()
that is of typeBlockCommit
and prevent appending aBlock<T>
if a validBlockCommit
is not provided (except for the genesisBlock<T>
1).Notes
By some unforeseen fault, either by missing
ConsensusMsg
s due to some network failure or by receiving invalidConsensusMsg
s due to an activity of a malicious node, a node'sContext
might become "out-of-sync" with the rest of the network. As long as it is below tolerance, i.e. 1/3 of the total power of the network, it seems the network should recover on its own. Even when there was a race condition between theSwarm<T>
and aContext
, it seems this wasn't really much of a problem (other than there being an obvious security hole where a malicious actor could've forced a fork on an honest node sinceSwarm<T>
currently bypasses the consensus check). With the updated approach provided here,Context
would generally take precedence toSwarm<T>
. Consider the following thought experiment.As-is scenario:
Context
of heightn
(i.e. itsBlockChain<T>.Tip.Index
isn - 1
).Context
is stuck atn
.Swarm<T>
forcibly syncs itsBlockChain<T>.Tip
to a known peer of heightn
(although it shouldn't as explained above).Context
is discarded and a newContext
of heightn + 1
gets started.Context
is on heightn + 1
.BlockCommit
for heightn
to provide asLastCommit
ofBlock<T>
of indexn + 1
, the node is incapable of proposing aBlock<T>
for aContext
of heightn + 1
2.Context
of heightn + 1
,Context
of heightn + 2
should start normally.To-be scenario:
Context
of heightn
(i.e. itsBlockChain<T>.Tip.Index
isn - 1
).Context
is stuck atn
.Swarm<T>
forcibly syncs itsBlockChain<T>.Tip
to a known peer of heightn + 1
.Context
is discarded and a newContext
of heightn + 1
gets started.Context
3 is on heightn + 2
.BlockCommit
for heightn
to provide asLastCommit
ofBlock<T>
of indexn + 1
, and the node is free to propose aBlock<T>
of indexn + 1
, it will be ignored.n + 2
, the node will not get the votes for its proposedBlock<T>
.ConsensusMsg
s (assuming there were no fault during network'sContext
of heightn + 1
phase), the node will quickly reach the same consensus as other nodes (even if it hasn't participated in it) for heightn + 1
.Context
for heightn + 2
, although the node might've missed several rounds, the rest should progress normally onwards. This means, the nodeContext
will become normal by the timeContext
for heightn + 3
starts.Footnotes
Note that this requirement is slightly different from whether
LastCommit
should or should not benull
for a PBFTBlock<T>
. For example, a PBFTBlock<T>
of index1
should be provided with aBlockCommit
during theBlockChain<T>.Append()
call, but theBlock<T>
itself would be missing aLastCommit
, i.e. it'll benull
. ↩This is not entirely true, as the node can propose under certain conditions by "copying" the
Block<T>
proposed by someone else. ↩I am using this term very loosely. 😗 ↩
The text was updated successfully, but these errors were encountered: