Automate flow-go test#98
Conversation
Encodes the script/README.md localnet validation procedure as a Go integration test behind the `localnet` build tag, runnable via `make localnet-test`. It bootstraps and funds an originator, starts the server against a running localnet, waits for the indexer to reach tip, then creates a derived account and transfers funds through the construction API — asserting via the REST API that the server indexes the network and observes the transfer. Funding renders basic-transfer.cdc through script.Compile (the template is invalid Cadence as-is, so `make fund-accounts` never worked), and the indexer fails fast on block-ID mismatches so a stale spork version or a flow-go incompatibility surfaces immediately. Verified against a flow-go-master localnet. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Opens the possibility of running Rosetta against the flow emulator. Localnet should be preferred over benchnet anyway.
📝 WalkthroughWalkthroughAdds a localnet compatibility test and Makefile entry point, centralizes Badger key prefixes, removes emulator chain-ID remapping, and adds mainnet28 spork version 8 hash verification coverage. ChangesLocalnet compatibility test
Index key prefix centralization
Protocol version 8 hash verification
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
indexdb/indexdb.go (1)
312-318: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the centralized prefix variables instead of hardcoded byte literals.
Several pre-allocated key construction sites still hardcode single-character byte literals accompanied by a comment, which defeats the purpose of centralizing prefixes in package variables. Reference the 0th index of the centralized prefix variables directly to enforce a single source of truth.
indexdb/indexdb.go#L312-L318: Replacekey[0] = 'a'withkey[0] = accountPrefix[0].indexdb/indexdb.go#L427-L432: Replacekey[0] = 'p'withkey[0] = isProxyPrefix[0].indexdb/indexdb.go#L444-L450: Replacekey[0] = 'a'withkey[0] = accountPrefix[0].indexdb/indexdb.go#L701-L715: ReplaceblockKey[0] = 'b'withblockKey[0] = blockPrefix[0], andhash2HeightKey[0] = 'c'withhash2HeightKey[0] = hash2HeightPrefix[0].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indexdb/indexdb.go` around lines 312 - 318, Replace hardcoded key-prefix byte literals with the centralized prefix variables’ first bytes in Store.HasBalance and the affected key-construction sites: use accountPrefix[0], isProxyPrefix[0], accountPrefix[0], blockPrefix[0], and hash2HeightPrefix[0] respectively. Apply these changes in indexdb/indexdb.go at lines 312-318, 427-432, 444-450, and 701-715, preserving the existing key layouts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@indexdb/indexdb.go`:
- Around line 28-35: Define byte constants for each prefix alongside the
existing prefix slices, then initialize accountPrefix, blockPrefix,
hash2HeightPrefix, height2HashPrefix, and isProxyPrefix from those constants.
Update hardcoded prefix assignments such as key[0] in the relevant code to use
the corresponding constant, while retaining the slices for iterator usage and
avoiding direct append operations on shared global slices.
- Around line 341-345: Eliminate concurrent append operations on shared prefix
slices in indexdb/indexdb.go at lines 341-345, 368-372, 456-462, and 781-789. In
the affected indexdb methods, allocate each key buffer with its exact required
length, copy the prefix and payload into it, and preserve the existing key
contents and ordering; update all listed blockPrefix, hash2HeightPrefix, and
height2HashPrefix constructions, with no direct changes needed beyond these
replacements.
---
Nitpick comments:
In `@indexdb/indexdb.go`:
- Around line 312-318: Replace hardcoded key-prefix byte literals with the
centralized prefix variables’ first bytes in Store.HasBalance and the affected
key-construction sites: use accountPrefix[0], isProxyPrefix[0],
accountPrefix[0], blockPrefix[0], and hash2HeightPrefix[0] respectively. Apply
these changes in indexdb/indexdb.go at lines 312-318, 427-432, 444-450, and
701-715, preserving the existing key layouts.
🪄 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: 89881450-9f1d-4cd8-b1fb-b55520350f31
📒 Files selected for processing (8)
Makefileindexdb/indexdb.golocalnet.jsonlocalnettest/doc.golocalnettest/localnet_test.goscript/README.mdstate/convert.gostate/convert_test.go
💤 Files with no reviewable changes (1)
- state/convert.go
| var ( | ||
| accountPrefix = []byte("a") | ||
| blockPrefix = []byte("b") | ||
| hash2HeightPrefix = []byte("c") | ||
| height2HashPrefix = []byte("d") | ||
| isProxyPrefix = []byte("p") | ||
| ) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Consider defining byte constants to fix inconsistencies and avoid potential slice-append races.
While centralizing the prefix definitions is a great refactor, defining them solely as global slices creates an unsafe pattern later in the file where append(prefix, ...) is called on them. If the capacity of a global slice happens to be larger than its length, concurrent append calls will write to the same backing array, causing a data race.
Additionally, because accountPrefix is a slice, other areas of the code (like line 315) are still forced to hardcode key[0] = 'a', defeating the purpose of centralizing the prefixes.
Consider defining the exact bytes as constants, and deriving the slices from them for iterators:
const (
accountPrefixByte = 'a'
blockPrefixByte = 'b'
hash2HeightPrefixByte = 'c'
height2HashPrefixByte = 'd'
isProxyPrefixByte = 'p'
)
var (
accountPrefix = []byte{accountPrefixByte}
blockPrefix = []byte{blockPrefixByte}
hash2HeightPrefix = []byte{hash2HeightPrefixByte}
height2HashPrefix = []byte{height2HashPrefixByte}
isProxyPrefix = []byte{isProxyPrefixByte}
)This allows the rest of the file to use key[0] = accountPrefixByte safely and cleanly, while avoiding the unsafe append on global slices.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Consider defining byte constants to fix inconsistencies and avoid potential slice-append races.
While centralizing the prefix definitions is a great refactor, defining them solely as global slices creates an unsafe pattern later in the file where append(prefix, ...) is called on them. If the capacity of a global slice happens to be larger than its length, concurrent append calls will write to the same backing array, causing a data race.
Additionally, because accountPrefix is a slice, other areas of the code (like line 315) are still forced to hardcode key[0] = 'a', defeating the purpose of centralizing the prefixes.
Consider defining the exact bytes as constants, and deriving the slices from them for iterators:
const (
accountPrefixByte = 'a'
blockPrefixByte = 'b'
hash2HeightPrefixByte = 'c'
height2HashPrefixByte = 'd'
isProxyPrefixByte = 'p'
)
var (
accountPrefix = []byte{accountPrefixByte}
blockPrefix = []byte{blockPrefixByte}
hash2HeightPrefix = []byte{hash2HeightPrefixByte}
height2HashPrefix = []byte{height2HashPrefixByte}
isProxyPrefix = []byte{isProxyPrefixByte}
)This allows the rest of the file to use key[0] = accountPrefixByte safely and cleanly, while avoiding the unsafe append on global slices.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@indexdb/indexdb.go` around lines 28 - 35, Define byte constants for each
prefix alongside the existing prefix slices, then initialize accountPrefix,
blockPrefix, hash2HeightPrefix, height2HashPrefix, and isProxyPrefix from those
constants. Update hardcoded prefix assignments such as key[0] in the relevant
code to use the corresponding constant, while retaining the slices for iterator
usage and avoiding direct append operations on shared global slices.
| var hash []byte | ||
| heightEnc := make([]byte, 8) | ||
| binary.BigEndian.PutUint64(heightEnc, height) | ||
| key := append([]byte("d"), heightEnc...) | ||
| key := append(height2HashPrefix, heightEnc...) | ||
| err := s.db.View(func(txn *badger.Txn) error { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Data race hazard: append on global slices.
Using append on a global slice variable is unsafe in concurrent code. If the global slice has any spare capacity (cap > len), concurrent append calls will write over each other in the same backing array, leading to a data race and index corruption. Pre-allocate the target slice and use copy to guarantee thread safety.
indexdb/indexdb.go#L341-L345: Replaceappend(height2HashPrefix, heightEnc...)with a safely pre-allocated slicekey := make([]byte, 1+8); key[0] = height2HashPrefix[0]; copy(key[1:], heightEnc).indexdb/indexdb.go#L368-L372: Replaceappend(hash2HeightPrefix, hash...)with safe pre-allocation andcopy().indexdb/indexdb.go#L456-L462: ReplaceappendonblockPrefix,hash2HeightPrefix, andheight2HashPrefixwith safe pre-allocation andcopy().indexdb/indexdb.go#L781-L789: ReplaceappendonblockPrefix,hash2HeightPrefix, andheight2HashPrefixwith safe pre-allocation andcopy().
📍 Affects 1 file
indexdb/indexdb.go#L341-L345(this comment)indexdb/indexdb.go#L368-L372indexdb/indexdb.go#L456-L462indexdb/indexdb.go#L781-L789
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@indexdb/indexdb.go` around lines 341 - 345, Eliminate concurrent append
operations on shared prefix slices in indexdb/indexdb.go at lines 341-345,
368-372, 456-462, and 781-789. In the affected indexdb methods, allocate each
key buffer with its exact required length, copy the prefix and payload into it,
and preserve the existing key contents and ordering; update all listed
blockPrefix, hash2HeightPrefix, and height2HashPrefix constructions, with no
direct changes needed beyond these replacements.
1 participant
Adds an automated version of the test described in script/README.md. Also includes some minor cleanups, but no changes to the Rosetta server itself.
The test should be run against a localnet running the new flow-go version, to confirm that the existing Rosetta server is compatible with the flow-go upgrade. If the test fails, most likely Rosetta will need to be updated with a new compatibility version (adding a new spork version with appropriate changes to be able to successfully verify new and old blocks).
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests