[multikey] Fix multikey serialization

CHANGELOG.md

@@ -4,18 +4,23 @@ All notable changes to the Aptos Go SDK will be captured in this file. This chan
 adheres to the format set out by [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 # Unreleased
+
+- [`Fix`][`Breaking`] Fix MultiKey implementation to be more consistent with the rest of the SDKs
 - Add BCS support for optional values
 
 # v1.1.0 (11/07/2024)
+
 - Add icon uri and project uri to FA client, reduce duplicated code
 - Add better error messages around script argument parsing
 - Add example for scripts with FA
 
 # v1.0.0 (10/28/2024)
+
 - [`Fix`] Paginate transactions properly if no given start version
 - Add account transactions API
 
 # v0.7.0 (8/19/2024)
+
 - [`Fix`] Parse GenesisTransaction properly
 - [`Fix`] Ensure if no block transactions are requested, it doesn't fail to fetch a block
 - [`Doc`] Fix comment from milliseconds to microseconds
@@ -24,6 +29,7 @@ adheres to the format set out by [Keep a Changelog](https://keepachangelog.com/e
 - [`Fix`] Fix MultiKey signature verification and building to work with any keys
 
 # v0.6.0 (6/28/2024)
+
 - [`Breaking`] Change type from Transaction to CommittedTransaction for cases that it's known they're committed
 - [`Fix`] Fix secp256k1 signing and verification to be correctly used
 - [`Fix`] Fix supply view function for FungibleAssetClient
@@ -42,11 +48,13 @@ adheres to the format set out by [Keep a Changelog](https://keepachangelog.com/e
 - Change signers to have simulation authenticators by default
 
 # v0.5.0 (6/21/2024)
+
 - [`Fix`] Fix sponsored transactions, and add example for sponsored transactions
 - Add examples to CI
 - Add node health check API to client
 
 # v0.4.1 (6/18/2024)
+
 - [`Fix`] Make examples more portable / not rely on internal packages
 
 # v0.4.0 (6/17/2024)

client_test.go

@@ -47,6 +47,10 @@ func initSigners() {
 		signer, err := NewMultiKeyTestSigner(3, 2)
 		return any(signer).(TransactionSigner), err
 	}
+	TestSigners["5-of-32 MultiKey"] = func() (TransactionSigner, error) {
+		signer, err := NewMultiKeyTestSigner(32, 5)
+		return any(signer).(TransactionSigner), err
+	}
 	/* TODO: MultiEd25519 is not supported ATM
 	TestSigners["MultiEd25519"] = func() (TransactionSigner, error) {
 		signer, err := NewMultiEd25519Signer(3, 2)

crypto/multiKey.go

@@ -349,33 +349,56 @@ func (ea *MultiKeyAuthenticator) UnmarshalBCS(des *bcs.Deserializer) {
 
 //region MultiKeyBitmap
 
-// MultiKeyBitmapSize represents the 4 bytes needed to make a 32-bit bitmap
-const MultiKeyBitmapSize = uint8(4)
+// MaxMultiKeySignatures is the maximum number supported here for ease of use
+const MaxMultiKeySignatures = uint8(32)
+const MaxMultiKeyBitmapBytes = MaxMultiKeySignatures / 8
 
 // MultiKeyBitmap represents a bitmap of signatures in a MultiKey public key that signed the transaction
-// There are a maximum of 32 possible values in MultiKeyBitmapSize, starting from the leftmost bit representing
+// There is a variable length to the possible signers, starting from the leftmost bit representing
 // index 0 of the public key
-type MultiKeyBitmap [MultiKeyBitmapSize]byte
+type MultiKeyBitmap struct {
+	inner []byte
+}
 
 // ContainsKey tells us if the current index is in the map
 func (bm *MultiKeyBitmap) ContainsKey(index uint8) bool {
+	if index >= MaxMultiKeySignatures {
+		return false
+	}
 	numByte, numBit := KeyIndices(index)
-	return (bm[numByte] & (128 >> numBit)) == 1
+
+	// If it's outside the map, then it's definitely false
+	if int(numByte) >= len(bm.inner) {
+		return false
+	}
+	return (bm.inner[numByte] & (128 >> numBit)) == 1
 }
 
 // AddKey adds the value to the map, returning an error if it is already added
 func (bm *MultiKeyBitmap) AddKey(index uint8) error {
+	if index >= MaxMultiKeySignatures {
+		return fmt.Errorf("index %d is greater than the maximum number of signatures %d", index, MaxMultiKeySignatures)
+	}
 	if bm.ContainsKey(index) {
 		return fmt.Errorf("index %d already in bitmap", index)
 	}
 	numByte, numBit := KeyIndices(index)
-	bm[numByte] = bm[numByte] | (128 >> numBit)
+
+	// Expand map if it needs to be extended
+	curLength := len(bm.inner)
+	if int(numByte) >= curLength {
+		newInner := make([]byte, numByte+1)
+		copy(newInner[0:curLength], bm.inner[:])
+		bm.inner = newInner
+	}
+
+	bm.inner[numByte] = bm.inner[numByte] | (128 >> numBit)
 	return nil
 }
 
 func (bm *MultiKeyBitmap) Indices() []uint8 {
 	indices := make([]uint8, 0)
-	for i := uint8(0); i < MultiKeyBitmapSize*8; i++ {
+	for i := uint8(0); i < MaxMultiKeySignatures; i++ {
 		if bm.ContainsKey(i) {
 			indices = append(indices, i)
 		}
@@ -396,7 +419,7 @@ func KeyIndices(index uint8) (numByte uint8, numBit uint8) {
 // Implements:
 //   - [bcs.Marshaler]
 func (bm *MultiKeyBitmap) MarshalBCS(ser *bcs.Serializer) {
-	ser.WriteBytes(bm[:])
+	ser.WriteBytes(bm.inner[:])
 }
 
 // UnmarshalBCS deserializes the bitmap from bytes
@@ -405,11 +428,12 @@ func (bm *MultiKeyBitmap) MarshalBCS(ser *bcs.Serializer) {
 //   - [bcs.Unmarshaler]
 func (bm *MultiKeyBitmap) UnmarshalBCS(des *bcs.Deserializer) {
 	length := des.Uleb128()
-	if length != uint32(MultiKeyBitmapSize) {
-		des.SetError(fmt.Errorf("MultiKeyBitmap must be %d bytes, got %d", MultiKeyBitmapSize, length))
+	if length > uint32(MaxMultiKeyBitmapBytes) {
+		des.SetError(fmt.Errorf("MultiKeyBitmap must be %d bytes or less, got %d", MaxMultiKeyBitmapBytes, length))
 		return
 	}
-	des.ReadFixedBytesInto(bm[:])
+	bm.inner = make([]byte, length)
+	des.ReadFixedBytesInto(bm.inner[:])
 }
 
 //endregion

crypto/multiKey_test.go

@@ -1,6 +1,7 @@
 package crypto
 
 import (
+	"encoding/hex"
 	"github.com/aptos-labs/aptos-go-sdk/bcs"
 	"github.com/stretchr/testify/assert"
 	"testing"
@@ -85,6 +86,18 @@ func TestMultiKeySerialization(t *testing.T) {
 
 }
 
+func TestMultiKey_Serialization_CrossPlatform(t *testing.T) {
+	serialized := "020140118d6ebe543aaf3a541453f98a5748ab5b9e3f96d781b8c0a43740af2b65c03529fdf62b7de7aad9150770e0994dc4e0714795fdebf312be66cd0550c607755e00401a90421453aa53fa5a7aa3dfe70d913823cbf087bf372a762219ccc824d3a0eeecccaa9d34f22db4366aec61fb6c204d2440f4ed288bc7cc7e407b766723a60901c0"
+	serializedBytes, err := hex.DecodeString(serialized)
+	assert.NoError(t, err)
+	signature := &MultiKeySignature{}
+	assert.NoError(t, bcs.Deserialize(signature, serializedBytes))
+
+	reserialized, err := bcs.Serialize(signature)
+	assert.NoError(t, err)
+	assert.Equal(t, serializedBytes, reserialized)
+}
+
 func createMultiKey(t *testing.T) (
 	*SingleSigner,
 	*SingleSigner,

signer_test.go

@@ -2,6 +2,7 @@ package aptos
 
 import (
 	"github.com/aptos-labs/aptos-go-sdk/crypto"
+	"math/rand/v2"
 )
 
 /* This is a collection of test signers, that don't make sense in the real world, but are used for testing */
@@ -91,15 +92,15 @@ type MultiKeyTestSigner struct {
 func NewMultiKeyTestSigner(numKeys uint8, signaturesRequired uint8) (*MultiKeyTestSigner, error) {
 	signers := make([]crypto.Signer, numKeys)
 	for i := range signers {
-		switch i % 3 {
+		switch i % 2 {
 		case 0:
 			signer, err := crypto.GenerateEd25519PrivateKey()
 			if err != nil {
 				return nil, err
 			}
 			// Wrap in a SingleSigner
 			signers[i] = &crypto.SingleSigner{Signer: signer}
-		case 1, 2:
+		case 1:
 			signer, err := crypto.GenerateSecp256k1Key()
 			if err != nil {
 				return nil, err
@@ -150,16 +151,25 @@ func (s *MultiKeyTestSigner) Sign(msg []byte) (authenticator *crypto.AccountAuth
 func (s *MultiKeyTestSigner) SignMessage(msg []byte) (crypto.Signature, error) {
 	indexedSigs := make([]crypto.IndexedAnySignature, s.SignaturesRequired)
 
-	// TODO: randomize keys used for testing
+	alreadyUsed := make(map[int]bool)
+
 	for i := uint8(0); i < s.SignaturesRequired; i++ {
-		// We skip over keys to ensure that order doesn't matter
-		signerIndex := (i * 2) % uint8(len(s.Signers))
+		// Find a random key
+		index := 0
+		for {
+			index = rand.IntN(len(s.Signers))
+			_, present := alreadyUsed[index]
+			if !present {
+				alreadyUsed[index] = true
+				break
+			}
+		}
 
-		sig, err := s.Signers[signerIndex].SignMessage(msg)
+		sig, err := s.Signers[index].SignMessage(msg)
 		if err != nil {
 			return nil, err
 		}
-		indexedSigs[i] = crypto.IndexedAnySignature{Signature: sig.(*crypto.AnySignature), Index: signerIndex}
+		indexedSigs[i] = crypto.IndexedAnySignature{Signature: sig.(*crypto.AnySignature), Index: uint8(index)}
 	}
 
 	return crypto.NewMultiKeySignature(indexedSigs)