Trick to avoid getting older events republishe #378
Conversation
…dwith requesting less events
WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Poem
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 review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/main.rs(1 hunks)
🔇 Additional comments (2)
src/main.rs (2)
Line range hint 75-82: LGTM: Improved error handling with detailed logging.
Good improvement in error handling by including the actual error value in the log message. This will help with debugging issues during client initialization.
72-73: Verify the impact of removing timestamp-based filtering.
The change from timestamp-based filtering to limit(0) raises some concerns:
- Setting
limit(0)is an unusual pattern. While it might prevent older events from being republished, it could potentially affect the retrieval of new events. - Removing the timestamp filter (
since(Timestamp::now() - 172800)) means we're no longer explicitly filtering by time window.
Let's verify the behavior of limit(0) in the Nostr protocol:
Consider these alternative approaches:
- Use a combination of
limit(n)withsince(timestamp)to get a controlled number of recent events - Implement event deduplication logic if duplicate events are the core issue
- Document the rationale behind using
limit(0)if it's proven to be the best solution
Would you like me to propose a specific implementation for any of these alternatives?
✅ Verification successful
The previous results revealed some important context about time-based filtering in app.rs, but we need to see the complete implementation to understand how limit(0) works with the overall event processing logic, especially given the 10-second age check in app.rs. This will help us verify if the removal of timestamp-based filtering in main.rs is actually compensated by other mechanisms.
Setting limit(0) with Kind::GiftWrap is safe due to existing time-based filtering
The removal of timestamp-based filtering in main.rs is actually compensated by a robust 10-second age check in the event processing pipeline:
- Events are filtered by
Kind::GiftWrapandlimit(0)at subscription level - Each received event goes through multiple validations:
- POW verification
- Signature verification
- 10-second age check:
event.rumor.created_at.as_u64() < since_time
The limit(0) approach is acceptable here because:
- It prevents initial backfilling of historical events
- Recent events are still processed due to real-time relay propagation
- The 10-second age check in
app.rsprovides a strict time-window filter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any similar patterns or documentation about limit(0)
rg -A 5 "limit\(0\)"
# Look for any tests or documentation about GiftWrap event handling
rg -A 10 "Kind::GiftWrap"
# Check if there are any comments or documentation about event filtering
rg -A 5 "since\(Timestamp::"
Length of output: 2210
Script:
#!/bin/bash
# Let's check the app.rs file for event handling logic and time-based filtering
cat src/app.rs
# Check for any documentation about the subscription filter changes
cat src/main.rs
# Look for any tests related to event filtering
fd "test" --exec cat {}
Length of output: 14875
@grunch @bilthon
playing a bit I came out with this, could be the simpler idea to do what we need.
Using
limitset to 0 we can avoid to get stored older events, while still getting new ones with notifications.Take a look!
Summary by CodeRabbit
New Features
Bug Fixes
Documentation