Update tx submission logic to only fetch a single account key from the COA #911
Conversation
m-Peter
requested review from
janezpodhostnik,
peterargue and
zhangchiqing
as code owners
WalkthroughReplaces passing full Flow account objects with lightweight balances and AccountKey usage: Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SingleTxPool
participant FlowClient
participant KeyStore
rect rgba(240,250,255,0.9)
Note over SingleTxPool,KeyStore: Old flow (account-based signing)
Client->>SingleTxPool: enqueue(tx)
SingleTxPool->>FlowClient: GetLatestBlock(ctx) + GetAccount(COA)
FlowClient-->>SingleTxPool: block, account
SingleTxPool->>SingleTxPool: buildTransaction(block, account, ...)
SingleTxPool->>KeyStore: SetProposerPayerAndSign(tx, account)
KeyStore-->>SingleTxPool: signed tx
end
rect rgba(255,245,240,0.9)
Note over SingleTxPool,KeyStore: New flow (address + AccountKey)
Client->>SingleTxPool: enqueue(tx)
SingleTxPool->>FlowClient: GetLatestBlock(ctx, true)
FlowClient-->>SingleTxPool: block
SingleTxPool->>FlowClient: GetAccountKeyAtLatestBlock(ctx, coaAddress, index)
FlowClient-->>SingleTxPool: accountKey
SingleTxPool->>SingleTxPool: buildTransaction(ctx, block, ...)
SingleTxPool->>KeyStore: SetProposerPayerAndSign(tx, coaAddress, accountKey)
KeyStore-->>SingleTxPool: signed tx
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-03-07T01:35:09.751ZApplied to files:
📚 Learning: 2025-01-24T20:15:10.908ZApplied to files:
📚 Learning: 2025-06-17T10:29:35.941ZApplied to files:
🧬 Code graph analysis (1)services/requester/single_tx_pool.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (14)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
m-Peter
force-pushed
the
mpeter/tx-submission-fetch-single-account-key
branch
2 times, most recently
from
2490549 to
ed88325
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
services/requester/keystore/key_store_test.go (1)
203-203: Signature update LGTM; consider stronger asserts on envelope signatureCall site matches the new API. Optionally assert signer details and a single envelope sig to harden the test.
- assert.NotEmpty(t, tx.EnvelopeSignatures) + if assert.Len(t, tx.EnvelopeSignatures, 1) { + sig := tx.EnvelopeSignatures[0] + assert.Equal(t, account.Address, sig.Address) + assert.Equal(t, account.Keys[0].Index, sig.KeyIndex) + }services/requester/batch_tx_pool.go (2)
219-227: Release signing key on send failure to avoid temporary starvationIf SendTransaction fails, the key remains locked until expiry. Notify the keystore immediately and count drops.
if err := t.client.SendTransaction(ctx, *flowTx); err != nil { - return err + // Proactively release the key associated with this Flow tx. + t.keystore.NotifyTransaction(flowTx.ID()) + t.collector.TransactionsDropped(len(hexEncodedTxs)) + return err }
245-263: Do the same early-release for single-submit pathMirror the early-release on error for single transactions.
if err := t.client.SendTransaction(ctx, *flowTx); err != nil { - return err + t.keystore.NotifyTransaction(flowTx.ID()) + t.collector.TransactionsDropped(1) + return err }services/requester/single_tx_pool.go (1)
159-199: Ensure key is released on send failure (call site change in Add)buildTransaction locks the key and records metadata. If SendTransaction fails, release immediately using NotifyTransaction to avoid waiting for expiry.
if err := t.client.SendTransaction(ctx, *flowTx); err != nil { - return err + t.keystore.NotifyTransaction(flowTx.ID()) + return err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
services/requester/batch_tx_pool.go(4 hunks)services/requester/keystore/account_key.go(1 hunks)services/requester/keystore/key_store_test.go(1 hunks)services/requester/single_tx_pool.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
services/requester/single_tx_pool.go (1)
services/requester/keystore/account_key.go (1)
AccountKey(11-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (3)
services/requester/single_tx_pool.go (3)
95-107: Latest block via context LGTMFetching the sealed latest block with ctx aligns with the new flow and reduces unnecessary COA calls.
159-163: Verified: all call sites successfully updated to new signatures✓ SetProposerPayerAndSign: Both call sites (single_tx_pool.go:190, key_store_test.go:203) use the 3-argument form
✓ buildTransaction: All three call sites (batch_tx_pool.go:220, batch_tx_pool.go:256, single_tx_pool.go:101) use the new signature with ctx and latestBlock
✓ No legacy call patterns remain
201-208: Verify intentional mutex design with maintainers before removalThe review comment suggests removing the outer mutex because
keystore.Take()already enforces per-key exclusivity. However, the signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, with key release happening asynchronously via event_subscriber.go, and this pattern ensures transaction integrity by preventing key reuse before transaction finalization.The current implementation serializes all transaction building operations (not just key acquisition), while the keystore itself manages per-key locking through its channel-based mechanism. If multiple signing keys exist, removing the outer mutex would enable concurrent transaction building with different keys. However, the retrieved learning suggests this serialization may be intentional for maintaining transaction integrity guarantees beyond simple per-key exclusivity.
Before removing the mutex, verify:
- Whether the serialization serves a purpose beyond per-key locking (e.g., ordering, state consistency)
- The impact of allowing concurrent transaction builds when multiple keys exist
- Whether the async key release mechanism remains sound under concurrent access patterns
| // now that the transaction is prepared, store the transaction's metadata | ||
| accKey.SetLockMetadata(flowTx.ID(), latestBlock.Height) | ||
|
|
||
| t.collector.OperatorBalance(account) |
There was a problem hiding this comment.
I have removed the OperatorBalance metric, because now we use GetAccountKeyAtLatestBlock, and we don't have access to the COA balance.
I am planning to add this back, on this PR: #896. Generally, we don't have to emit this metric on every tx submission, we can do it at a less frequent rate.
| hexEncodedTx cadence.String, | ||
| ) error { | ||
| latestBlock, account, err := t.fetchFlowLatestBlockAndCOA(ctx) | ||
| latestBlock, err := t.client.GetLatestBlock(ctx, true) |
There was a problem hiding this comment.
Maybe we could throttle the calls to GetLatestBlock, given the latest block only changes about once per second for mainnet, and we are only using the block ID as reference block ID, it's ok to have a little bit delay. This could further reduce the load to AN.
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
| hexEncodedTx cadence.String, | ||
| ) error { | ||
| latestBlock, account, err := t.fetchFlowLatestBlockAndCOA(ctx) | ||
| latestBlock, err := t.client.GetLatestBlock(ctx, true) |
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.
services/requester/single_tx_pool.go
Outdated
| // building and signing transactions should be | ||
| // blocking, so we don't have conflicts with keys. |
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 building the transaction part is quite straightforward, and it doesn't need to be protected by the lock.
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 signing the transaction part required taking an account key from the keystore and then signing the transaction with it:
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 accKey.SetProposerPayerAndSign(...), which doesn't seem to be needing any lock protection.
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 fetchSigningAccountKey(), because I didn't want the added AN call:
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.
m-Peter
force-pushed
the
mpeter/tx-submission-fetch-single-account-key
branch
from
f030b4c to
1c781f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
services/requester/single_tx_pool.go (1)
201-208: Consider clarifying the comment about mutex usage.The comment on lines 202-203 states "blocking, so we don't have conflicts with keys," but this is slightly misleading. Since
keystore.Take()uses a channel internally, it already provides synchronization to prevent multiple goroutines from taking the same key. The mutex here primarily ensures consistency of the available keys metric (line 165) rather than preventing key conflicts.Consider updating the comment to be more precise:
- // building and signing transactions should be - // blocking, so we don't have conflicts with keys. + // Mutex ensures consistent metric reporting for available keys + // across concurrent transaction builds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
bootstrap/bootstrap.go(2 hunks)metrics/collector.go(2 hunks)metrics/nop.go(1 hunks)services/requester/batch_tx_pool.go(4 hunks)services/requester/keystore/account_key.go(1 hunks)services/requester/keystore/key_store_test.go(1 hunks)services/requester/requester.go(2 hunks)services/requester/single_tx_pool.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/requester/keystore/key_store_test.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-17T10:29:35.941Z
Learnt from: m-Peter
Repo: onflow/flow-evm-gateway PR: 831
File: services/requester/pool.go:202-214
Timestamp: 2025-06-17T10:29:35.941Z
Learning: In Flow EVM Gateway's transaction pool (services/requester/pool.go), signing keys acquired with keystore.Take() are intentionally held until the transaction is sealed, not released immediately after sending. The key release happens asynchronously when sealed transactions are ingested via event_subscriber.go NotifyTransaction() -> keystore unsafeUnlockKey() -> key.Done(). This pattern ensures transaction integrity by preventing key reuse before transaction finalization.
Applied to files:
services/requester/keystore/account_key.goservices/requester/single_tx_pool.go
📚 Learning: 2025-03-07T01:35:09.751Z
Learnt from: peterargue
Repo: onflow/flow-evm-gateway PR: 772
File: services/requester/keystore/key_store.go:50-62
Timestamp: 2025-03-07T01:35:09.751Z
Learning: In the flow-evm-gateway codebase, panics are acceptable in scenarios where immediate detection of critical bugs is desired during development and testing, particularly for invariant violations that should never occur in a correctly functioning system (e.g., when a key is available but locked in the keystore implementation).
Applied to files:
services/requester/keystore/account_key.goservices/requester/single_tx_pool.go
📚 Learning: 2025-01-24T20:15:10.908Z
Learnt from: peterargue
Repo: onflow/flow-evm-gateway PR: 682
File: services/ingestion/event_subscriber.go:85-90
Timestamp: 2025-01-24T20:15:10.908Z
Learning: When calculating blocks to catch up in Flow, initialize the counter to 0 and only set it when the latest chain height is greater than the current height (latestOnChainHeight > currentHeight). There's no need to handle the else case as 0 blocks to catch up is the correct default when we're at the same height or ahead.
Applied to files:
services/requester/keystore/account_key.go
🧬 Code graph analysis (1)
services/requester/single_tx_pool.go (1)
services/requester/keystore/account_key.go (1)
AccountKey(11-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (5)
services/requester/batch_tx_pool.go (3)
164-170: LGTM: Latest block retrieval simplified.The change to fetch the latest block directly without COA context is correct and aligns with the PR objectives. The error message accurately reflects the new approach.
Note: Past reviewers suggested throttling
GetLatestBlockcalls for performance, which is being addressed in PR #896.
198-226: LGTM: Function signature updated correctly.The removal of the
accountparameter and addition ofctxto thebuildTransactioncall are consistent with the new signing flow. The transaction building logic remains sound.
241-274: LGTM: Single transaction submission updated correctly.The changes to fetch the latest block directly and pass
ctxtobuildTransactionare consistent with the batch submission path. The implementation is correct.services/requester/single_tx_pool.go (2)
95-98: LGTM: Latest block retrieval updated correctly.The change to fetch the latest block directly aligns with the PR objectives and is consistent with the batch pool implementation.
101-107: LGTM: Context correctly propagated to buildTransaction.The addition of the
ctxparameter is appropriate and matches the updated function signature.
m-Peter
force-pushed
the
mpeter/tx-submission-fetch-single-account-key
branch
from
5cf48e3 to
253d859
Compare
Work Towards: #773
Description
Instead of fetching the entire account info, such as balances and all of its account keys, we can simply fetch only the account key we are going to be using for signing the next Cadence transaction.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Refactor
Chores