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
WalkthroughRefactors remove reliance on full Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SingleTxPool
participant FlowClient
participant KeyStore
rect rgb(240,250,255)
Note over SingleTxPool,KeyStore: Old flow (used full flow.Account)
Client->>SingleTxPool: Add(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 rgb(255,245,240)
Note over SingleTxPool,KeyStore: New flow (address + AccountKey)
Client->>SingleTxPool: Add(tx)
SingleTxPool->>FlowClient: GetLatestBlock(ctx, true)
FlowClient-->>SingleTxPool: block
SingleTxPool->>FlowClient: GetAccountKey(coaAddress, block)
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)
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
from
616f2b7 to
2490549
Compare
m-Peter
force-pushed
the
mpeter/tx-submission-fetch-single-account-key
branch
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.
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
Refactor
Chores