zksync-sdk / zksync2-go

zksync2-go is a geth library adapted to work with the zkSync Era.
Apache License 2.0
87 stars 36 forks source link

L2Wallet SendTransaction is not thread safe #42

Open Meteriox opened 3 months ago

Meteriox commented 3 months ago

I found that when multiple goroutines concurrently call SendTransaction to send transactions, it's possible to get the same txhash for different transactions. This can lead to some transactions not being confirmed, and calling WaitMined after sending the transaction can't retrieve the transaction information, resulting in a deadlock. The code to reproduce this issue is as follows: `func main() { PrivateKey := "" ZkSyncEraProvider := ""

client, err := clients.Dial(ZkSyncEraProvider)
if err != nil {
    return
}
defer client.Close()

wallet, err := accounts.NewWalletL2(common.Hex2Bytes(PrivateKey), &client)
if err != nil {
    return
}

SatsAddress := common.HexToAddress("***")
ZBTCAddress := common.HexToAddress("***")
l2address := common.HexToAddress("***")

contractABI, err := abi.JSON(strings.NewReader(erc20ABI))
if err != nil {
    log.Fatal(err)
}

var wg sync.WaitGroup
wg.Add(3) 

go func() {
    defer wg.Done()
    amount := ToWei("1", 18)
    mintData, err := contractABI.Pack("mint", l2address, amount)
    if err != nil {
        return
    }
    hash, err := wallet.SendTransaction(context.Background(), &accounts.Transaction{
        To:   &SatsAddress,
        Data: mintData,
    })
    if err != nil {
        fmt.Println(err)
        return
    }
    _, err = client.WaitMined(context.Background(), hash)
    if err != nil {
        log.Panic(err)
    }
}()

go func() {
    defer wg.Done()
    amount := ToWei("0.001", 18)
    mintData, err := contractABI.Pack("mint", l2address, amount)
    if err != nil {
        return
    }
    hash, err := wallet.SendTransaction(context.Background(), &accounts.Transaction{
        To:   &ZBTCAddress,
        Data: mintData,
    })
    if err != nil {
        fmt.Println(err)
        return
    }
    _, err = client.WaitMined(context.Background(), hash)
    if err != nil {
        log.Panic(err)
    }
}()

go func() {
    defer wg.Done()
    amount := ToWei("0.001", 18)
    mintData, err := contractABI.Pack("mint", l2address, amount)
    if err != nil {
        return
    }
    hash, err := wallet.SendTransaction(context.Background(), &accounts.Transaction{
        To:   &ZBTCAddress,
        Data: mintData,
    })
    if err != nil {
        fmt.Println(err)
        return
    }
    _, err = client.WaitMined(context.Background(), hash)
    if err != nil {
        log.Panic(err)
    }
}()
wg.Wait()

}`

However, after adding a lock to SendTransaction, a new problem arose. Since the Nonce calculation in PopulateTransaction is based on an RPC call rather than local caching, concurrent calls to SendTransaction can lead to the Nonce not being updated in time. This can result in transactions with the same Nonce field being generated. When the previous transaction is submitted and successfully processed, the subsequent transaction will be rejected due to an invalid Nonce field.

go func() { defer wg.Done() amount := ToWei("1", 18) mintData, err := contractABI.Pack("mint", l2address, amount) if err != nil { return } lock.Lock() hash, err := wallet.SendTransaction(context.Background(), &accounts.Transaction{ To: &SatsAddress, Data: mintData, }) if err != nil { fmt.Println(err) return } lock.Unlock() _, err = client.WaitMined(context.Background(), hash) if err != nil { log.Panic(err) } }()

In the end, I had to include WaitMined in the locked scope as well, forcing the originally concurrent transactions to be executed and confirmed in a completely serial manner.

go func() { defer wg.Done() amount := ToWei("1", 18) mintData, err := contractABI.Pack("mint", l2address, amount) if err != nil { return } lock.Lock() hash, err := wallet.SendTransaction(context.Background(), &accounts.Transaction{ To: &SatsAddress, Data: mintData, }) if err != nil { fmt.Println(err) return } _, err = client.WaitMined(context.Background(), hash) if err != nil { log.Panic(err) } lock.Unlock() }()

My question is, when using zksync-gosdk, is it only possible to execute transactions serially, and not to send transactions concurrently? If concurrent transactions are possible, how can it be done? If not, please add a note in the comments of SendTransaction indicating that it is not thread-safe for concurrent use.

danijelTxFusion commented 3 months ago

Method WalletL2.sendTransaction is not implemented to be thread-safe. If there is a need by the community to make it thread-safe, then it will be implemented.

Meteriox commented 3 months ago

Method WalletL2.sendTransaction is not implemented to be thread-safe. If there is a need by the community to make it thread-safe, then it will be implemented.

If my requirement now is to support concurrency safety when calling a contract to send transactions, which interface or function should I use to implement it? Or should I directly use the rpc method of the node to implement it?

danijelTxFusion commented 3 months ago

If my requirement now is to support concurrency safety when calling a contract to send transactions, which interface or function should I use to implement it? Or should I directly use the rpc method of the node to implement it?

You can call directlyeth_sendTransaction RPC method. Here are the steps:

client, err := clients.Dial("https://sepolia.era.zksync.dev")
if err != nil {
    log.Panic(err)
}
defer client.Close()

chainID, err := client.ChainID(context.Background())
if err != nil {
    log.Panic(err)
}
baseSigner, err := NewBaseSignerFromRawPrivateKey(rawPrivateKey, chainID.Int64())
if err != nil {
    log.Panic(err)
}
signer := Signer(baseSigner)

var preparedTx zkTypes.Transaction712 // prepared tx in thread-safe manner

signature, err := signer.SignTypedData(signer.Domain(), preparedTx)
if err != nil {
    log.Panic(err)
}
tx, err := preparedTx.RLPValues(signature)
if err != nil {
    log.Panic(err)
}
hahs, err := client.SendRawTransaction(ctx context.Context, tx)
if err != nil {
    log.Panic(err)
}
Meteriox commented 2 months ago

If my requirement now is to support concurrency safety when calling a contract to send transactions, which interface or function should I use to implement it? Or should I directly use the rpc method of the node to implement it?

You can call directlyeth_sendTransaction RPC method. Here are the steps:

  • Prepare your transaction manually with correct nonce (similar like this)
  • Signs the prepared transaction using Signer.
  • Send signed transaction using Client.sendRawTransaction.
client, err := clients.Dial("https://sepolia.era.zksync.dev")
if err != nil {
  log.Panic(err)
}
defer client.Close()

chainID, err := client.ChainID(context.Background())
if err != nil {
    log.Panic(err)
}
baseSigner, err := NewBaseSignerFromRawPrivateKey(rawPrivateKey, chainID.Int64())
if err != nil {
    log.Panic(err)
}
signer := Signer(baseSigner)

var preparedTx zkTypes.Transaction712 // prepared tx in thread-safe manner

signature, err := signer.SignTypedData(signer.Domain(), preparedTx)
if err != nil {
  log.Panic(err)
}
tx, err := preparedTx.RLPValues(signature)
if err != nil {
  log.Panic(err)
}
hahs, err := client.SendRawTransaction(ctx context.Context, tx)
if err != nil {
  log.Panic(err)
}

i have tried the above mentioned method to send raw transaction concurrently, but i have found that i need to mange tx.nonce locally which is difficult for me, so please implement the wallet sendtx method to be thread safety,thks!