Define an abstract type for wallet.Sig #293

manoranjith · 2021-12-14T10:43:37Z

Closes #292

- Previously Sig was defined as a variable length byte array and was encoded/decoded using the Encode/DecodeSig methods. - But, its length depends upon the wallet backend. So, redefine it as an interface that implements binary.(Un)marshaler interfaces. - Now, the wallet backend should implement the logic for converting a Sig to/from its binary representation. The perunio serializer can then directly Encode/Decode this binary representation as a byte array. Signed-off-by: Manoranjith <[email protected]>

- Use a struct for storing the r and s components of the ECDSA signature. - This eliminates the need for serializing / deserializing the signature each time when it is generated / verified respectively. - Also, replace the test TestSignatureSerialize with GenericMarshalerTest Signed-off-by: Manoranjith <[email protected]>

- Unexport the SigLen field, as it is not needed outside of the wallet package. - Remove the length check in signature tests. Because, VerifySignature function checks if the signature was generated by the corresponding wallet, before verifying it it. - In transactor tests, use the sigLen variable defined within the package. Signed-off-by: Manoranjith <[email protected]>

Signed-off-by: Manoranjith <[email protected]>

- (En|De)code signatures directly using perunio.(En|De)code. - SignData method on dummy account returns a Sig instead of byte array. Signed-off-by: Manoranjith <[email protected]>

- After the wallet.Sig interface was introduced, each wallet backend defines its own concrete type for implementing the Sig interface. - These implementations also include an UnmarshalBinary function, which is used for unmarshalling signatures from their binary representation. - Because of this, when binary representations of signatures have incorrect length (when they are tampered), the unmarshalling of these signatures themselves fail. Which removes the need for checking the response of VerifySignature function for signatures of incorrect length. - Hence, the tests are updated accordingly. Signed-off-by: Manoranjith <[email protected]>

Signed-off-by: Manoranjith <[email protected]>

- Previously, signatures used in tests were directly generated as byte arrays. - After the changing type of wallet.Sig from byte array to interface, this does not work. - Hence, use the wallet randomizer to generate signatures needed for tests. Signed-off-by: Manoranjith <[email protected]>

matthiasgeihs · 2021-12-14T14:07:05Z

backend/ethereum/channel/adjudicator.go

+		sigBytes, err := req.Sig.MarshalBinary()
+		if err != nil {
+			return nil, errors.WithMessage(err, "converting to byte array")
+		}
+		return a.contract.Progress(opts, params, state, ethNewState, ethActorIndex, sigBytes)


This looks a bit like you are mixing concerns here. MarshalBinary should typically be used by a marshaler that doesn't know much about the concrete type and just wants some byte representation. However, here we know the type and the contract expects a very specific byte representation.

Suggested change

sigBytes, err := req.Sig.MarshalBinary()

if err != nil {

return nil, errors.WithMessage(err, "converting to byte array")

}

return a.contract.Progress(opts, params, state, ethNewState, ethActorIndex, sigBytes)

sig, ok := req.Sig.(*ethwallet.Sig)

if !ok {

return nil, errors.Errorf("invalid signature type: %T", req.Sig)

}

return a.contract.Progress(opts, params, state, ethNewState, ethActorIndex, *sig)

matthiasgeihs · 2021-12-14T14:08:43Z

backend/ethereum/channel/adjudicator.go

