Conversation
Test implemented summaryUnit
Integration
@pierluca @AnomalRoil What do you think of this test coverage? Can you think of more tests that I should have? |
|
One test I would add is:
That is: assuming partial connectivity rather than full connectivity. |
AnomalRoil
left a comment
AnomalRoil
left a comment
There was a problem hiding this comment.
LGTM so far, but I'm a bit confused with all the Message types you have:
- generic message types
- concrete message types
- protobuf message types
Why can't you rely on just the protobuf types and use them directly?
thehoul
force-pushed
the
rbc
branch
4 times, most recently
from
fd43447 to
5efba0d
Compare
|
Left todo
|
thehoul
force-pushed
the
rbc
branch
2 times, most recently
from
b69ce9f to
f5817e1
Compare
pierluca
left a comment
pierluca
left a comment
There was a problem hiding this comment.
First chunk of review, working my way through the various files :-)
More will come...
| package reedsolomon | ||
|
|
||
| import ( | ||
| "github.com/HACKERALERT/infectious" |
There was a problem hiding this comment.
Let's avoid pulling that dependency and copy the structs you need instead.
There was a problem hiding this comment.
Their licensing is clear and compatible, but non-trivial, I'd be very cautious either ways when adjusting the licensing in this repo as well.
https://github.com/HACKERALERT/infectious/blob/master/LICENSE
There was a problem hiding this comment.
I'll need help to figure out the licensing, I've never done it before!
thehoul
force-pushed
the
rbc
branch
2 times, most recently
from
825e856 to
a6f0066
Compare
add general protobuf Packet message type for all ADKG messages
improve tests and add UDP network small and stress tests
improve RBC to use less maps refactor a few names and add comments
add general protobuf Packet message type for all ADKG messages
rbc/bracha/rbc.go
Outdated
| b.finished = true | ||
| b.value = s |
There was a problem hiding this comment.
these look like "writes", are you sure you don't need a b.Lock rather for these?
| func checkReadyThreshold(readyCount, threshold int) bool { | ||
| return readyCount > threshold | ||
| } | ||
|
|
||
| // checkEchoThreshold checks if the number of ECHO messages has | ||
| // reached the required threshold | ||
| func checkEchoThreshold(echoCount, threshold int) bool { | ||
| return echoCount > 2*threshold | ||
| } |
There was a problem hiding this comment.
do we want >= or > to consider the threshold is reached? I think it depends on whether you talk about the threshold of malicious parties, as in 3f+1 for at most f malicious, and then you want at least 2f+1, or if we are talking about the threshold t of required messages in which case I think >= is more correct.
Opinion?
| // Config | ||
| network := networking.NewFakeNetwork() | ||
| threshold := 1 | ||
| nbNodes := 3*threshold + 1 |
There was a problem hiding this comment.
looks like this answers my previous comment, but you might want to document you're talking about the threshold of malicious parties f whenever you say threshold somewhere in the documentation.
3 participants
Implementation according to algorithm 4 in this paper: https://eprint.iacr.org/2021/777.pdf