Update tx submission logic to only fetch a single account key from the COA #911
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -161,10 +161,10 @@ func (t *BatchTxPool) processPooledTransactions(ctx context.Context) { | |
| case <-ctx.Done(): | ||
| return | ||
| case <-ticker.C: | ||
| latestBlock, err := t.client.GetLatestBlock(ctx, true) | ||
| if err != nil { | ||
| t.logger.Error().Err(err).Msg( | ||
| "failed to get latest Flow block on batch tx submission", | ||
| ) | ||
| continue | ||
| } | ||
|
|
@@ -181,7 +181,6 @@ func (t *BatchTxPool) processPooledTransactions(ctx context.Context) { | |
| err := t.batchSubmitTransactionsForSameAddress( | ||
| ctx, | ||
| latestBlock, | ||
| pooledTxs, | ||
| ) | ||
| if err != nil { | ||
|
|
@@ -199,7 +198,6 @@ func (t *BatchTxPool) processPooledTransactions(ctx context.Context) { | |
| func (t *BatchTxPool) batchSubmitTransactionsForSameAddress( | ||
| ctx context.Context, | ||
| latestBlock *flow.Block, | ||
| pooledTxs []pooledEvmTx, | ||
| ) error { | ||
| // Sort the transactions based on their nonce, to make sure | ||
|
|
@@ -220,8 +218,8 @@ func (t *BatchTxPool) batchSubmitTransactionsForSameAddress( | |
|
|
||
| script := replaceAddresses(runTxScript, t.config.FlowNetworkID) | ||
| flowTx, err := t.buildTransaction( | ||
| ctx, | ||
| latestBlock, | ||
| script, | ||
| cadence.NewArray(hexEncodedTxs), | ||
| coinbaseAddress, | ||
|
|
@@ -244,7 +242,7 @@ func (t *BatchTxPool) submitSingleTransaction( | |
| ctx context.Context, | ||
| hexEncodedTx cadence.String, | ||
| ) error { | ||
| latestBlock, err := t.client.GetLatestBlock(ctx, true) | ||
|
There was a problem hiding this comment. Maybe we could throttle the calls to There was a problem hiding this comment. Agreed, there should be a component that is responsible for querying (or being subscribed to) the last block, which we then use here. I think that might be best to put in a separate PR though. There was a problem hiding this comment. I have already opened a separate PR for that: #896 |
||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
@@ -256,8 +254,8 @@ func (t *BatchTxPool) submitSingleTransaction( | |
|
|
||
| script := replaceAddresses(runTxScript, t.config.FlowNetworkID) | ||
| flowTx, err := t.buildTransaction( | ||
| ctx, | ||
| latestBlock, | ||
| script, | ||
| cadence.NewArray([]cadence.Value{hexEncodedTx}), | ||
| coinbaseAddress, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,6 @@ import ( | |
| flowGo "github.com/onflow/flow-go/model/flow" | ||
| "github.com/rs/zerolog" | ||
| "github.com/sethvargo/go-retry" | ||
|
|
||
| "github.com/onflow/flow-evm-gateway/config" | ||
| "github.com/onflow/flow-evm-gateway/metrics" | ||
|
|
@@ -93,15 +92,15 @@ func (t *SingleTxPool) Add( | |
| return err | ||
| } | ||
|
|
||
| latestBlock, err := t.client.GetLatestBlock(ctx, true) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| script := replaceAddresses(runTxScript, t.config.FlowNetworkID) | ||
| flowTx, err := t.buildTransaction( | ||
| ctx, | ||
| latestBlock, | ||
| script, | ||
| cadence.NewArray([]cadence.Value{hexEncodedTx}), | ||
| coinbaseAddress, | ||
|
|
@@ -157,8 +156,8 @@ func (t *SingleTxPool) Add( | |
| // buildTransaction creates a Cadence transaction from the provided script, | ||
| // with the given arguments and signs it with the configured COA account. | ||
| func (t *SingleTxPool) buildTransaction( | ||
| ctx context.Context, | ||
| latestBlock *flow.Block, | ||
| script []byte, | ||
| args ...cadence.Value, | ||
| ) (*flow.Transaction, error) { | ||
|
|
@@ -177,53 +176,33 @@ func (t *SingleTxPool) buildTransaction( | |
| } | ||
| } | ||
|
|
||
| accKey, err := t.fetchSigningAccountKey() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| coaAddress := t.config.COAAddress | ||
| accountKey, err := t.client.GetAccountKeyAtLatestBlock(ctx, coaAddress, accKey.Index) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if err := accKey.SetProposerPayerAndSign(flowTx, coaAddress, accountKey); err != nil { | ||
| accKey.Done() | ||
| return nil, err | ||
| } | ||
|
|
||
| // now that the transaction is prepared, store the transaction's metadata | ||
| accKey.SetLockMetadata(flowTx.ID(), latestBlock.Height) | ||
|
|
||
|
There was a problem hiding this comment. I have removed the |
||
| return flowTx, nil | ||
|
Comment on lines
+179
to
198
There was a problem hiding this comment. Critical: Resource leak when GetAccountKeyAtLatestBlock fails. The signing key obtained from Apply this diff to fix the resource leak: accKey, err := t.fetchSigningAccountKey()
if err != nil {
return nil, err
}
coaAddress := t.config.COAAddress
accountKey, err := t.client.GetAccountKeyAtLatestBlock(ctx, coaAddress, accKey.Index)
if err != nil {
+ accKey.Done()
return nil, err
}🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| func (t *SingleTxPool) fetchSigningAccountKey() (*keystore.AccountKey, error) { | ||
| // building and signing transactions should be | ||
| // blocking, so we don't have conflicts with keys. | ||
|
Comment on lines
+202
to
+203
There was a problem hiding this comment. this doesn't seem accurate anymore since the lock only protects getting the key. should the building/signing also be atomic? There was a problem hiding this comment. Hey @peterargue , good flag 👍 . I'll try to reason through this change, as I don't really remember why this comment was there in the first place 😅 The code previously looked like this: // building and signing transactions should be blocking,
// so we don't have keys conflict
t.mux.Lock()
defer t.mux.Unlock()
accKey, err := t.keystore.Take()
if err != nil {
return nil, err
}
if err := accKey.SetProposerPayerAndSign(flowTx, account); err != nil {
accKey.Done()
return nil, err
}
// now that the transaction is prepared, store the transaction's metadata
accKey.SetLockMetadata(flowTx.ID(), latestBlock.Height)
t.collector.OperatorBalance(account)
return flowTx, nilThe flowTx := flow.NewTransaction().
SetScript(script).
SetReferenceBlockID(latestBlock.ID).
SetComputeLimit(flowGo.DefaultMaxTransactionGasLimit)
for _, arg := range args {
if err := flowTx.AddArgument(arg); err != nil {
return nil, fmt.Errorf("failed to add argument: %s, with %w", arg, err)
}
}The t.mux.Lock()
defer t.mux.Unlock()
accKey, err := t.keystore.Take()
if err != nil {
return nil, err
}
if err := accKey.SetProposerPayerAndSign(flowTx, account); err != nil {
accKey.Done()
return nil, err
}So the lock acquisition was there to only prevent that no two go-routines would end up taking the same key, and operating on it. As soon as an account key is taken, the signing is done with What's more, I am not even sure if taking an account key from the keystore does in fact need any lock protection. select {
case key := <-k.availableKeys:
if !key.lock() {
// this should never happen and means there's a bug
panic(fmt.Sprintf("key %d available, but locked", key.Index))
}
return key, nil
default:
return nil, ErrNoKeysAvailable
}Given that the keystore keeps the available account keys in a channel, is it even possible for two go-routines to end up taking the same key? 🤔 To conclude, I created the coaAddress := t.config.COAAddress
accountKey, err := t.client.GetAccountKeyAtLatestBlock(ctx, coaAddress, accKey.Index)
if err != nil {
return nil, err
}to be performed with the lock acquired. This would create other requests to block. |
||
| t.mux.Lock() | ||
| defer t.mux.Unlock() | ||
|
|
||
| return t.keystore.Take() | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid zeroing operator_balance on fetch errors.
When
GetAccountBalanceAtLatestBlockfails,accBalanceremains the zero value (0), yet we still push it into the metric. That will falsely drop theoperator_balancegauge to 0 and trigger alarms even though the real balance is unchanged. Bail out on the error instead.📝 Committable suggestion
🤖 Prompt for AI Agents