db: Fix channel/ticket state mismatch on startup #6930
Conversation
📝 WalkthroughWalkthroughThis pull request performs a minor version update in the Cargo manifest and modifies the caching mechanism in the SQL database module. The cache key for Changes
Sequence Diagram(s)sequenceDiagram
participant TM as TicketManager
participant C as Cache (HoprDbCaches)
participant TO as TicketOperations
TM->>C: Insert ticket (compute channel_epoch, then call insert((channel, epoch), updated_value))
C-->>TM: Confirm updated cache entry
TM->>C: Retrieve unrealized value using key (channel, epoch)
C-->>TM: Return cached unrealized value
TO->>C: Invalidate cache entry using key (channel, epoch)
C-->>TO: Confirm invalidation
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 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
|
|
@NumberFour8 's solution is better, but this one would work as well. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_websocket_api.py (1)
214-215: Enhanced timeout and logging for deprecated test.Significantly increasing the timeout from 5 to 1000 seconds and adding message logging will help in debugging the websocket connection issues mentioned in the xfail marker. Be aware that such a long timeout could potentially cause test runs to hang if messages aren't received.
Consider adding a more descriptive log prefix like "DEPRECATED TEST:" to make these log entries more identifiable in the logs.
tests/network-simulation/hoprd.cfg (1)
1-34: Document the purpose of this configuration file.This is a new configuration file for network simulation testing. Consider adding a comment at the top of the file explaining its purpose, especially its relation to the ticket state mismatch fix that this PR addresses.
+# Configuration file for network simulation tests +# Used to test the fix for channel/ticket state mismatch on startup +# See PR #6930 for details hopr: db:
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
hoprd/hoprd/src/main.rs(1 hunks)tests/network-simulation/hoprd.cfg(1 hunks)tests/test_websocket_api.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: hoprd / docker
- GitHub Check: hopli / docker
- GitHub Check: tests-smart-contracts
- GitHub Check: Docs / Rust docs
- GitHub Check: tests-unit
- GitHub Check: tests-smoke-websocket
- GitHub Check: tests-unit-nightly
- GitHub Check: Linter
🔇 Additional comments (8)
hoprd/hoprd/src/main.rs (1)
358-359: Enhanced logging for inactive websocket state.This trace log provides better visibility into the application's websocket state when there are no active connections, complementing the existing error log that fires when websocket notifications fail. This is a good debugging improvement that helps understand messaging flow.
tests/test_websocket_api.py (3)
9-9: Appropriate import for new logging functionality.Adding the logging import supports the new debug message functionality added below.
183-183: Minor type clarification.Changing from integer to floating-point representation is functionally equivalent but makes the type more explicit.
190-190: Good use of xfail marker for known issues.Adding an xfail marker with a clear explanation properly documents that this test is expected to fail due to known websocket connection issues. This prevents the test from causing confusing failures in CI pipelines.
tests/network-simulation/hoprd.cfg (4)
24-24: Add private key configuration or document that it's intentionally empty.The
private_keyfield is empty. If this is intentional for security or testing purposes, consider adding a comment. Otherwise, ensure this field is properly configured for the testing environment.
9-9: Clarify if empty strategies array is intentional.The
strategiesarray is empty. Is this intentional? If specific strategies are needed for the testing environment, they should be added here.
13-14: Verify that these settings support the ticket state mismatch fix.The PR aims to fix a channel/ticket state mismatch, particularly related to unrealized balance across channel epochs. Ensure that the
network: localandcheck_unrealized_balance: truesettings properly support testing this fix.
28-28:❓ Verification inconclusive
Verify YAML syntax compatibility.
The use of
!IPv4tag for IP address specification may not be compatible with all YAML parsers. Ensure that the parser used in your application supports this custom tag, or consider using a standard string representation.
🏁 Script executed:
#!/bin/bash # Check if the YAML parser used in the application supports the !IPv4 tag # First let's find where this config is being loaded echo "Searching for files that load hoprd.cfg..." rg -l "hoprd\.cfg" --type rustLength of output: 127
Action Required: Manually Verify YAML Parser Custom Tag Support
The configuration file at
tests/network-simulation/hoprd.cfg(line 28) uses the custom YAML tag!IPv4for the IP address. Our initial search for references tohoprd.cfgin the Rust codebase did not yield any hits, so we couldn’t automatically determine whether the YAML parser in use supports this tag.
- Verification Needed: Please manually confirm that the YAML parser (likely invoked via libraries like
serde_yamlor similar) correctly recognizes and processes the!IPv4tag without causing errors.- Alternative Suggestion: If parser support for custom YAML tags is not implemented or verified, consider changing the IP address specification to a standard string format (e.g.,
"0.0.0.0") to ensure compatibility.
Due to a race-condition the node could lose track of the on-chain status of a to-be-redeemed ticket. Therefore, now it checks on restart if any ticket is marked as
BeingRedeemedand matches the next expected ticket index of its channel. If so, its being reset toUntouched.Also:
Refs #6926