Inconsistent Signature Order in PartialSign Function

VincentDebug commented 5 months ago

Description

In the current implementation of the PartialSign function in transaction.go, the order of signatures does not consistently correspond with the order of signer keys. This could potentially lead to issues where signatures are incorrectly mapped to their respective public keys, especially in transactions involving multiple signers.

Steps to Reproduce

Create a Transaction with multiple signers.
Call PartialSign for each signer in an order that does not match the order of signerKeys.
Observe that the signatures array does not maintain the order of signerKeys.

Expected Behavior

The signatures in the Transaction.Signatures should correspond exactly in order to the signerKeys array regardless of the order in which PartialSign is called.

Actual Behavior

The signatures are appended in the order that PartialSign is called, which may not match the order of signerKeys.

Possible Solution

Modify the PartialSign method to ensure that signatures are placed in the correct positions within the Transaction.Signatures array according to their corresponding signerKeys.

Additional Information

I have implemented a temporary workaround that involves manually rearranging the signatures array after each call to PartialSign. However, a more robust solution within the PartialSign function would be preferable to prevent potential errors and improve code reliability.

Environment

Library Version: v1.10.0
Go Version: go version go1.22.4 darwin/amd64
OS: macOS Sonoma

I am happy to provide further details or assist in testing any potential fixes.

VincentDebug commented 5 months ago

Test Case to Reproduce Issue

Here's a simple test code that demonstrates the issue with the PartialSign function where the order of signatures may not correspond to the order of signer keys.

func TestPartialSignTransaction(t *testing.T) {
    signers := []PrivateKey{
        NewWallet().PrivateKey,
        NewWallet().PrivateKey,
    }
    instructions := []Instruction{
        &testTransactionInstructions{
            accounts: []*AccountMeta{
                {PublicKey: signers[0].PublicKey(), IsSigner: true, IsWritable: false},
                {PublicKey: signers[1].PublicKey(), IsSigner: true, IsWritable: true},
            },
            data:      []byte{0xaa, 0xbb},
            programID: MustPublicKeyFromBase58("11111111111111111111111111111111"),
        },
    }

    blockhash, err := HashFromBase58("A9QnpgfhCkmiBSjgBuWk76Wo3HxzxvDopUq9x6UUMmjn")
    require.NoError(t, err)

    trx, err := NewTransaction(instructions, blockhash)
    require.NoError(t, err)

    assert.Equal(t, trx.Message.Header.NumRequiredSignatures, uint8(2))

    signatures, err := trx.PartialSign(func(key PublicKey) *PrivateKey {
        if key.Equals(signers[1].PublicKey()) {
            return &signers[1]
        }
        return nil
    })
    require.NoError(t, err)
    assert.Equal(t, len(signatures), 1)

    // full sign
    signatures, err = trx.PartialSign(func(key PublicKey) *PrivateKey {
        if key.Equals(signers[0].PublicKey()) {
            return &signers[0]
        }
        return nil
    })
    require.NoError(t, err)
    assert.Equal(t, len(signatures), 2)
    require.NoError(t, trx.VerifySignatures())
}

Log result

=== RUN   TestPartialSignTransaction
    transaction_test.go:152: 
            Error Trace:    transaction_test.go:152
            Error:          Received unexpected error:
                            invalid signature by Gw79QEFJ536Z1zFFHN644fqbzhLJ9fy5jHaE2B3QwENy
            Test:           TestPartialSignTransaction
--- FAIL: TestPartialSignTransaction (0.00s)

FAIL

Test Code Based on https://github.com/gagliardetto/solana-go/blob/a5e17d6253468337b4851105413f827a8b152f5a/transaction_test.go#L110-L134

VincentDebug commented 5 months ago

Temporary Workaround for this Issue

func PartialSignTransaction(tx *solana.Transaction, signer *solana.PrivateKey) (*solana.Signature, error) {
    // Ensure that the transaction has the correct number of signatures initialized
    numRequiredSignatures := tx.Message.Header.NumRequiredSignatures
    if len(tx.Signatures) == 0 {
        tx.Signatures = make([]solana.Signature, numRequiredSignatures)
    } else if len(tx.Signatures) != int(numRequiredSignatures) {
        return nil, fmt.Errorf("invalid signatures length, expected %d, actual %d", numRequiredSignatures, len(tx.Signatures))
    }

    fullIndex := -1  // This will track the index of the signer in the message's accounts
    signerIndex := 0 // This is the expected index position of the signer's signature in the transaction

    // Perform the partial signing only with the provided private key
    _, err := tx.PartialSign(func(key solana.PublicKey) *solana.PrivateKey {
        fullIndex++
        if signer.PublicKey().Equals(key) {
            signerIndex = fullIndex
            return signer
        }
        return nil
    })
    if err != nil {
        return nil, err
    }

    // Move the last signature (the one just added) to the correct position if needed
    if signerIndex != len(tx.Signatures)-1 {
        tx.Signatures[signerIndex] = tx.Signatures[len(tx.Signatures)-1]
        tx.Signatures = tx.Signatures[:len(tx.Signatures)-1]
    }

    return &tx.Signatures[signerIndex], nil
}

This code snippet ensures each signature is indexed correctly according to the signer's position, despite the order in which signatures are requested or provided.

VincentDebug commented 5 months ago

I have submitted a PR.

🔗 View Pull Request #222

I welcome all maintainers and contributors to review the changes.

VincentDebug commented 5 months ago

gagliardetto / solana-go