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.

Sign up for GitHub

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

refactor: solomachine generic VerifyNonMembership #1720

Merged
merged 5 commits into from
Jul 27, 2022
Merged

refactor: solomachine generic VerifyNonMembership #1720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
82c4ded
adding verification of non-membership with tests
damiannolan Jul 17, 2022
a438052
refactor common code to produceVerificationArgs
damiannolan Jul 18, 2022
a1bd87b
Merge branch 'damian/solomachine-generic-verify' into damian/solomach…
damiannolan Jul 19, 2022
7769258
removing unused produce args func
damiannolan Jul 19, 2022
b8d5d50
Update modules/light-clients/06-solomachine/client_state_test.go
damiannolan Jul 27, 2022
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
96 changes: 38 additions & 58 deletions modules/light-clients/06-solomachine/client_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,55 +118,16 @@ func (cs *ClientState) VerifyMembership(
path []byte,
value []byte,
) error {
// TODO: Attempt to refactor code to smaller function
if revision := height.GetRevisionNumber(); revision != 0 {
return sdkerrors.Wrapf(sdkerrors.ErrInvalidHeight, "revision must be 0 for solomachine, got revision-number: %d", revision)
}

// sequence is encoded in the revision height of height struct
sequence := height.GetRevisionHeight()
latestSequence := cs.GetLatestHeight().GetRevisionHeight()
if latestSequence != sequence {
return sdkerrors.Wrapf(
sdkerrors.ErrInvalidHeight,
"client state sequence != proof sequence (%d != %d)", latestSequence, sequence,
)
publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, proof)
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] I know produceVerificationArgs wasn't changed in this PR, so feel free to dis-regard, but I think 5 values is a bit too many to return. What do you think about wrapping the first 4 values in a struct and having the the method signature return the struct and an error? (unless we can't change the method signature for some reason)

verificationArgs, err := produceVerificationArgs(cdc, cs, height, proof)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like a great suggestion to me

I guess my only concern would be constructing a type verificationArgs which are dependent on arguments that needed to be checked rather than the totality of arguments required for verification. The path/diversifier/data being the others. We could also take away some of the return arguments (public key, sequence) can be derived from the client state. Originally this function was made to reduce redundancy of calls by 6-7 functions, now only 2 functions rely on it so redundancy isn't as much of a concern. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

In general I agree, and normally lean against having such a large number of return args. I'd be happy to continue the discussion or explore options for improving the function sig and its usage, maybe we could potentially open an issue for this? At least this an unexported function so whatever we decide should not have any implications on consumers.
I'm going to go ahead and merge this to the [WIP] branch and get it R4R with renaming types to remove the V2 suffixes.

if err != nil {
return err
}

var merklePath commitmenttypes.MerklePath
if err := cdc.Unmarshal(path, &merklePath); err != nil {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "failed to unmarshal path into ICS 23 commitment merkle path")
}

var timestampedSigData TimestampedSignatureData
if err := cdc.Unmarshal(proof, &timestampedSigData); err != nil {
return sdkerrors.Wrapf(err, "failed to unmarshal proof into type %T", timestampedSigData)
}

timestamp := timestampedSigData.Timestamp

if len(timestampedSigData.SignatureData) == 0 {
return sdkerrors.Wrap(ErrInvalidProof, "signature data cannot be empty")
}

sigData, err := UnmarshalSignatureData(cdc, timestampedSigData.SignatureData)
if err != nil {
return err
}

if cs.ConsensusState == nil {
return sdkerrors.Wrap(clienttypes.ErrInvalidConsensus, "consensus state cannot be empty")
}

if cs.ConsensusState.GetTimestamp() > timestamp {
return sdkerrors.Wrapf(ErrInvalidProof, "the consensus state timestamp is greater than the signature timestamp (%d >= %d)", cs.ConsensusState.GetTimestamp(), timestamp)
}

publicKey, err := cs.ConsensusState.GetPubKey()
if err != nil {
return err
}

signBytes := &SignBytesV2{
Sequence: sequence,
Timestamp: timestamp,
Expand Down Expand Up @@ -203,11 +164,40 @@ func (cs *ClientState) VerifyNonMembership(
proof []byte,
path []byte,
) error {
// TODO: Implement 06-solomachine VerifyNonMembership
publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, proof)
if err != nil {
return err
}

var merklePath commitmenttypes.MerklePath
if err := cdc.Unmarshal(path, &merklePath); err != nil {
return sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "failed to unmarshal path into ICS 23 commitment merkle path")
}

signBytes := &SignBytesV2{
Sequence: sequence,
Timestamp: timestamp,
Diversifier: cs.ConsensusState.Diversifier,
Path: []byte(merklePath.String()),
Data: nil,
}

