failure indicating commitments #3391
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kostko
reviewed
Oct 8, 2020
kostko
added
c:breaking/runtime
ptrus
force-pushed
the
ptrus/feature/failure-indicating-commitments
branch
3 times, most recently
from
d5d902f
to
c2df984
Compare
kostko
reviewed
Oct 9, 2020
ptrus
force-pushed
the
ptrus/feature/failure-indicating-commitments
branch
9 times, most recently
from
3cee767
to
5442a19
Compare
Codecov Report
@@ Coverage Diff @@
## master #3391 +/- ##
==========================================
- Coverage 66.03% 65.93% -0.10%
==========================================
Files 371 371
Lines 33285 33367 +82
==========================================
+ Hits 21979 22002 +23
- Misses 8075 8127 +52
- Partials 3231 3238 +7
Continue to review full report at Codecov.
|
ptrus
force-pushed
the
ptrus/feature/failure-indicating-commitments
branch
7 times, most recently
from
60c624f
to
cdf79b1
Compare
kostko
reviewed
Oct 13, 2020
ptrus
force-pushed
the
ptrus/feature/failure-indicating-commitments
branch
3 times, most recently
from
87b571b
to
e79973c
Compare
4 tasks
ptrus
force-pushed
the
ptrus/feature/failure-indicating-commitments
branch
from
e79973c
to
36fba86
Compare
Added, not sure how to best test these cases maybe with byzantine-storage tests, need to think about it a bit, but probably can be done in a separate PR. |
|
Yeah we should add Byzantine tests for all these cases, can you open an issue for it? |
kostko
reviewed
Oct 15, 2020
ptrus
force-pushed
the
ptrus/feature/failure-indicating-commitments
branch
from
36fba86
to
8f71c9e
Compare
|
ptrus
force-pushed
the
ptrus/feature/failure-indicating-commitments
branch
from
8f71c9e
to
f2b8be8
Compare
kostko
approved these changes
Oct 15, 2020
| n.abortBatchLocked(err) | ||
| return | ||
| } | ||
|
|
||
| // Sign the commitment and submit. | ||
| commit, err := commitment.SignExecutorCommitment(n.commonNode.Identity.NodeSigner, proposedResults) | ||
| // TODO: Add crash point. |
There was a problem hiding this comment.
I don't think we need one as there is one below and nothing interesting happens inbetween.
| return | ||
| } | ||
|
|
||
| if batch == nil || batch.computed == nil { |
There was a problem hiding this comment.
Since this body is longer, maybe invert the check so less stuff is indented?
ptrus
force-pushed
the
ptrus/feature/failure-indicating-commitments
branch
from
f2b8be8
to
82f876d
Compare
ptrus
force-pushed
the
ptrus/feature/failure-indicating-commitments
branch
from
82f876d
to
66063cc
Compare
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The E2E tests with increased epoch times (#3322) discovered a failure edge case that can happen if the storage committee becomes unavailable after proposer already proposes a batch, but before any of the nodes submit a commitment. In that case the nodes currently abort the processing and wait for a round failure. Since there is no submitted commitments the round failure only happens at the next epoch transition.
This PR adds support for failure indicating commitments (which were first proposed as part of the ADR 0005), which solve the above mentioned issue since compute nodes can now submit a failure indicating commitment and with it trigger a round failure.
NOTE: only the subset (support for failure indicating commitments) of the mentioned ADR is implemented here.
TODO: