Skip to content
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

Tests - Partial signatures container #338

Merged
merged 6 commits into from
Feb 6, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ssv/spectest/all_tests.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package spectest
import (
"github.com/bloxapp/ssv-spec/ssv/spectest/tests"
"github.com/bloxapp/ssv-spec/ssv/spectest/tests/messages"
"github.com/bloxapp/ssv-spec/ssv/spectest/tests/partialsigcontainer"
"github.com/bloxapp/ssv-spec/ssv/spectest/tests/runner"
"github.com/bloxapp/ssv-spec/ssv/spectest/tests/runner/consensus"
"github.com/bloxapp/ssv-spec/ssv/spectest/tests/runner/duties/newduty"
Expand Down Expand Up @@ -151,4 +152,10 @@ var AllTests = []tests.TestF{
valcheckattestations.ConsensusDataNil,
valcheckattestations.Valid,
valcheckproposer.BlindedBlock,

partialsigcontainer.OneSignature,
partialsigcontainer.Quorum,
partialsigcontainer.Duplicate,
partialsigcontainer.DuplicateQuorum,
partialsigcontainer.Invalid,
}
2 changes: 1 addition & 1 deletion ssv/spectest/generate/tests.json

Large diffs are not rendered by default.

8 changes: 8 additions & 0 deletions ssv/spectest/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/bloxapp/ssv-spec/ssv"
tests2 "github.com/bloxapp/ssv-spec/ssv/spectest/tests"
"github.com/bloxapp/ssv-spec/ssv/spectest/tests/messages"
"github.com/bloxapp/ssv-spec/ssv/spectest/tests/partialsigcontainer"
"github.com/bloxapp/ssv-spec/ssv/spectest/tests/runner/duties/newduty"
"github.com/bloxapp/ssv-spec/ssv/spectest/tests/runner/duties/synccommitteeaggregator"
"github.com/bloxapp/ssv-spec/ssv/spectest/tests/valcheck"
Expand Down Expand Up @@ -134,6 +135,13 @@ func parseAndTest(t *testing.T, name string, test interface{}) {
Tests: typedTests,
}

typedTest.Run(t)
case reflect.TypeOf(&partialsigcontainer.PartialSigContainerTest{}).String():
byts, err := json.Marshal(test)
require.NoError(t, err)
typedTest := &partialsigcontainer.PartialSigContainerTest{}
require.NoError(t, json.Unmarshal(byts, &typedTest))

typedTest.Run(t)
default:
panic("unsupported test type " + testType)
Expand Down
36 changes: 36 additions & 0 deletions ssv/spectest/tests/partialsigcontainer/duplicate.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package partialsigcontainer

import (
"github.com/bloxapp/ssv-spec/qbft"
"github.com/bloxapp/ssv-spec/ssv/spectest/tests"
"github.com/bloxapp/ssv-spec/types"
"github.com/bloxapp/ssv-spec/types/testingutils"
)

func Duplicate() tests.SpecTest {

// Create a test key set
ks := testingutils.Testing4SharesSet()

// Create PartialSignatureMessage for testing
msg1 := testingutils.PostConsensusAttestationMsg(ks.Shares[1], 1, qbft.FirstHeight)
msg2 := testingutils.PostConsensusAttestationMsg(ks.Shares[1], 1, qbft.FirstHeight)
msg3 := testingutils.PostConsensusAttestationMsg(ks.Shares[2], 2, qbft.FirstHeight)
msgs := []*types.PartialSignatureMessage{msg1.Message.Messages[0], msg2.Message.Messages[0], msg3.Message.Messages[0]}

// Verify the reconstructed signature
expectedSig, err := types.ReconstructSignatures(map[types.OperatorID][]byte{1: msgs[0].PartialSignature, 3: msgs[2].PartialSignature})
if err != nil {
panic(err.Error())
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am actually surprised that this doesn't err...
you think we need this? anyhow if there is an expectedError then expectedResult isn't checked

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed isn't checked and we don't need. It's only confusing so I'll drop.


return &PartialSigContainerTest{
Name: "duplicate",
Quorum: ks.Threshold,
ValidatorPubKey: ks.ValidatorPK.Serialize(),
SignatureMsgs: msgs,
ExpectedError: "could not reconstruct a valid signature",
ExpectedResult: expectedSig.Serialize(),
ExpectedQuorum: false,
}
}
36 changes: 36 additions & 0 deletions ssv/spectest/tests/partialsigcontainer/duplicatequorum.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package partialsigcontainer

import (
"github.com/bloxapp/ssv-spec/qbft"
"github.com/bloxapp/ssv-spec/ssv/spectest/tests"
"github.com/bloxapp/ssv-spec/types"
"github.com/bloxapp/ssv-spec/types/testingutils"
)

func DuplicateQuorum() tests.SpecTest {

// Create a test key set
ks := testingutils.Testing4SharesSet()

// Create PartialSignatureMessage for testing
msg1 := testingutils.PostConsensusAttestationMsg(ks.Shares[1], 1, qbft.FirstHeight)
msg12 := testingutils.PostConsensusAttestationMsg(ks.Shares[1], 1, qbft.FirstHeight)
msg2 := testingutils.PostConsensusAttestationMsg(ks.Shares[2], 2, qbft.FirstHeight)
msg3 := testingutils.PostConsensusAttestationMsg(ks.Shares[3], 3, qbft.FirstHeight)
msgs := []*types.PartialSignatureMessage{msg1.Message.Messages[0], msg12.Message.Messages[0], msg2.Message.Messages[0], msg3.Message.Messages[0]}

// Verify the reconstructed signature
expectedSig, err := types.ReconstructSignatures(map[types.OperatorID][]byte{1: msgs[0].PartialSignature, 2: msgs[2].PartialSignature, 3: msgs[3].PartialSignature})
if err != nil {
panic(err.Error())
}

return &PartialSigContainerTest{
Name: "duplicate quorum",
Quorum: ks.Threshold,
ValidatorPubKey: ks.ValidatorPK.Serialize(),
SignatureMsgs: msgs,
ExpectedResult: expectedSig.Serialize(),
ExpectedQuorum: true,
}
}
Comment on lines +10 to +36
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we test a scenario where we have a duplicate msg but one is signed correctly and the other not? what are we expecting in that case? I guess to take the valid.
it that the Invalid test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PartialSigContainer internal functionality is to only add a partial signature for a (signer, signing root) once. It uses a map for that. If we try to add another partial sig for the same author (even if the first one is invalid and the second one is valid), it won't do anything because the map already has this entry. I.e. it doesn't do any validation before adding.

For the duplicate and quorum test (this one), as a black box, we expect the partial sig container to produce the correct signature even if we call AddSignature twice for the same message/signature.

The two test cases you said are not tested. It will raise an error when reconstructing the signature if we add the invalid one first.

The invalid test, actually, tests a quorum with wrong signatures.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two test cases you said are not tested. It will raise an error when reconstructing the signature if we add the invalid one first.

@MatheusFranco99 should we add an issue for those two tests, or they exist in other PRs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GalRogozinski
Actually, I should have said the "single test case" he mentioned.

Receiving a first valid and then a second invalid message from the same author is not tested here but I don't see this as an issue. I think that the current PartialSigContainer module isn't supposed to do validation. On the other hand, the Runner is supposed to do it and we have the tests for it.

36 changes: 36 additions & 0 deletions ssv/spectest/tests/partialsigcontainer/invalid.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package partialsigcontainer

import (
"github.com/bloxapp/ssv-spec/qbft"
"github.com/bloxapp/ssv-spec/ssv/spectest/tests"
"github.com/bloxapp/ssv-spec/types"
"github.com/bloxapp/ssv-spec/types/testingutils"
)

func Invalid() tests.SpecTest {

// Create a test key set
ks := testingutils.Testing4SharesSet()

// Create PartialSignatureMessage for testing
msg1 := testingutils.PostConsensusAttestationMsg(ks.Shares[1], 1, qbft.FirstHeight)
msg2 := testingutils.PostConsensusAttestationMsg(ks.Shares[1], 2, qbft.FirstHeight) // invalid signature
msg3 := testingutils.PostConsensusAttestationMsg(ks.Shares[3], 3, qbft.FirstHeight)
msgs := []*types.PartialSignatureMessage{msg1.Message.Messages[0], msg2.Message.Messages[0], msg3.Message.Messages[0]}

// Verify the reconstructed signature
expectedSig, err := types.ReconstructSignatures(map[types.OperatorID][]byte{1: msgs[0].PartialSignature, 2: msgs[1].PartialSignature, 3: msgs[2].PartialSignature})
if err != nil {
panic(err.Error())
}

return &PartialSigContainerTest{
Name: "invalid",
Quorum: ks.Threshold,
ValidatorPubKey: ks.ValidatorPK.Serialize(),
SignatureMsgs: msgs,
ExpectedError: "could not reconstruct a valid signature",
ExpectedResult: expectedSig.Serialize(),
ExpectedQuorum: true,
}
}
34 changes: 34 additions & 0 deletions ssv/spectest/tests/partialsigcontainer/one_signature.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package partialsigcontainer

import (
"github.com/bloxapp/ssv-spec/qbft"
"github.com/bloxapp/ssv-spec/ssv/spectest/tests"
"github.com/bloxapp/ssv-spec/types"
"github.com/bloxapp/ssv-spec/types/testingutils"
)

func OneSignature() tests.SpecTest {

// Create a test key set
ks := testingutils.Testing4SharesSet()

// Create PartialSignatureMessage for testing
msg := testingutils.PostConsensusAttestationMsg(ks.Shares[1], 1, qbft.FirstHeight)
msgs := []*types.PartialSignatureMessage{msg.Message.Messages[0]}

// Verify the reconstructed signature
expectedSig, err := types.ReconstructSignatures(map[types.OperatorID][]byte{1: msgs[0].PartialSignature})
if err != nil {
panic(err.Error())
}

return &PartialSigContainerTest{
Name: "one signature",
Quorum: ks.Threshold,
ValidatorPubKey: ks.ValidatorPK.Serialize(),
SignatureMsgs: msgs,
ExpectedError: "could not reconstruct a valid signature",
ExpectedResult: expectedSig.Serialize(),
ExpectedQuorum: false,
}
}
35 changes: 35 additions & 0 deletions ssv/spectest/tests/partialsigcontainer/quorum.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package partialsigcontainer

import (
"github.com/bloxapp/ssv-spec/qbft"
"github.com/bloxapp/ssv-spec/ssv/spectest/tests"
"github.com/bloxapp/ssv-spec/types"
"github.com/bloxapp/ssv-spec/types/testingutils"
)

func Quorum() tests.SpecTest {

// Create a test key set
ks := testingutils.Testing4SharesSet()

// Create PartialSignatureMessage for testing
msg1 := testingutils.PostConsensusAttestationMsg(ks.Shares[1], 1, qbft.FirstHeight)
msg2 := testingutils.PostConsensusAttestationMsg(ks.Shares[2], 2, qbft.FirstHeight)
msg3 := testingutils.PostConsensusAttestationMsg(ks.Shares[3], 3, qbft.FirstHeight)
msgs := []*types.PartialSignatureMessage{msg1.Message.Messages[0], msg2.Message.Messages[0], msg3.Message.Messages[0]}

// Verify the reconstructed signature
expectedSig, err := types.ReconstructSignatures(map[types.OperatorID][]byte{1: msgs[0].PartialSignature, 2: msgs[1].PartialSignature, 3: msgs[2].PartialSignature})
if err != nil {
panic(err.Error())
}

return &PartialSigContainerTest{
Name: "quorum",
Quorum: ks.Threshold,
ValidatorPubKey: ks.ValidatorPK.Serialize(),
SignatureMsgs: msgs,
ExpectedResult: expectedSig.Serialize(),
ExpectedQuorum: true,
}
}
54 changes: 54 additions & 0 deletions ssv/spectest/tests/partialsigcontainer/test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package partialsigcontainer

import (
"testing"

"github.com/bloxapp/ssv-spec/ssv"
"github.com/bloxapp/ssv-spec/types"
"github.com/stretchr/testify/require"
)

type PartialSigContainerTest struct {
Name string
Quorum uint64
ValidatorPubKey []byte
SignatureMsgs []*types.PartialSignatureMessage
ExpectedError string
ExpectedResult []byte
ExpectedQuorum bool
}

func (test *PartialSigContainerTest) TestName() string {
return "PartialSigContainer " + test.Name
}

func (test *PartialSigContainerTest) Run(t *testing.T) {
ps := ssv.NewPartialSigContainer(test.Quorum)

roots := make(map[[32]byte]bool)
// Add signature messages
for _, sigMsg := range test.SignatureMsgs {
ps.AddSignature(sigMsg)
roots[sigMsg.SigningRoot] = true
}

for root := range roots {

// Check quorum
require.Equal(t, test.ExpectedQuorum, ps.HasQuorum(root))

result, err := ps.ReconstructSignature(root, test.ValidatorPubKey)
// Check the result and error
if len(test.ExpectedError) > 0 {
require.Error(t, err)
require.Contains(t, err.Error(), test.ExpectedError)
} else {
require.NoError(t, err)
require.EqualValues(t, test.ExpectedResult, result)
}
}
}

func (test *PartialSigContainerTest) GetPostState() (interface{}, error) {
return nil, nil
}