[Access] Fix executor address matching in scheduled tx indexer#8539
[Access] Fix executor address matching in scheduled tx indexer#8539peterargue wants to merge 3 commits intomasterfrom
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
📝 WalkthroughWalkthroughThe changes implement height-aware executor address mapping for scheduled transactions. The executor address transitions from a fixed field to a version-dependent structure using the chain height versioning system. Transaction identification now requires height context, and error handling is relaxed for executor transactions with missing scheduled-transaction arguments. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
peterargue
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
module/state_synchronization/indexer/extended/scheduled_transactions_test.go (1)
783-811: Minor: test name is slightly misleading now.
TestScheduledTransactionsIndexer_ExecutorTxNoArgumentsno longer exercises an executor tx per se — after the fix, a zero-argumentFlowServiceAccount-authorized tx is explicitly not considered an executor tx. The test correctly validates that behavior by asserting the "have no corresponding executor transaction" error, but the name implies the tx is still an executor tx with missing args.Consider renaming to something like
TestScheduledTransactionsIndexer_SystemTxWithNoArgumentsNotMatchedto reflect the actual intent (this is primarily about distinguishing v0ProcessCallbacksTransaction/SystemChunkTransactionfrom the executor tx).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@module/state_synchronization/indexer/extended/scheduled_transactions_test.go` around lines 783 - 811, The test name TestScheduledTransactionsIndexer_ExecutorTxNoArguments is misleading because it verifies that a zero-argument FlowServiceAccount-authorized system transaction is NOT treated as an executor tx; rename the test function to a clearer name (e.g., TestScheduledTransactionsIndexer_SystemTxWithNoArgumentsNotMatched) and update any references/imports/mentions to that symbol so the test declaration, any calls, and logging reflect the new name while leaving the body (creating noArgTx, pendingEvt, and the assertions) unchanged.module/state_synchronization/indexer/extended/scheduled_transactions.go (2)
125-129: Double-check the 3-entry version map is intentional (and keep the assertion working).Including
access.VersionLatestalongsideVersion0/Version1is required here because the fallbackaccess.NewStaticHeightVersionMapper(access.LatestBoundary)only definesVersionLatest, andaccess.NewVersionedpanics if any version returned by the mapper is missing from the map (seemodel/access/versioned.golines 24-29). Worth a brief inline comment so the extraVersionLatestentry isn't "cleaned up" as a duplicate ofVersion1by a future refactor.💡 Proposed note
executorAddr := access.NewVersioned(map[access.Version]flow.Address{ - systemcollection.Version0: sc.FlowServiceAccount.Address, - systemcollection.Version1: sc.ScheduledTransactionExecutor.Address, - access.VersionLatest: sc.ScheduledTransactionExecutor.Address, + systemcollection.Version0: sc.FlowServiceAccount.Address, + systemcollection.Version1: sc.ScheduledTransactionExecutor.Address, + // VersionLatest is required for chains without an explicit mapping + // (fallback mapper emits only VersionLatest). + access.VersionLatest: sc.ScheduledTransactionExecutor.Address, }, versionMapper)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@module/state_synchronization/indexer/extended/scheduled_transactions.go` around lines 125 - 129, The version map passed to access.NewVersioned for executorAddr intentionally includes three keys (systemcollection.Version0, systemcollection.Version1, access.VersionLatest) because the fallback mapper (versionMapper created via access.NewStaticHeightVersionMapper(access.LatestBoundary)) can return access.VersionLatest and access.NewVersioned will panic if a returned version is missing; add a brief inline comment next to the executorAddr/access.NewVersioned call explaining that the extra access.VersionLatest entry is required to satisfy the fallback mapper and must not be removed as a duplicate of Version1 to keep the existing assertion behavior intact.
119-129: Optional: consider centralizing the height-version fallback.The
if !ok { versionMapper = access.NewStaticHeightVersionMapper(access.LatestBoundary) }pattern here duplicates the fallback logic insystemcollection.Default(seemodel/access/systemcollection/system_collection.golines 32-40). If additional call sites emerge that need a version-aware address/type keyed offChainHeightVersions, extracting a small helper likesystemcollection.VersionMapperFor(chainID)would keep the fallback behavior consistent across the codebase.Not blocking — this file is currently the only second consumer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@module/state_synchronization/indexer/extended/scheduled_transactions.go` around lines 119 - 129, Duplicate fallback logic for ChainHeightVersions is used here; replace the inline check and access.NewStaticHeightVersionMapper(access.LatestBoundary) with a centralized helper (e.g. add systemcollection.VersionMapperFor(chainID) that returns the mapper or the static LatestBoundary fallback) and then call that helper to assign versionMapper before building executorAddr (referencing systemcollection.ChainHeightVersions, versionMapper, and access.NewStaticHeightVersionMapper in the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@module/state_synchronization/indexer/extended/scheduled_transactions.go`:
- Around line 437-446: The current isExecutorTransaction(tx
*flow.TransactionBody, height uint64) incorrectly classifies v0
ProcessCallbacksTransaction as an executor transaction; update
isExecutorTransaction to add a discriminator that ensures the tx script/content
matches the executor scheduled-transaction script (or explicitly excludes the
ProcessCallbacksTransaction pattern) before returning true—use tx.Script (or its
byte signature/known hash or specific entrypoint/name in the script) alongside
the existing checks and keep executorAddr.ByHeight(height) usage; alternatively,
implement a helper like isProcessCallbacksTransaction(tx) and return false from
isExecutorTransaction when that helper detects a callback transaction so
argument decoding for scheduled tx IDs is only attempted for true executor
transactions.
---
Nitpick comments:
In
`@module/state_synchronization/indexer/extended/scheduled_transactions_test.go`:
- Around line 783-811: The test name
TestScheduledTransactionsIndexer_ExecutorTxNoArguments is misleading because it
verifies that a zero-argument FlowServiceAccount-authorized system transaction
is NOT treated as an executor tx; rename the test function to a clearer name
(e.g., TestScheduledTransactionsIndexer_SystemTxWithNoArgumentsNotMatched) and
update any references/imports/mentions to that symbol so the test declaration,
any calls, and logging reflect the new name while leaving the body (creating
noArgTx, pendingEvt, and the assertions) unchanged.
In `@module/state_synchronization/indexer/extended/scheduled_transactions.go`:
- Around line 125-129: The version map passed to access.NewVersioned for
executorAddr intentionally includes three keys (systemcollection.Version0,
systemcollection.Version1, access.VersionLatest) because the fallback mapper
(versionMapper created via
access.NewStaticHeightVersionMapper(access.LatestBoundary)) can return
access.VersionLatest and access.NewVersioned will panic if a returned version is
missing; add a brief inline comment next to the executorAddr/access.NewVersioned
call explaining that the extra access.VersionLatest entry is required to satisfy
the fallback mapper and must not be removed as a duplicate of Version1 to keep
the existing assertion behavior intact.
- Around line 119-129: Duplicate fallback logic for ChainHeightVersions is used
here; replace the inline check and
access.NewStaticHeightVersionMapper(access.LatestBoundary) with a centralized
helper (e.g. add systemcollection.VersionMapperFor(chainID) that returns the
mapper or the static LatestBoundary fallback) and then call that helper to
assign versionMapper before building executorAddr (referencing
systemcollection.ChainHeightVersions, versionMapper, and
access.NewStaticHeightVersionMapper in the change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4377f2a-cb8d-447b-9efb-cb372f1a4a5e
📒 Files selected for processing (2)
module/state_synchronization/indexer/extended/scheduled_transactions.gomodule/state_synchronization/indexer/extended/scheduled_transactions_test.go
| // isExecutorTransaction returns true if the transaction was submitted by the scheduled executor | ||
| // account: sole authorizer is the scheduled executor address and payer is the empty address. | ||
| func (s *ScheduledTransactions) isExecutorTransaction(tx *flow.TransactionBody) bool { | ||
| // account for the given block height: sole authorizer matches the height-appropriate executor | ||
| // address, payer is the empty address, and the transaction has at least one argument (the | ||
| // scheduled tx ID). | ||
| func (s *ScheduledTransactions) isExecutorTransaction(tx *flow.TransactionBody, height uint64) bool { | ||
| return tx.Payer == flow.EmptyAddress && | ||
| len(tx.Authorizers) == 1 && | ||
| tx.Authorizers[0] == s.scheduledExecutorAddr | ||
| len(tx.Arguments) >= 1 && | ||
| tx.Authorizers[0] == s.executorAddr.ByHeight(height) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for v0 system collection builder and enumerate transactions it emits,
# focusing on any that are authorized by the service account and carry arguments.
fd -t f 'system_collection' model/access/systemcollection/
echo "---- v0 builder ----"
fd -t f 'builder' model/access/systemcollection/ | xargs -r rg -n -C2 'Authorizers|Arguments|ServiceAccount|Payer'
echo "---- cross-reference ----"
rg -nP --type=go -C3 'AddAuthorizer|SetPayer' -g '!**/*_test.go' model/Repository: onflow/flow-go
Length of output: 3928
🏁 Script executed:
#!/bin/bash
# Examine the full implementations of system transactions in v0
echo "=== ProcessCallbacksTransaction (v0) ==="
sed -n '70,85p' model/access/systemcollection/system_collection_v0.go
echo "=== ProcessScheduledTransactionsTransaction (v0) ==="
sed -n '40,51p' model/access/systemcollection/system_collection_v0.go
echo "=== SystemChunkTransaction (v0) ==="
sed -n '145,160p' model/access/systemcollection/system_collection_v0.go
echo "=== Check if Payer is ever set in v0 builders ==="
rg -n 'SetPayer' model/access/systemcollection/system_collection_v0.go
echo "=== Executor address initialization ==="
rg -n 'executorAddr' module/state_synchronization/indexer/extended/scheduled_transactions.go | head -20Repository: onflow/flow-go
Length of output: 2170
🏁 Script executed:
#!/bin/bash
# Check the default Payer value in TransactionBodyBuilder
echo "=== TransactionBodyBuilder Build method ==="
rg -n 'type TransactionBodyBuilder' model/flow/transaction_body_builder.go -A 20
echo "=== TransactionBodyBuilder.Build() method ==="
rg -n 'func.*Build.*TransactionBody' model/flow/transaction_body_builder.go -A 10
echo "=== Executor address initialization for v0 vs v1 ==="
sed -n '125,135p' module/state_synchronization/indexer/extended/scheduled_transactions.go
echo "=== FlowServiceAccount address ==="
rg -n 'FlowServiceAccount' systemcontracts/ | head -10Repository: onflow/flow-go
Length of output: 6130
🏁 Script executed:
#!/bin/bash
# Check the argument decoding logic at line 406
echo "=== Context around line 406 ==="
sed -n '400,420p' module/state_synchronization/indexer/extended/scheduled_transactions.go
echo "=== Full ProcessScheduledTransactions function signature ==="
rg -n 'func.*ProcessScheduledTransactions' module/state_synchronization/indexer/extended/scheduled_transactions.go -A 30 | head -40Repository: onflow/flow-go
Length of output: 1043
Residual risk is real: ProcessCallbacksTransaction would be misidentified as executor transaction in v0 blocks.
The combination Payer == EmptyAddress && len(Authorizers) == 1 && Authorizers[0] == executor(height) && len(Arguments) >= 1 correctly handles ProcessScheduledTransactionsTransaction (zero args) and SystemChunkTransaction (zero args), and the height-aware executor address correctly maps to FlowServiceAccount in v0 blocks.
However, ProcessCallbacksTransaction in v0 satisfies all four conditions:
- Payer: EmptyAddress (never set via
SetPayer) - Authorizers: single
FlowServiceAccount.Address - Arguments: ≥1 (contains callback event ID)
- For v0 blocks,
executorAddr.ByHeight()resolves toFlowServiceAccount.Address
When misidentified as an executor transaction, the code attempts to decode Arguments[0] as a scheduled transaction ID at line 406. Since ProcessCallbacksTransaction's argument is a callback event ID, the decoding fails and aborts block indexing. This is not theoretical: such transactions exist in v0 blocks.
A discriminator (e.g., script content comparison) is needed to distinguish executor transactions from ProcessCallbacksTransaction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@module/state_synchronization/indexer/extended/scheduled_transactions.go`
around lines 437 - 446, The current isExecutorTransaction(tx
*flow.TransactionBody, height uint64) incorrectly classifies v0
ProcessCallbacksTransaction as an executor transaction; update
isExecutorTransaction to add a discriminator that ensures the tx script/content
matches the executor scheduled-transaction script (or explicitly excludes the
ProcessCallbacksTransaction pattern) before returning true—use tx.Script (or its
byte signature/known hash or specific entrypoint/name in the script) alongside
the existing checks and keep executorAddr.ByHeight(height) usage; alternatively,
implement a helper like isProcessCallbacksTransaction(tx) and return false from
isExecutorTransaction when that helper detects a callback transaction so
argument decoding for scheduled tx IDs is only attempted for true executor
transactions.
4 participants
Summary
isExecutorTransactionin the scheduled transactions indexer only matchedScheduledTransactionExecutor.Address, but v0 system collections (mainnet blocks before 133408444, testnet before 290050888) produce executor transactions withFlowServiceAccount.Addressas the authorizer. This caused failed scheduled transactions to throw an exception because they could not be matched to thePendingExecutionevent.Arguments >= 1check to prevent matching other v0 system transactions (ProcessCallbacksTransaction, SystemChunkTransaction) that share the service account authorizer but have no arguments.Test plan
TestScheduledTransactionsIndexer_FailedTransactionV1verifies v1 behaviorTestScheduledTransactionsIndexer_FailedTransactionnow verifies v0 behaviorTestScheduledTransactionsIndexer_ExecutorTxNoArgumentsverifies that zero-argument system txs are not matchedSummary by CodeRabbit
Bug Fixes
Tests