Skip to content

Conversation

@janezpodhostnik
Copy link
Contributor

@janezpodhostnik janezpodhostnik commented Dec 11, 2025

This is part of the effort to enable concurrent execution of transactions.

Exposing the transaction index to cadence allows cadence logic to target different registers according to the transaction index and avoid contention.

Summary by CodeRabbit

Release Notes

  • New Features

    • Added capability to retrieve the current transaction index within transactions and scripts
    • FlowFees account now available by default during system bootstrap
    • Introduced random source history function for system operations
  • Tests

    • Added comprehensive test suite validating transaction index retrieval in both transaction and script execution contexts

✏️ Tip: You can customize this high-level summary in your review settings.

@janezpodhostnik janezpodhostnik self-assigned this Dec 11, 2025
@janezpodhostnik janezpodhostnik requested a review from a team as a code owner December 11, 2025 15:54
@github-actions
Copy link
Contributor

github-actions bot commented Dec 11, 2025

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@codecov-commenter
Copy link

codecov-commenter commented Dec 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@janezpodhostnik janezpodhostnik requested review from a team and turbolent December 18, 2025 15:09
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@turbolent turbolent changed the title Add transaction index to cadence [FVM] Expose transaction index to Cadence Dec 18, 2025
@janezpodhostnik
Copy link
Contributor Author

That change is related, because without it I cannot test changes on a localnet/emulator/integration scenario

address review comments

fix hashes

extract metrics changes

tweak test assertions
@janezpodhostnik janezpodhostnik force-pushed the janez/transaction-index-in-cadence branch from 9e3dfb6 to 238d248 Compare December 19, 2025 15:00
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@coderabbitai
Copy link

coderabbitai bot commented Jan 6, 2026

📝 Walkthrough

Walkthrough

This PR introduces support for a new getTransactionIndex() Cadence runtime function that allows transactions and scripts to query the current transaction index. Changes include adding the function declaration to the Cadence runtime, updating the Environment interface to expose transaction index via TxIndex(), modifying bootstrap to include FlowFees account keys, and updating corresponding state commitment hashes and tests.

Changes

Cohort / File(s) Summary
Cadence Runtime Function Declarations
fvm/runtime/cadence_function_declarations.go
Adds two new Cadence standard library functions: getTransactionIndex() returning UInt32 (fetches current transaction index from runtime environment), and randomSourceHistory() returning [UInt8] (delegates to RandomSourceHistory() method)
Runtime Environment Interface & Setup
fvm/runtime/reusable_cadence_runtime.go
Replaces RandomSourceHistory() ([]byte, error) method with TxIndex() uint32 in Environment interface; introduces declareStandardLibraryFunctions() to register transaction index and random source history declarations in both TxRuntimeEnv and ScriptRuntimeEnv; removes prior declareRandomSourceHistory() implementation
Bootstrap Configuration
fvm/bootstrap.go
Updates Bootstrap function to include FlowFeesAccountPublicKeys with service account public key in default account keys initialization, ensuring FlowFees account is prepared during bootstrap
Bootstrap & Execution State Tests
engine/execution/state/bootstrap/bootstrap_test.go, utils/unittest/execution_state.go
Updates expected state commitment bytes in TestBootstrapLedger_ZeroTokenSupply and TestBootstrapLedger_EmptyTransaction; revises GenesisStateCommitmentHex constant and chain-specific hex values (Testnet and default cases) to reflect new bootstrap state
Transaction Index Feature Tests
fvm/fvm_test.go
Adds new test suite TestTransactionIndexCall with subtests verifying getTransactionIndex() returns correct transaction index in transactions and returns 0 in scripts; includes strconv import for log parsing

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • onflow/flow-go#8289: Updates expected state commitment bytes in the same bootstrap test file, likely part of related state changes.
  • onflow/flow-go#8281: Modifies bootstrap test state commitments similarly, suggesting coordinated state transition updates.

Suggested reviewers

  • peterargue
  • fxamacker

Poem

🐰 A new index hops through the chain,
Transactions now remember their name,
Bootstrap builds with flowfees in sight,
State commits to hash it just right,
Cadence speaks what the runtime has learned!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title clearly and specifically describes the main objective: exposing the transaction index to Cadence runtime for concurrent transaction execution.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ec27707 and 216a53f.

📒 Files selected for processing (6)
  • engine/execution/state/bootstrap/bootstrap_test.go
  • fvm/bootstrap.go
  • fvm/fvm_test.go
  • fvm/runtime/cadence_function_declarations.go
  • fvm/runtime/reusable_cadence_runtime.go
  • utils/unittest/execution_state.go
