ssz: implement non-implemented tests #165
Conversation
|
@aaron-blox @lior-blox status? |
GalRogozinski
changed the base branch from
os/ssz_expirements
to
ssz-beacon-version-block
nkryuchkov
force-pushed
the
ssz-implement-non-implemented-tests
branch
from
e395efb
to
a9c3322
Compare
| var TestingPrepareMessageWithRoundAndFullData = func( | ||
| sk *bls.SecretKey, | ||
| id types.OperatorID, | ||
| round qbft.Round, | ||
| fullData []byte, | ||
| ) *qbft.SignedMessage { | ||
| msg := &qbft.Message{ | ||
| MsgType: qbft.PrepareMsgType, | ||
| Height: qbft.FirstHeight, | ||
| Round: round, | ||
| Identifier: TestingIdentifier, | ||
| Root: sha256.Sum256(fullData), | ||
| } | ||
| ret := SignQBFTMsg(sk, id, msg) | ||
| ret.FullData = fullData | ||
| return ret | ||
| } |
There was a problem hiding this comment.
This might not be in the scope of this PR... But maybe we should change TestingPrepareMessageWithParams to accept fullData?
There was a problem hiding this comment.
I agree that this needs to be done, but that's actually not the only thing that needs to be done among message generators, and solving it can potentially cause a huge amount of merge conflicts. I'd prefer doing this change in a separate PR and merging it quickly (it should be easy to review such a PR and make sure it doesn't break anything because all tests will pass)
qbft/spectest/tests/messages/marshal_justification_with_full_data.go
Outdated
Show resolved
Hide resolved
qbft/spectest/tests/messages/marshal_justification_without_full_data.go
Outdated
Show resolved
Hide resolved
…onsWithFullData, messages.UnmarshalJustifications
olegshmuelov
force-pushed
the
ssz-implement-non-implemented-tests
branch
from
b4c5555
to
486e7d9
Compare
| panic("implement") | ||
| ks := testingutils.Testing4SharesSet() | ||
|
|
||
| msg := testingutils.TestingCommitMultiSignerMessageWithHeight( |
There was a problem hiding this comment.
@GalRogozinski this kind of makes it a weird situation no? A decided message was received but the node thinks its invalid so it is basically stuck?
There was a problem hiding this comment.
Is this the expected behavior we expect?
There was a problem hiding this comment.
I see
If I understand correctly QBFT doesn't know what "invalid" data is. It just wants to reach a consensus on some data.
Also SSV protocol only has "slashable" checks. It doesn't know if the data is valid for the underlying blockchain protocol (ethereum in our case).
So maybe we should change "invalid" to "arbitrary"? Because "invalid" implies that it should be rejected.
There was a problem hiding this comment.
actually what I wrote is inaccurate. We currently do have validity checks for consensus data. But we check validity at earlier phases, and this should suffice. So I think the conclusion from my last answer holds...
There was a problem hiding this comment.
Lets open a ticket to handle this in the future.
Currently what will happen is if an invalid data is decided, a node might reject it and fail.
Only way this can happen, I think, is if there was a fork and the node didn't update (because a majority did sign it after all)
There was a problem hiding this comment.
There are 2 scenarios a node will get an invalid decided message
- malicious cluster majority
- fork, didn't update.
In both cases the node should reject I think
No description provided.