signBz, err := cdc.Marshal(signBytes)
if err != nil {
return err
}

if err := VerifySignature(publicKey, signBz, sigData); err != nil {
return err
}

cs.Sequence++
cs.ConsensusState.Timestamp = timestamp
setClientState(clientStore, cdc, cs)

return nil
}

// TODO: Remove function as no longer used
// produceVerificationArgs perfoms the basic checks on the arguments that are
// shared between the verification functions and returns the public key of the
// consensus state, the unmarshalled proof representing the signature and timestamp
Expand All @@ -216,34 +206,22 @@ func produceVerificationArgs(
cdc codec.BinaryCodec,
cs *ClientState,
height exported.Height,
prefix exported.Prefix,
proof []byte,
) (cryptotypes.PubKey, signing.SignatureData, uint64, uint64, error) {
if revision := height.GetRevisionNumber(); revision != 0 {
return nil, nil, 0, 0, sdkerrors.Wrapf(sdkerrors.ErrInvalidHeight, "revision must be 0 for solomachine, got revision-number: %d", revision)
}
// sequence is encoded in the revision height of height struct
sequence := height.GetRevisionHeight()
if prefix == nil {
return nil, nil, 0, 0, sdkerrors.Wrap(commitmenttypes.ErrInvalidPrefix, "prefix cannot be empty")
}

_, ok := prefix.(*commitmenttypes.MerklePrefix)
if !ok {
return nil, nil, 0, 0, sdkerrors.Wrapf(commitmenttypes.ErrInvalidPrefix, "invalid prefix type %T, expected MerklePrefix", prefix)
}

if proof == nil {
return nil, nil, 0, 0, sdkerrors.Wrap(ErrInvalidProof, "proof cannot be empty")
}

timestampedSigData := &TimestampedSignatureData{}
if err := cdc.Unmarshal(proof, timestampedSigData); err != nil {
var timestampedSigData TimestampedSignatureData
if err := cdc.Unmarshal(proof, &timestampedSigData); err != nil {
return nil, nil, 0, 0, sdkerrors.Wrapf(err, "failed to unmarshal proof into type %T", timestampedSigData)
}

timestamp := timestampedSigData.Timestamp

if len(timestampedSigData.SignatureData) == 0 {
return nil, nil, 0, 0, sdkerrors.Wrap(ErrInvalidProof, "signature data cannot be empty")
}
Expand All @@ -257,6 +235,8 @@ func produceVerificationArgs(
return nil, nil, 0, 0, sdkerrors.Wrap(clienttypes.ErrInvalidConsensus, "consensus state cannot be empty")
}

// sequence is encoded in the revision height of height struct
sequence := height.GetRevisionHeight()
latestSequence := cs.GetLatestHeight().GetRevisionHeight()
if latestSequence != sequence {
return nil, nil, 0, 0, sdkerrors.Wrapf(
Expand Down
235 changes: 235 additions & 0 deletions modules/light-clients/06-solomachine/client_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,13 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
},
false,
},
{
"proof is nil",
func() {
proof = nil
},
false,
},
{
"proof verification failed",
func() {
Expand Down Expand Up @@ -554,6 +561,234 @@ func (suite *SoloMachineTestSuite) TestVerifyMembership() {
}
}

func (suite *SoloMachineTestSuite) TestVerifyNonMembership() {
// test singlesig and multisig public keys
for _, sm := range []*ibctesting.Solomachine{suite.solomachine, suite.solomachineMulti} {

var (
clientState *solomachine.ClientState
err error
height clienttypes.Height
path []byte
proof []byte
signBytes solomachine.SignBytesV2
)

testCases := []struct {
name string
malleate func()
expPass bool
}{
{
"success",
func() {},
true,
},
{
"success: packet receipt absence verification",
func() {
merklePath := suite.solomachine.GetPacketReceiptPath(ibctesting.MockPort, ibctesting.FirstChannelID)
signBytes = solomachine.SignBytesV2{
Sequence: sm.GetHeight().GetRevisionHeight(),
Timestamp: sm.Time,
Diversifier: sm.Diversifier,
Path: []byte(merklePath.String()),
Data: nil,
}

signBz, err := suite.chainA.Codec.Marshal(&signBytes)
suite.Require().NoError(err)

sig := sm.GenerateSignature(signBz)

signatureDoc := &solomachine.TimestampedSignatureData{
SignatureData: sig,
Timestamp: sm.Time,
}

path, err = suite.chainA.Codec.Marshal(&merklePath)
suite.Require().NoError(err)

proof, err = suite.chainA.Codec.Marshal(signatureDoc)
suite.Require().NoError(err)
},
true,
},
{
"consensus state in client state is nil",
func() {
clientState = solomachine.NewClientState(1, nil, false)
},
false,
},
{
"client state latest height is less than sequence",
func() {
consensusState := &solomachine.ConsensusState{
Timestamp: sm.Time,
PublicKey: sm.ConsensusState().PublicKey,
}

clientState = solomachine.NewClientState(sm.Sequence-1, consensusState, false)
},
false,
},
{
"height revision number is not zero",
func() {
height = clienttypes.NewHeight(1, sm.GetHeight().GetRevisionHeight())
},
false,
},
{
"malformed merkle path fails to unmarshal",
func() {
path = []byte("invalid path")
},
false,
},
{
"malformed proof fails to unmarshal",
func() {
merklePath := suite.solomachine.GetClientStatePath(counterpartyClientIdentifier)
path, err = suite.chainA.Codec.Marshal(&merklePath)
suite.Require().NoError(err)

proof = []byte("invalid proof")
},
false,
},
{
"consensus state timestamp is greater than signature",
func() {
consensusState := &solomachine.ConsensusState{
Timestamp: sm.Time + 1,
PublicKey: sm.ConsensusState().PublicKey,
}

clientState = solomachine.NewClientState(sm.Sequence, consensusState, false)
},
false,
},
{
"signature data is nil",
func() {
signatureDoc := &solomachine.TimestampedSignatureData{
SignatureData: nil,
Timestamp: sm.Time,
}

proof, err = suite.chainA.Codec.Marshal(signatureDoc)
suite.Require().NoError(err)
},
false,
},
{
"consensus state public key is nil",
func() {
clientState.ConsensusState.PublicKey = nil
},
false,
},
{
"malformed signature data fails to unmarshal",
func() {
signatureDoc := &solomachine.TimestampedSignatureData{
SignatureData: []byte("invalid signature data"),
Timestamp: sm.Time,
}

proof, err = suite.chainA.Codec.Marshal(signatureDoc)
suite.Require().NoError(err)
},
false,
},
{
"proof is nil",
func() {
proof = nil
},
false,
},
{
"proof verification failed",
func() {
signBytes.Data = []byte("invalid non-membership data value")

signBz, err := suite.chainA.Codec.Marshal(&signBytes)
suite.Require().NoError(err)

sig := sm.GenerateSignature(signBz)

signatureDoc := &solomachine.TimestampedSignatureData{
SignatureData: sig,
Timestamp: sm.Time,
}

proof, err = suite.chainA.Codec.Marshal(signatureDoc)
suite.Require().NoError(err)
},
false,
},
}

for _, tc := range testCases {
tc := tc

suite.Run(tc.name, func() {
clientState = sm.ClientState()
height = clienttypes.NewHeight(sm.GetHeight().GetRevisionNumber(), sm.GetHeight().GetRevisionHeight())

merklePath := commitmenttypes.NewMerklePath("ibc", "solomachine")
signBytes = solomachine.SignBytesV2{
Sequence: sm.GetHeight().GetRevisionHeight(),
Timestamp: sm.Time,
Diversifier: sm.Diversifier,
Path: []byte(merklePath.String()),
Data: nil,
}

signBz, err := suite.chainA.Codec.Marshal(&signBytes)
suite.Require().NoError(err)

sig := sm.GenerateSignature(signBz)

signatureDoc := &solomachine.TimestampedSignatureData{
SignatureData: sig,
Timestamp: sm.Time,
}

path, err = suite.chainA.Codec.Marshal(&merklePath)
suite.Require().NoError(err)

proof, err = suite.chainA.Codec.Marshal(signatureDoc)
suite.Require().NoError(err)

tc.malleate()

var expSeq uint64
if clientState.ConsensusState != nil {
expSeq = clientState.Sequence + 1
}

err = clientState.VerifyNonMembership(
suite.chainA.GetContext(), suite.store, suite.chainA.Codec,
height, 0, 0, // solomachine does not check delay periods
proof, path,
)

if tc.expPass {
suite.Require().NoError(err)
suite.Require().Equal(expSeq, clientState.Sequence)
suite.Require().Equal(expSeq, suite.GetSequenceFromStore(), "sequence not updated in the store (%d) on valid test case %s", suite.GetSequenceFromStore(), tc.name)
} else {
suite.Require().Error(err)
}
})
}
}
}

func (suite *SoloMachineTestSuite) TestGetTimestampAtHeight() {
tmPath := ibctesting.NewPath(suite.chainA, suite.chainB)
suite.coordinator.SetupClients(tmPath)
Expand Down