🧰 Additional context used
📓 Path-based instructions (7)
**/*.go

📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)

Follow Go coding conventions as documented in @docs/agents/CodingConventions.md

Follow Go coding standards and conventions as documented in @docs/agents/GoDocs.md

**/*.go: Follow the existing module structure in /module/, /engine/, /model/ and use dependency injection patterns for component composition
Implement proper interfaces before concrete types
Follow Go naming conventions and the project's coding style defined in /docs/CodingConventions.md
Use mock generators: run make generate-mocks after interface changes
All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Use comprehensive error wrapping for debugging; avoid fmt.Errorf, use irrecoverable package for exceptions
NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Uses golangci-lint with custom configurations (.golangci.yml) and custom linters for Flow-specific conventions (struct write checking)

Files:

  • engine/execution/state/bootstrap/bootstrap_test.go
  • fvm/fvm_test.go
  • fvm/bootstrap.go
  • fvm/runtime/cadence_function_declarations.go
  • utils/unittest/execution_state.go
  • fvm/runtime/reusable_cadence_runtime.go
**/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*_test.go: Unit tests should be co-located with the code they test
Follow the existing pattern of *_test.go files for test naming
Use fixtures for realistic test data as defined in /utils/unittest/

Files:

  • engine/execution/state/bootstrap/bootstrap_test.go
  • fvm/fvm_test.go
{module,engine,cmd}/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

All major processing components must implement the Component interface from /module/component/component.go to ensure consistent lifecycle management and graceful shutdown patterns

Files:

  • engine/execution/state/bootstrap/bootstrap_test.go
{crypto,fvm,ledger,access,engine}/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation

Files:

  • engine/execution/state/bootstrap/bootstrap_test.go
  • fvm/fvm_test.go
  • fvm/bootstrap.go
  • fvm/runtime/cadence_function_declarations.go
  • fvm/runtime/reusable_cadence_runtime.go
{network,engine,consensus}/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Network messages must be authenticated and validated

Files:

  • engine/execution/state/bootstrap/bootstrap_test.go
{crypto,fvm,ledger,storage}/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Security checks for cryptographic misuse must be enforced using gosec

Files:

  • fvm/fvm_test.go
  • fvm/bootstrap.go
  • fvm/runtime/cadence_function_declarations.go
  • fvm/runtime/reusable_cadence_runtime.go
{storage,ledger,execution,fvm}/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

State consistency is paramount; use proper synchronization primitives

Files:

  • fvm/fvm_test.go
  • fvm/bootstrap.go
  • fvm/runtime/cadence_function_declarations.go
  • fvm/runtime/reusable_cadence_runtime.go
🧠 Learnings (2)
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation

Applied to files:

  • fvm/fvm_test.go
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {storage,ledger,execution,fvm}/**/*.go : State consistency is paramount; use proper synchronization primitives

Applied to files:

  • utils/unittest/execution_state.go
🧬 Code graph analysis (4)
fvm/fvm_test.go (6)
fvm/fvm.go (2)
  • Run (78-85)
  • VM (106-122)
model/flow/chain.go (1)
  • Chain (263-275)
fvm/context.go (3)
  • Context (32-54)
  • NewContextFromParent (62-64)
  • WithCadenceLogging (261-266)
fvm/environment/system_contracts.go (1)
  • ServiceAddress (83-85)
engine/execution/testutil/fixtures.go (1)
  • SignTransactionAsServiceAccount (161-163)
fvm/script.go (1)
  • Script (27-35)
fvm/bootstrap.go (1)
model/flow/account.go (1)
  • AccountPublicKey (24-32)
fvm/runtime/cadence_function_declarations.go (1)
fvm/runtime/reusable_cadence_runtime.go (1)
  • ReusableCadenceRuntime (20-26)
utils/unittest/execution_state.go (1)
model/flow/chain.go (1)
  • Sandboxnet (26-26)
⏰ 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). (37)
  • GitHub Check: Lint (./insecure/)
  • GitHub Check: Lint (./)
  • GitHub Check: Lint (./integration/)
  • GitHub Check: Unit Tests (engine/execution/computation)
  • GitHub Check: Unit Tests (network/p2p/connection)
  • GitHub Check: Unit Tests (cmd)
  • GitHub Check: Unit Tests (network/test/cohort2)
  • GitHub Check: Unit Tests (module/dkg)
  • GitHub Check: Unit Tests (module)
  • GitHub Check: Unit Tests (engine/common)
  • GitHub Check: Unit Tests (engine/verification)
  • GitHub Check: Unit Tests (utils)
  • GitHub Check: Unit Tests (network)
  • GitHub Check: Unit Tests (admin)
  • GitHub Check: Unit Tests (engine/consensus)
  • GitHub Check: Unit Tests (others)
  • GitHub Check: Unit Tests (engine/collection)
  • GitHub Check: Unit Tests (storage)
  • GitHub Check: Unit Tests (network/p2p)
  • GitHub Check: Unit Tests (engine/execution/ingestion)
  • GitHub Check: Unit Tests (fvm)
  • GitHub Check: Unit Tests (engine/access)
  • GitHub Check: Unit Tests (engine/execution)
  • GitHub Check: Unit Tests (network/alsp)
  • GitHub Check: Unit Tests (network/p2p/scoring)
  • GitHub Check: Unit Tests (network/test/cohort1)
  • GitHub Check: Unit Tests (ledger)
  • GitHub Check: Unit Tests (network/p2p/node)
  • GitHub Check: Unit Tests (engine)
  • GitHub Check: Unit Tests (consensus)
  • GitHub Check: Unit Tests (state)
  • GitHub Check: Integration Tests Others (integration)
  • GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
  • GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
  • GitHub Check: Unit Tests Insecure (insecure)
  • GitHub Check: Docker Build
  • GitHub Check: analyze-code (go)
