feat: make blocktracker aware of block hashes #400
Conversation
|
Warning Rate limit exceeded@fbac has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis pull request introduces changes across multiple files to enhance block tracking and logging functionality. Key modifications include adding block hash support in database and indexing operations, updating configuration files for local network contracts, and standardizing error message formatting. The changes encompass database migrations, indexer logic, and test utilities, focusing on improving the precision and clarity of block-related operations. Changes
Suggested reviewers
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pkg/testutils/blockchain.go (1)
9-11: Add function documentation.Consider adding a doc comment explaining the purpose and use case of this utility function. This would help other developers understand when to use this conversion function versus other hash generation utilities.
+// IntToHash converts an int64 to a common.Hash, useful for generating deterministic test hashes func IntToHash(x int64) common.Hash {pkg/indexer/interface.go (1)
10-12: Consider consolidating block retrieval methodsTo ensure atomic retrieval of the latest block data and prevent potential mismatches between the block number and hash, consider providing a single method that returns a
Blockstruct containing both values.Apply this diff to update the interface:
type IBlockTracker interface { - GetLatestBlockNumber() uint64 - GetLatestBlockHash() common.Hash + GetLatestBlock() *Block UpdateLatestBlock(ctx context.Context, block uint64, hash common.Hash) error }This change promotes consistency and simplifies the interface by encapsulating block data within a single method.
pkg/indexer/blockTracker_test.go (1)
87-91: Consider using a helper function for block updates in concurrent tests.The block update code is repeated across multiple goroutines. Consider extracting it into a helper function for better maintainability.
+func updateBlock(ctx context.Context, t *testing.T, tracker *BlockTracker, blockNum uint64) { + err := tracker.UpdateLatestBlock( + ctx, + blockNum, + testutils.IntToHash(int64(blockNum)) + ) + require.NoError(t, err) +}pkg/indexer/indexer.go (1)
59-60: Minor: Consider removing trailing commaThe trailing comma after the
validationServiceparameter, while valid, is inconsistent with Go's common style.- validationService mlsvalidate.MLSValidationService, + validationService mlsvalidate.MLSValidationService
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.gitignore(1 hunks)contracts/config/anvil_localnet/GroupMessages.json(1 hunks)contracts/config/anvil_localnet/IdentityUpdates.json(1 hunks)pkg/blockchain/registryAdmin.go(1 hunks)pkg/blockchain/signer.go(2 hunks)pkg/db/queries.sql(1 hunks)pkg/db/queries/models.go(1 hunks)pkg/db/queries/queries.sql.go(2 hunks)pkg/indexer/blockTracker.go(3 hunks)pkg/indexer/blockTracker_test.go(5 hunks)pkg/indexer/indexer.go(4 hunks)pkg/indexer/indexer_test.go(1 hunks)pkg/indexer/interface.go(1 hunks)pkg/migrations/00003_add-latest-block.up.sql(1 hunks)pkg/mocks/indexer/mock_IBlockTracker.go(5 hunks)pkg/sync/syncWorker.go(3 hunks)pkg/testutils/blockchain.go(1 hunks)pkg/testutils/random.go(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- pkg/blockchain/registryAdmin.go
- pkg/blockchain/signer.go
- pkg/sync/syncWorker.go
🔇 Additional comments (15)
pkg/testutils/random.go (1)
66-69: Implementation looks good!The function follows the established pattern of other random generators in this file. While the implementation is similar to
RandomLogTopic, having a separate function provides better semantic clarity for tests specifically dealing with block hashes.contracts/config/anvil_localnet/GroupMessages.json (2)
8-9: Consider implications of starting from block 0.Setting both
deploymentBlockandlatestUpgradeBlockto 0 means the system will track from the genesis block. While this ensures no historical data is missed, verify if this aligns with the PR's objective of making blocktracker aware of block hashes.Consider:
- Performance implications of tracking from block 0
- Whether historical data before the contract deployment is relevant
- If this change is temporary for testing purposes
4-5: Verify contract addresses match local network deployment.The new addresses (
0x5FbDB2315678...and0xe7f1725E7734...) appear to be deterministic addresses commonly used in local Hardhat/Anvil networks. This is consistent with the file's location in theanvil_localnetdirectory.✅ Verification successful
Contract addresses are valid and consistent with local network configuration.
The addresses follow Hardhat/Anvil's deterministic deployment pattern, with proper separation between contracts and consistent use of deployer/admin addresses across configurations. The configuration is correctly isolated to the local network environment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistency of contract addresses across local network configs # Look for any mismatches in contract addresses that might indicate inconsistent deployments # Search for these addresses in other config files echo "Checking for contract address consistency across config files:" rg -l "0x5FbDB2315678afecb367f032d93F642f64180aa3|0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512" contracts/config/Length of output: 299
Script:
#!/bin/bash # Find all JSON files in the config directory echo "All JSON configuration files:" fd -e json . contracts/config/ # Check if there are any other files mentioning "localnet" that might need updates echo -e "\nFiles containing 'localnet':" rg -l "localnet" contracts/config/Length of output: 319
Script:
#!/bin/bash # Check the content of both JSON files echo "Content of GroupMessages.json:" cat contracts/config/anvil_localnet/GroupMessages.json echo -e "\nContent of IdentityUpdates.json:" cat contracts/config/anvil_localnet/IdentityUpdates.jsonLength of output: 1014
pkg/db/queries/queries.sql.go (2)
80-97: Verify database schema alignment with new queriesThe
GetLatestBlockfunction and theGetLatestBlockRowstruct now includeblock_hash. Ensure that the database schema and migrations have been updated to include theblock_hashcolumn in thelatest_blocktable.
389-405: Consistency in generated code and source SQL filesChanges in the generated
queries.sql.gofile reflect modifications in the SQL queries. Ensure that these changes originate from updates in the source SQL files and that the code generator (sqlc) is rerun to maintain consistency.pkg/db/queries/models.go (1)
30-30: LGTM! Verify the corresponding SQL schema changes.The addition of
BlockHash []bytefield to theLatestBlockstruct is appropriate for storing block hashes.Run this script to verify the corresponding SQL schema changes:
✅ Verification successful
Schema implementation verified and correct
The
block_hashcolumn is properly defined in the SQL schema:
- Table creation with
block_hash BYTEA NOT NULL- Appropriate CRUD operations in place
- Consistent with the Go struct implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the SQL schema changes for the block_hash column # Expected: Find ALTER TABLE or CREATE TABLE statements adding block_hash column rg -g "*.sql" "block_hash.*latest_block"Length of output: 42
Script:
#!/bin/bash # Find all SQL files and their content fd -e sql -x cat {}Length of output: 6661
pkg/indexer/indexer_test.go (1)
22-24: LGTM! Test changes align with the new block hash functionality.The initialization of
newBlockHashand its usage in the mock expectations are appropriate.Also applies to: 28-28
pkg/indexer/blockTracker_test.go (2)
38-45: LGTM! Comprehensive test coverage for block updates.The test properly verifies both block number and hash updates, including the case where a higher block number is updated.
128-134: LGTM! Proper test isolation for multiple contract addresses.The test correctly verifies that different contract addresses maintain separate block numbers and hashes.
pkg/mocks/indexer/mock_IBlockTracker.go (1)
26-44: LGTM! Mock implementation properly handles block hash functionality.The mock implementation correctly handles the new block hash functionality with proper error checking for missing return values.
pkg/indexer/indexer.go (2)
150-150: Method rename reflects clearer intentGood change! Renaming from
GetLatestBlocktoGetLatestBlockNumberbetter describes what the method returns.Also applies to: 167-167
215-217: Verify error handling for block hash updatesThe error from
UpdateLatestBlockis logged but not propagated. Consider if this error should trigger a retry of the entire event processing.✅ Verification successful
Current error handling is appropriate
The BlockTracker is a progress tracking mechanism where errors are non-fatal and shouldn't halt event processing. The current approach of logging errors is sufficient because:
- Database errors or invalid hashes are infrastructure issues that wouldn't be resolved by retrying
- Failed updates don't affect the integrity of already processed events
- Errors are properly logged for monitoring and debugging purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any other places in the codebase where block tracker errors are handled differently rg -A 3 "UpdateLatestBlock.*error"Length of output: 1165
Script:
#!/bin/bash # Find implementation of UpdateLatestBlock ast-grep --pattern 'func $_($_ *$_) UpdateLatestBlock(ctx context.Context, block uint64, hash common.Hash) error { $$$ }' # Find all usages of UpdateLatestBlock rg -B 3 -A 3 "UpdateLatestBlock\(ctx,"Length of output: 2638
Script:
#!/bin/bash # Find the BlockTracker implementation fd blockTracker.go --exec cat {}Length of output: 2709
pkg/migrations/00003_add-latest-block.up.sql (1)
3-4: Verify migration strategy for existing dataAdding a NOT NULL column to an existing table requires consideration of:
- How existing rows will be populated with block hashes
- Whether a corresponding down migration exists
- The deployment strategy to avoid breaking production
✅ Verification successful
Migration strategy is properly implemented ✅
The NOT NULL constraints are safe as this is a new table creation rather than an alteration of an existing table. The down migration is also properly implemented with table drop.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for corresponding down migration fd --type f "00003.*down.sql" # Check if this is the latest migration fd --type f "0000[4-9].*up.sql"Length of output: 112
Script:
#!/bin/bash # Check content of both migration files echo "=== UP Migration ===" cat pkg/migrations/00003_add-latest-block.up.sql echo -e "\n=== DOWN Migration ===" cat pkg/migrations/00003_add-latest-block.down.sqlLength of output: 385
contracts/config/anvil_localnet/IdentityUpdates.json (1)
4-5: LGTM: Local environment configuration updatesThe changes to contract addresses and block numbers are specific to the local development environment (anvil_localnet).
Also applies to: 8-9
pkg/db/queries.sql (1)
117-118: LGTM: Consistent with schema changesThe SELECT query has been properly updated to include the new block_hash column.
fbac
force-pushed
the
feat/store-block-number-and-hashes
branch
from
32558dd to
03a7564
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pkg/indexer/blockTracker.go (2)
28-31: Consider consistent synchronization approach for Block struct fields.The Block struct uses atomic operations for
numberbut relies on the parent's mutex forhash. This split approach could be confusing and error-prone. Consider either:
- Using the mutex for both fields
- Making the hash field atomic using atomic.Value
type Block struct { - number atomic.Uint64 - hash common.Hash + mu sync.RWMutex + number uint64 + hash common.Hash }
108-111: Improve Block initialization and error message.Consider these improvements:
- Use composite literal for Block initialization
- Make the error message more descriptive by including the actual value
- block := &Block{ - number: atomic.Uint64{}, - hash: common.Hash{}, - } + block := new(Block) - return block, fmt.Errorf( - "invalid block number %d for contract %s", - latestBlock.BlockNumber, - contractAddress, - ) + return block, fmt.Errorf( + "expected non-negative block number but got %d for contract %s", + latestBlock.BlockNumber, + contractAddress, + )Also applies to: 121-126
pkg/indexer/blockTracker_test.go (2)
38-45: Add test cases for hash validation and concurrent access.Consider adding these test cases:
- Test invalid (empty) hash validation
- Test concurrent access to block hash
Example test case for invalid hash:
func TestUpdateLatestBlockWithInvalidHash(t *testing.T) { // ... setup code ... err = tracker.UpdateLatestBlock(ctx, 100, common.Hash{}) require.Error(t, err) require.Contains(t, err.Error(), "invalid block hash") }
87-91: Enhance concurrent update testing.Consider these improvements to make the test more robust:
- Use different hash values instead of deriving from block number
- Add intermediate assertions during concurrent updates
- err := tracker.UpdateLatestBlock( - ctx, - blockNum, - testutils.IntToHash(int64(blockNum)), - ) + hash := testutils.RandomHash() // Add this helper + err := tracker.UpdateLatestBlock(ctx, blockNum, hash) + require.NoError(t, err) + + // Add intermediate verification + currentNum := tracker.GetLatestBlockNumber() + require.GreaterOrEqual(t, currentNum, blockNum)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.gitignore(1 hunks)contracts/config/anvil_localnet/GroupMessages.json(1 hunks)contracts/config/anvil_localnet/IdentityUpdates.json(1 hunks)pkg/blockchain/registryAdmin.go(1 hunks)pkg/blockchain/signer.go(2 hunks)pkg/db/queries.sql(1 hunks)pkg/db/queries/models.go(1 hunks)pkg/db/queries/queries.sql.go(2 hunks)pkg/indexer/blockTracker.go(3 hunks)pkg/indexer/blockTracker_test.go(5 hunks)pkg/indexer/indexer.go(4 hunks)pkg/indexer/indexer_test.go(1 hunks)pkg/indexer/interface.go(1 hunks)pkg/migrations/00003_add-latest-block.up.sql(1 hunks)pkg/mocks/indexer/mock_IBlockTracker.go(5 hunks)pkg/sync/syncWorker.go(3 hunks)pkg/testutils/blockchain.go(1 hunks)pkg/testutils/random.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- pkg/testutils/blockchain.go
- pkg/db/queries/models.go
- pkg/testutils/random.go
- pkg/blockchain/signer.go
- pkg/migrations/00003_add-latest-block.up.sql
- pkg/indexer/indexer_test.go
- pkg/indexer/interface.go
- pkg/sync/syncWorker.go
- contracts/config/anvil_localnet/GroupMessages.json
- .gitignore
- pkg/db/queries.sql
- pkg/blockchain/registryAdmin.go
- pkg/indexer/indexer.go
- contracts/config/anvil_localnet/IdentityUpdates.json
🔇 Additional comments (4)
pkg/db/queries/queries.sql.go (2)
80-97: LGTM! Well-structured changes to support block hash tracking.The addition of block hash tracking is implemented correctly with proper types and error handling.
399-407: LGTM! Clean implementation of block hash parameter handling.The parameter struct and function changes are implemented correctly with proper error handling.
pkg/indexer/blockTracker.go (1)
57-61: LGTM! Thread-safe implementation.The implementation correctly uses mutex synchronization for accessing the hash field, addressing the potential data race concern.
pkg/mocks/indexer/mock_IBlockTracker.go (1)
26-71: LGTM! Auto-generated mock implementation.The mock implementation correctly reflects the interface changes for the new hash-aware block tracker.
fbac
force-pushed
the
feat/store-block-number-and-hashes
branch
from
03a7564 to
e293c0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
pkg/db/queries/queries.sql.go (2)
80-97:⚠️ Potential issueAdd block hash validation to prevent reorg vulnerabilities
The query needs additional validation to handle chain reorganizations properly. Currently, it only retrieves the block hash without validating it against the previous state.
Consider modifying the query to:
- Compare block hashes when block numbers match
- Validate block parent hash for consistency
SELECT block_number, block_hash FROM latest_block WHERE - contract_address = $1 + contract_address = $1 + AND ( + -- Allow higher block numbers + block_number > (SELECT block_number FROM latest_block WHERE contract_address = $1) + OR + -- For same block number, ensure hash matches to detect reorgs + (block_number = (SELECT block_number FROM latest_block WHERE contract_address = $1) + AND block_hash = (SELECT block_hash FROM latest_block WHERE contract_address = $1)) + )
389-405:⚠️ Potential issueEnhance block validation in SetLatestBlock query
The current WHERE clause only checks if the new block number is higher, which is insufficient for proper chain reorganization handling. This could allow blocks from a different chain fork to be inserted if they have higher numbers.
Consider enhancing the validation:
INSERT INTO latest_block(contract_address, block_number, block_hash) VALUES ($1, $2, $3) ON CONFLICT (contract_address) DO UPDATE SET block_number = $2, block_hash = $3 WHERE - $2 > latest_block.block_number + ( + -- Allow higher blocks + $2 > latest_block.block_number + OR + -- For same height blocks, ensure hash matches to detect reorgs + ($2 = latest_block.block_number AND $3 = latest_block.block_hash) + )pkg/indexer/blockTracker.go (1)
81-89:⚠️ Potential issueFix validation order in UpdateLatestBlock.
The hash validation happens after the DB update. If the hash is invalid, the DB would already be updated with an invalid state.
Move the validation before the DB update:
+ hashBytes := common.Hash(hash) + if hashBytes == (common.Hash{}) { + return fmt.Errorf("invalid block hash %s", hashBytes.String()) + } + if err := bt.updateDB(ctx, block, hash); err != nil { return err } - - hashBytes := common.Hash(hash) - - if hashBytes == (common.Hash{}) { - return fmt.Errorf("invalid block hash %s", hashBytes.String()) - }
🧹 Nitpick comments (3)
pkg/db/queries/queries.sql.go (1)
Line range hint
80-405: Consider enhancing chain reorganization handlingWhile adding block hash awareness is a good first step, the current design might not fully handle complex chain reorganization scenarios. Consider these architectural improvements:
- Maintain a block history table to track parent-child relationships
- Store a configurable number of recent blocks to handle deep reorgs
- Add ability to detect and recover from chain splits by reverting to the last common ancestor
Example schema for block history:
CREATE TABLE block_history ( contract_address TEXT NOT NULL, block_number BIGINT NOT NULL, block_hash BYTEA NOT NULL, parent_hash BYTEA NOT NULL, is_canonical BOOLEAN NOT NULL DEFAULT true, created_at TIMESTAMP NOT NULL DEFAULT NOW(), PRIMARY KEY (contract_address, block_number, block_hash) );pkg/indexer/blockTracker.go (2)
28-31: LGTM! Good encapsulation of block-related data.The new
Blockstruct provides good encapsulation of block-related data with thread-safe operations on the block number.Consider adding a comment to document that the
hashfield must be accessed under theBlockTracker.mulock:type Block struct { number atomic.Uint64 + // hash must be accessed under BlockTracker.mu lock hash common.Hash }
109-134: Consider simplifying the Block initialization.The implementation is correct, but the Block initialization could be more concise.
Consider this simpler initialization:
- block := &Block{ - number: atomic.Uint64{}, - hash: common.Hash{}, - } + block := new(Block)The zero value of
atomic.Uint64andcommon.Hashis already what we want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.gitignore(1 hunks)contracts/config/anvil_localnet/GroupMessages.json(1 hunks)contracts/config/anvil_localnet/IdentityUpdates.json(1 hunks)pkg/blockchain/registryAdmin.go(1 hunks)pkg/blockchain/registryAdmin_test.go(1 hunks)pkg/blockchain/signer.go(2 hunks)pkg/db/queries.sql(1 hunks)pkg/db/queries/models.go(1 hunks)pkg/db/queries/queries.sql.go(2 hunks)pkg/indexer/blockTracker.go(3 hunks)pkg/indexer/blockTracker_test.go(5 hunks)pkg/indexer/indexer.go(4 hunks)pkg/indexer/indexer_test.go(1 hunks)pkg/indexer/interface.go(1 hunks)pkg/migrations/00003_add-latest-block.up.sql(1 hunks)pkg/mocks/indexer/mock_IBlockTracker.go(4 hunks)pkg/sync/syncWorker.go(3 hunks)pkg/testutils/blockchain.go(1 hunks)pkg/testutils/random.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- .gitignore
- pkg/blockchain/registryAdmin.go
- pkg/testutils/random.go
- pkg/testutils/blockchain.go
- pkg/db/queries/models.go
- pkg/migrations/00003_add-latest-block.up.sql
- pkg/blockchain/signer.go
- pkg/indexer/indexer_test.go
- pkg/db/queries.sql
- pkg/indexer/interface.go
- contracts/config/anvil_localnet/GroupMessages.json
- pkg/sync/syncWorker.go
- pkg/indexer/indexer.go
- contracts/config/anvil_localnet/IdentityUpdates.json
🔇 Additional comments (5)
pkg/blockchain/registryAdmin_test.go (1)
58-58: LGTM! Error message standardization.The error message update aligns with the project-wide standardization of error messages (lowercase format) and provides better context.
Let's verify that this matches the actual error message in the implementation:
✅ Verification successful
✅ Error message verification successful
The error message in the test correctly matches the implementation in
registryAdmin.go, where it's used to validate owner addresses.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the corresponding error message in the implementation rg -l "invalid owner address provided" --type goLength of output: 119
Script:
#!/bin/bash # Get the context of the error message in the implementation rg "invalid owner address provided" pkg/blockchain/registryAdmin.go -B 2 -A 2Length of output: 191
pkg/indexer/blockTracker.go (2)
57-61: LGTM! Thread-safe implementation.The method correctly acquires the mutex lock to safely access the hash field, addressing potential data race concerns.
97-102: LGTM! Clean DB update implementation.The method correctly updates both block number and hash in the database.
pkg/indexer/blockTracker_test.go (1)
38-63: LGTM! Comprehensive test coverage.The tests thoroughly verify both block number and hash updates, including:
- Edge cases (lower/same block numbers)
- Persistence verification
- Concurrent updates
- Multiple contract addresses
Also applies to: 87-91, 101-104, 138-141, 149-152
pkg/mocks/indexer/mock_IBlockTracker.go (1)
24-69: LGTM! Correctly generated mock implementation.The mock implementation correctly reflects the interface changes with proper method signatures and type handling.
Also applies to: 71-89, 116-126, 142-144
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/indexer/blockTracker.go (2)
67-101: Well-structured update logic with proper validations!Good improvements:
- Double-check pattern for thread safety
- Hash validation before DB update
- Empty hash validation using custom error
Consider adding a comment explaining the double-check pattern for future maintainers.
// Quick check without lock if block <= bt.latestBlock.number.Load() { return nil } + + // Double-check pattern: re-verify after acquiring lock to prevent unnecessary DB updates bt.mu.Lock() defer bt.mu.Unlock()
116-142: Good implementation of block loading logic!Consider wrapping the negative block number error with a custom error type for better error handling across the package.
+// ErrInvalidBlockNumber indicates that a negative block number was encountered +var ErrInvalidBlockNumber = errors.New("invalid block number") func loadLatestBlock(...) (*Block, error) { // ... if latestBlock.BlockNumber < 0 { - return block, fmt.Errorf( - "invalid block number %d for contract %s", - latestBlock.BlockNumber, - contractAddress, - ) + return block, fmt.Errorf("%w: %d for contract %s", + ErrInvalidBlockNumber, + latestBlock.BlockNumber, + contractAddress) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/db/queries.sql(1 hunks)pkg/indexer/blockTracker.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test (Node)
- GitHub Check: Lint-Go
🔇 Additional comments (5)
pkg/indexer/blockTracker.go (3)
28-35: Well-structured Block type and error definition!Good use of atomic.Uint64 for thread-safe block number access and common.Hash for standardized hash representation.
57-64: Clean implementation of getter methods!Good thread-safety practices:
- Using atomic operations for block number
- Proper mutex protection for hash access
105-110: Clean DB update implementation!Correctly handles the new block hash parameter in the database update.
pkg/db/queries.sql (2)
107-114: Robust block update logic with reorg detection!Good improvements:
- Added block hash tracking
- Update condition prevents unnecessary updates
- Hash inequality check helps detect potential chain reorganizations
117-119: Clean SELECT query update!Correctly retrieves both block number and hash to support the enhanced block tracking functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/indexer/blockTracker.go (1)
63-99: Consider optimizing the hash comparison logic.The function has good validation checks but could be optimized by moving the hash comparison before acquiring the mutex lock.
func (bt *BlockTracker) UpdateLatestBlock( ctx context.Context, block uint64, hashBytes []byte, ) error { + newHash := common.Hash(hashBytes) + if newHash == (common.Hash{}) { + return ErrEmptyBlockHash + } + // Quick check without lock if block <= bt.latestBlock.number.Load() { return nil } bt.mu.Lock() defer bt.mu.Unlock() // Re-check after acquiring lock if block <= bt.latestBlock.number.Load() { return nil } - newHash := common.Hash(hashBytes) - - if newHash == (common.Hash{}) { - return ErrEmptyBlockHash - } - if newHash == bt.latestBlock.hash { return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pkg/db/queries.sql(1 hunks)pkg/db/queries/queries.sql.go(2 hunks)pkg/indexer/blockTracker.go(3 hunks)pkg/indexer/blockTracker_test.go(6 hunks)pkg/indexer/indexer.go(4 hunks)pkg/indexer/interface.go(1 hunks)pkg/mocks/indexer/mock_IBlockTracker.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/indexer/interface.go
- pkg/indexer/blockTracker_test.go
- pkg/indexer/indexer.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint-Go
- GitHub Check: Test (Node)
🔇 Additional comments (8)
pkg/indexer/blockTracker.go (4)
28-31: LGTM! Good encapsulation of block data.The
Blockstruct provides a clean encapsulation of block data with proper use of atomic operations for the block number.
33-35: LGTM! Clear error definition.The error variable is well-defined and follows Go's error handling conventions.
57-60: LGTM! Thread-safe implementation.The
GetLatestBlockmethod correctly uses mutex synchronization to prevent data races.
113-138: LGTM! Proper initialization and error handling.The
loadLatestBlockfunction properly initializes theBlockstruct and handles potential errors.pkg/db/queries.sql (2)
107-114: LGTM! Proper block hash handling with reorg protection.The query correctly handles block hash updates and includes protection against chain reorganizations by validating both block number and hash.
118-119: LGTM! Complete block data retrieval.The query retrieves both block number and hash, providing complete block information.
pkg/mocks/indexer/mock_IBlockTracker.go (1)
Line range hint
25-138: LGTM! Auto-generated mock implementation.The mock implementation correctly reflects the interface changes.
pkg/db/queries/queries.sql.go (1)
Line range hint
80-406: LGTM! Auto-generated database access code.The generated code correctly implements the SQL queries with proper type handling.
pkg/testutils/blockchain.go
Outdated
| "github.com/ethereum/go-ethereum/common" | ||
| ) | ||
|
|
||
| func IntToHash(x int64) common.Hash { |
There was a problem hiding this comment.
Maybe Int64ToHash in case we need other functions for other types of ints?
There was a problem hiding this comment.
Makes sense, renamed in the last commit.
Changelog
blockTrackeraware of block hashes. This is the first step to enhance the indexer.Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
New Features
Improvements
Chores