Ensure consistency of the Logs DB #6755
Conversation
📝 WalkthroughWalkthroughThis pull request introduces enhancements to the log indexing and database management system. The changes focus on improving log origin validation and consistency checks across multiple components. A new database migration is added to create a Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
db/sql/src/logs.rs (3)
349-400: Optimize existence checks usingexistsinstead ofcount.Using
countqueries to check for the existence of records can be less efficient than usingexists. Consider usingexiststo improve performance.Consider applying the following changes:
-let log_count = Log::find() - .count(tx.as_ref()) - .await - .map_err(|e| DbError::from(DbSqlError::from(e)))?; +let log_exists = Log::find() + .exists(tx.as_ref()) + .await + .map_err(|e| DbError::from(DbSqlError::from(e)))?; -let log_topic_count = LogTopicInfo::find() - .count(tx.as_ref()) - .await - .map_err(|e| DbError::from(DbSqlError::from(e)))?; +let log_topic_exists = LogTopicInfo::find() + .exists(tx.as_ref()) + .await + .map_err(|e| DbError::from(DbSqlError::from(e)))?;
391-392: Correct condition in existence check.In the condition
if log_topic_count != 1, it's more accurate to checkif log_topic_count == 0to detect missing address-topic pairs, as there should not be multiple identical entries.Consider changing the condition:
-if log_topic_count != 1 { +if log_topic_count == 0 {
865-885: Enhance test assertions to verify specific errors.In the test function, verify that the errors returned are specifically
DbError::InconsistentLogsto ensure the test checks for the correct error type.Consider modifying the test:
-db.ensure_logs_origin(vec![(addr_1, topic_2)]) - .await - .expect_err("expected error due to inconsistent logs in the db"); +let result = db.ensure_logs_origin(vec![(addr_1, topic_2)]).await; +assert!(matches!(result, Err(DbError::InconsistentLogs)), "Expected InconsistentLogs error");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
chain/indexer/src/block.rs(1 hunks)db/api/Cargo.toml(1 hunks)db/api/src/errors.rs(1 hunks)db/api/src/logs.rs(1 hunks)db/migration/src/lib.rs(2 hunks)db/migration/src/m20250107_000019_logs_meta_table.rs(1 hunks)db/sql/Cargo.toml(1 hunks)db/sql/src/logs.rs(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- db/api/Cargo.toml
- db/sql/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: hopli / docker
- GitHub Check: hoprd / docker
- GitHub Check: Docs / Rust docs
- GitHub Check: tests-unit-nightly
- GitHub Check: tests-unit
- GitHub Check: tests-smart-contracts
- GitHub Check: Linter
🔇 Additional comments (7)
db/sql/src/logs.rs (1)
16-17: LGTM!The added imports for
LogTopicInfoare appropriate.db/api/src/errors.rs (1)
16-18: LGTM!The new
InconsistentLogserror variant is appropriately added to handle log inconsistency errors.db/migration/src/m20250107_000019_logs_meta_table.rs (3)
8-39: LGTM! Well-designed schema for Ethereum log tracking.The table schema is well-structured with appropriate column sizes for Ethereum addresses (40 chars) and topics (64 chars). The unique index on (Address, Topic) correctly enforces the business requirement of unique address-topic pairs.
41-54: LGTM! Proper cleanup order in down migration.The down migration correctly drops the index before dropping the table, following database best practices.
57-65: LGTM! Clear and well-documented enum.The
LogTopicInfoenum provides type-safe identifiers with helpful documentation for each field.db/api/src/logs.rs (1)
4-4: LGTM! Well-designed trait extension with clear documentation.The new
ensure_logs_originmethod is well-documented and aligns perfectly with the PR objectives. The method signature correctly usesVec<(Address, Hash)>for address-topic pairs.Let's verify that all implementations of this trait are updated:
Also applies to: 10-21
✅ Verification successful
Implementation verified and properly tested
The trait is implemented in
db/sql/src/logs.rsforHoprDbwith proper error handling and transaction support. The implementation is also well-tested with both success and error cases.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all implementations of HoprDbLogOperations to ensure they're updated ast-grep --pattern 'impl $_ for $_ { $$$ async fn ensure_logs_origin($$$) { $$$ } $$$ }'Length of output: 102
Script:
#!/bin/bash # First, find the trait definition to confirm its name rg "trait\s+HoprDbLogOperations" -A 2 # Then find all implementations rg "impl.*HoprDbLogOperations.*for"Length of output: 418
Script:
#!/bin/bash # Check the implementation file for the ensure_logs_origin method rg "ensure_logs_origin" db/sql/src/logs.rs -B 1 -A 10Length of output: 950
db/migration/src/lib.rs (1)
21-21: LGTM! Proper registration of the new migration.The migration is correctly registered in
MigratorChainLogs, following the documented separation of logs from the main database.Also applies to: 135-135
This PR adds a new table into the Logs DB, that collects smart contract topics assignments that were used to sync the database.
If the DB is empty, it populates it with the given array of smart contract topics. Once the DB is not empty, it checks each time that Indexer starts, that the SC topics in the DB are consistent with what the user attempts to sync.
Closes #6660