+	var err error
+	for i := range signs {
+		if signs[i] != nil {
+			byteArrays[i], err = signs[i].MarshalBinary()


Same comments as above (regarding concerns of MarshalBinary vs byte representation for a contract call) applies here.

matthiasgeihs · 2021-12-14T14:11:58Z

backend/ethereum/channel/withdraw.go

+	if err != nil {
+		return assetholder.AssetHolderWithdrawalAuth{}, nil, errors.WithMessage(err, "sign data")
+	}
+	sigBytes, err := sig.MarshalBinary()


same comments as above regarding byte representation for contract call applies here.

matthiasgeihs · 2021-12-14T14:15:46Z

backend/ethereum/wallet/backend.go

+	ethSigCopy := make([]byte, sigLen)
+	copy(ethSigCopy, *ethSig)
+	if ethSigCopy[sigLen-1] >= sigVSubtract {
+		ethSigCopy[sigLen-1] -= sigVSubtract
 	}


Should add a comment that explains what is happening here.

matthiasgeihs · 2021-12-14T14:16:51Z

backend/ethereum/wallet/hd/account.go

@@ -35,14 +35,15 @@ func (a *Account) Address() wallet.Address {
 }

 // SignData is used to sign data with this account.
-func (a *Account) SignData(data []byte) ([]byte, error) {
+func (a *Account) SignData(data []byte) (wallet.Sig, error) {
 	hash := crypto.Keccak256(data)
 	sig, err := a.wallet.SignText(a.Account, hash)
 	if err != nil {
 		return nil, errors.Wrap(err, "SignText")
 	}
 	sig[64] += 27


Should add a comment that explains what is happening here.

Should use a const instead of 27. Why is the linter not catching this?

matthiasgeihs · 2021-12-14T14:48:43Z

client/updatemsgs.go

-	c.Sig, err = wallet.DecodeSig(r)
-	return err
+	c.Sig = wallet.NewSig()
+	return perunio.Decode(r, c.Sig)


So there is still a direct dependency from client onto perunio?

matthiasgeihs · 2021-12-14T14:51:21Z

wallet/sig.go

+//
+// It is sent over wire as a binary data.


This part is not useful to mention here as it explains the functionality of a different package.

Suggested change

//

// It is sent over wire as a binary data.

//

// It is sent over wire as a binary data.

matthiasgeihs · 2021-12-14T14:52:17Z

wallet/sig.go

@@ -99,7 +116,8 @@ func DecodeSparseSigs(r io.Reader, sigs *[]Sig) (err error) {
 			if ((mask[maskIdx] >> bitIdx) % binaryModulo) == 0 {
 				(*sigs)[sigIdx] = nil
 			} else {
-				(*sigs)[sigIdx], err = DecodeSig(r)
+				(*sigs)[sigIdx] = NewSig()
+				err = perunio.Decode(r, (*sigs)[sigIdx])


So there is still a direct dependency from wallet onto perunio?

matthiasgeihs · 2021-12-14T14:54:31Z

wallet/test/wallet.go

@@ -111,15 +118,20 @@ func GenericSignatureSizeTest(t *testing.T, s *Setup) {
 	sign, err := acc.SignData(s.DataToSign)
 	require.NoError(t, err, "Sign with unlocked account should succeed")

+	signRaw, err := sign.MarshalBinary()


Suggested change

signRaw, err := sign.MarshalBinary()

sigRaw, err := sig.MarshalBinary()

matthiasgeihs · 2021-12-14T14:56:08Z

wallet/test/wallet.go

+	tamperer(&sigBytes)
+	return sigBytes
+}
+
 // GenericSignatureSizeTest tests that the size of the signatures produced by
 // Account.Sign(…) does not vary between executions (tested with 2048 samples).
 func GenericSignatureSizeTest(t *testing.T, s *Setup) {


I think we can throw that test out. Where do we specify that a signature byte representation must always have the same length?

matthiasgeihs · 2021-12-14T15:26:04Z

General concern regarding this change: #295 (comment)

matthiasgeihs · 2022-01-21T13:05:54Z

Closed. See #233 for more details.

Manoranjith added 7 commits December 14, 2021 16:03

♻️ [eth/channel] Update eth/channel to use the Sig interface

817acbd

Signed-off-by: Manoranjith <[email protected]>

♻️ [client] Update client pkg to use the Sig interface

0dfaf41

- (En|De)code signatures directly using perunio.(En|De)code. - SignData method on dummy account returns a Sig instead of byte array. Signed-off-by: Manoranjith <[email protected]>

♻️ [persistence/test] Use Sig.Clone for cloning a signature

bf56d1c

Signed-off-by: Manoranjith <[email protected]>

manoranjith requested a review from matthiasgeihs December 14, 2021 10:43

manoranjith force-pushed the 292-abstract-type-for-sig branch from 3915786 to 0ecc12a Compare December 14, 2021 11:25

manoranjith mentioned this pull request Dec 14, 2021

Move verify function from wallet.Backend to wallet.Sig #295

Closed

manoranjith force-pushed the 292-abstract-type-for-sig branch from 0ecc12a to be3a590 Compare December 14, 2021 12:39

matthiasgeihs reviewed Dec 14, 2021

View reviewed changes

matthiasgeihs closed this Jan 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define an abstract type for wallet.Sig #293

Define an abstract type for wallet.Sig #293

manoranjith commented Dec 14, 2021

matthiasgeihs Dec 14, 2021

matthiasgeihs Dec 14, 2021

matthiasgeihs Dec 14, 2021

matthiasgeihs Dec 14, 2021

matthiasgeihs Dec 14, 2021

matthiasgeihs Dec 14, 2021

matthiasgeihs Dec 14, 2021

matthiasgeihs Dec 14, 2021

matthiasgeihs Dec 14, 2021

matthiasgeihs Dec 14, 2021

matthiasgeihs commented Dec 14, 2021

matthiasgeihs commented Jan 21, 2022

	signRaw, err := sign.MarshalBinary()
	sigRaw, err := sig.MarshalBinary()

Define an abstract type for wallet.Sig #293

Define an abstract type for wallet.Sig #293

Conversation

manoranjith commented Dec 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasgeihs commented Dec 14, 2021

matthiasgeihs commented Jan 21, 2022