🔇 Additional comments (12)
engine/execution/state/bootstrap/bootstrap_test.go (2)

61-61: LGTM: Test expectation updated for new bootstrap state.

The expected state commitment hex has been updated to reflect the new bootstrap behavior that includes FlowFees account keys. This aligns with the bootstrap changes in fvm/bootstrap.go.


110-110: LGTM: Test expectation updated for new bootstrap state.

The expected state commitment hex has been updated to reflect the new bootstrap behavior that includes FlowFees account keys. This aligns with the bootstrap changes in fvm/bootstrap.go.

utils/unittest/execution_state.go (2)

26-26: LGTM: Genesis state commitment updated for new bootstrap behavior.

The GenesisStateCommitmentHex constant has been updated to reflect the new bootstrap state that includes FlowFees account keys.


90-90: LGTM: Chain-specific genesis commitments updated.

The genesis state commitment values for Testnet and the default case have been updated to match the new bootstrap behavior that includes FlowFees account keys.

Also applies to: 95-95

fvm/bootstrap.go (1)

252-252: FlowFees account keys added to default bootstrap.

The FlowFeesAccountPublicKeys field is now initialized with the service account public key, ensuring the FlowFees account is created with keys during bootstrap. This enables the FlowFees contract deployment path and supports concurrent execution testing.

Based on past review comments, the author noted this change is necessary for testing on localnet/emulator/integration scenarios, and a reviewer suggested moving this to a separate PR if unrelated to exposing transaction index to Cadence.

fvm/fvm_test.go (2)

10-10: LGTM: Import added for test support.

The strconv import is used in the new TestTransactionIndexCall test to parse the logged transaction index from string format.


4282-4359: LGTM: Comprehensive test coverage for getTransactionIndex.

The new test suite covers both transaction and script contexts:

  • Transaction test verifies that getTransactionIndex returns the correct txIndex (3) passed to fvm.Transaction.
  • Script test verifies that getTransactionIndex returns 0 in script context, which is expected behavior.

Both tests are well-structured and provide good coverage for the new Cadence runtime function.

fvm/runtime/cadence_function_declarations.go (2)

18-53: LGTM: randomSourceHistory function properly implemented.

The blockRandomSourceDeclaration correctly implements the randomSourceHistory host function, which delegates to env.RandomSourceHistory() and returns the result as a Cadence ByteArray. The function is appropriately documented as being only for System transaction use.


61-82: This suggestion would introduce a critical bug and should not be applied.

The current implementation is intentionally correct for this pooled runtime pattern. The ReusableCadenceRuntime is reused across multiple operations via a pool:

  • At initialization, fvmEnv is unset (nil)
  • When borrowed, it's set to the current environment
  • When returned, it's set back to nil for reuse

The lazy nil check inside the closure is essential because the declaration is created once but must handle fvmEnv changing between nil and valid values across multiple borrow/return cycles. The suggested refactoring would move the check to function creation time, when fvmEnv is not yet initialized, causing either a panic or use of a stale environment. The current pattern is a necessary implementation detail for safe pooling, not a style preference.

Likely an incorrect or invalid review comment.

fvm/runtime/reusable_cadence_runtime.go (3)

38-38: LGTM!

The method name change from declareRandomSourceHistory() to declareStandardLibraryFunctions() appropriately reflects the broader scope of declaring multiple standard library functions.


43-49: Good implementation of declaration reuse.

The implementation correctly follows the past review suggestion to reuse the transactionIndexDeclaration for both environments (lines 46-48).

Note the asymmetry: blockRandomSourceDeclaration is only declared for TxRuntimeEnv (line 44), while transactionIndexDeclaration is declared for both TxRuntimeEnv and ScriptRuntimeEnv. This is likely intentional—scripts shouldn't access block random source but should be able to query transaction index. Please confirm this design decision is correct.


16-17: No action required — both RandomSourceHistory() and TxIndex() correctly coexist in the interface.

The AI summary's claim that TxIndex() is "replacing" RandomSourceHistory() is inaccurate. RandomSourceHistory() is actively used in the codebase (fvm/runtime/cadence_function_declarations.go:38) and is part of the core Environment interface. TxIndex() is a new method being added alongside it, not a replacement. Both methods should remain in the interface.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants