core/qbft: refactor to explicit justifications #500
Conversation
Codecov Report
@@ Coverage Diff @@
## main #500 +/- ##
==========================================
+ Coverage 56.39% 56.53% +0.13%
==========================================
Files 87 87
Lines 7846 7974 +128
==========================================
+ Hits 4425 4508 +83
- Misses 2829 2866 +37
- Partials 592 600 +8
Continue to review full report at Codecov.
|
corverroos
force-pushed
the
corver/qbftexplicit
branch
from
0fd9213
to
0972499
Compare
corverroos
added
the
merge when ready
| // Run returns the consensus decided value (Qcommit) or a context closed error. | ||
| func Run(ctx context.Context, d Definition, t Transport, instance, process int64, inputValue []byte) (value []byte, err error) { | ||
| // Run executes the consensus algorithm until the context closed. | ||
| //nolint:gocognit // If is indeed a complex algorithm. |
There was a problem hiding this comment.
| //nolint:gocognit // If is indeed a complex algorithm. | |
| //nolint:gocognit // It is indeed a complex algorithm. |
There was a problem hiding this comment.
will fix in generics PR 👍
| preparedJustify []Msg | ||
| qCommit []Msg | ||
| buffer []Msg | ||
| dedupIn = make(map[dedupKey]bool) |
There was a problem hiding this comment.
A very common term for "deduplication"
| func Run(ctx context.Context, d Definition, t Transport, instance, process int64, inputValue []byte) (value []byte, err error) { | ||
| // Run executes the consensus algorithm until the context closed. | ||
| //nolint:gocognit // If is indeed a complex algorithm. | ||
| func Run(ctx context.Context, d Definition, t Transport, instance []byte, process int64, inputValue []byte) (err error) { |
There was a problem hiding this comment.
Suggest: parameter names can be def Defination, trn Transport
There was a problem hiding this comment.
sure, but d and t also works no?
There was a problem hiding this comment.
def sounds more of defination and t confuses with t of testing
| @@ -72,13 +75,22 @@ const ( | |||
|
|
|||
| // Msg defines the inter process messages. | |||
There was a problem hiding this comment.
suggest: replace word process with nodes as it is confusing since we deal with Charon nodes not processes
There was a problem hiding this comment.
the algorithm paper talks about processes (so sticking with its terminology)
There was a problem hiding this comment.
yeah sure, paper talks about processes but we are dealing with nodes not processes so we can/should modify terminologies as per our usecases
Refactors QBFT to explicit justifications.
Also:
category: refactor
ticket: #445
feature_set: alpha