Multi relay support

[tompro]

User description

📝 Description

Derive relay set from our nostr contacts with upper limit. We only connect to trusted and participant relays for now. Try to have at least one relay per contact (but try to fit all). Update relays on contact add. I need a bunch of relays to actually test all behaviors (especially the max total).

Relates to #613

---

✅ Checklist

Please ensure the following tasks are completed before requesting a review:

• My code adheres to the coding guidelines of this project.
• I have run cargo fmt.
• I have run cargo clippy.
• I have added or updated tests (if applicable).
• All CI/CD steps were successful.
• I have updated the documentation (if applicable).
• I have checked that there are no console errors or warnings.
• I have verified that the application builds without errors.
• I've described the changes made to the API. (modification, addition, deletion).

---

🚀 Changes Made

• New Features:
  • Active relay management

---

📋 Review Guidelines

Please focus on the following while reviewing:

• Does the code follow the repository's contribution guidelines?
• Are there any potential bugs or performance issues?
• Are there any typos or grammatical errors in the code or comments?

---

PR Type

Enhancement

---

Description

• Add dynamic relay management based on Nostr contacts

  • Implement relay calculation algorithm prioritizing trusted contacts
  • Add get_all() method to NostrContactStoreApi for fetching all contacts
  • Trigger relay refresh when contacts are added or updated

• Add max_relays configuration field with default of 50

  • Limit contact relays while always including user-configured relays

• Implement comprehensive relay calculation and update logic

  • Pass 1: Include all user relays (exempt from limit)
  • Pass 2: Add first relay from each trusted/participant contact
  • Pass 3: Fill remaining slots with additional contact relays

• Add extensive test coverage for relay calculation algorithm

  • 10 test cases covering edge cases and priority scenarios

---

Diagram Walkthrough

flowchart LR
  A["NostrConfig<br/>max_relays field"] --> B["NostrClient<br/>refresh_relays()"]
  C["NostrContactStore<br/>get_all()"] --> B
  B --> D["calculate_relay_set_internal<br/>algorithm"]
  D --> E["update_relays()<br/>add/remove relays"]
  F["NostrContactProcessor<br/>upsert_contact()"] --> B

Loading

File Walkthrough

	Relevant files
Configuration changes	2 files lib.rs Add max_relays config field with default value                      +14/-1    lib.rs Add nostr_max_relays config field to WASM Config                  +2/-0
Tests	3 files mod.rs Add get_all() to mock and update test config                          +33/-0    mod.rs Add get_all() to mock NostrContactStore implementation      +1/-0      test_utils.rs Add get_all() to mock and update test config                          +2/-0
Enhancement	6 files nostr.rs Add get_all() method to NostrContactStoreApi trait              +2/-0      nostr_contact_store.rs Implement get_all() to fetch all contacts from store          +10/-0    nostr_contact_processor.rs Add relay refresh trigger on contact updates                          +16/-2    lib.rs Pass contact store and trigger initial relay refresh          +13/-1    nostr.rs Implement relay calculation and refresh logic with tests  +371/-9  context.rs Pass contact store to create_nostr_clients function            +7/-2

---

[Copilot]

Pull request overview

This PR implements dynamic multi-relay support by deriving relay connections from Nostr contacts with an upper limit. The system now connects to trusted and participant contact relays in addition to user-configured relays, automatically refreshing connections when contacts are added.

Key Changes

• Added max_relays configuration field to limit total relay connections (defaults to 50)
• Implemented relay calculation algorithm prioritizing trusted contacts over participants
• Added automatic relay refresh on contact addition/update
• Added get_all() method to NostrContactStoreApi for retrieving all contacts

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
crates/bcr-ebill-api/src/lib.rs	Added max_relays field to NostrConfig with default implementation
crates/bcr-ebill-wasm/src/lib.rs	Hardcoded max_relays to 50 in WASM initialization
crates/bcr-ebill-wasm/src/context.rs	Updated NostrClient creation to pass nostr_contact_store
crates/bcr-ebill-persistence/src/nostr.rs	Added get_all() method to NostrContactStoreApi trait
crates/bcr-ebill-persistence/src/db/nostr_contact_store.rs	Implemented get_all() method for SurrealDB store
crates/bcr-ebill-transport/src/lib.rs	Added initial relay refresh on client creation and passed contact store
crates/bcr-ebill-transport/src/nostr.rs	Implemented relay calculation, refresh logic, and comprehensive tests
crates/bcr-ebill-transport/src/handler/nostr_contact_processor.rs	Added relay refresh trigger on contact upsert
crates/bcr-ebill-transport/src/handler/mod.rs	Updated mock to include new get_all() method
crates/bcr-ebill-transport/src/test_utils.rs	Updated test config and mocks with new get_all() method
crates/bcr-ebill-api/src/tests/mod.rs	Added config tests and updated mocks
.gitignore	Added .worktrees/ directory for git worktrees

[codecov]

Codecov Report

❌ Patch coverage is 24.09639% with 63 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/bcr-ebill-transport/src/nostr.rs	20.68%	46 Missing ⚠️
...cr-ebill-persistence/src/db/nostr_contact_store.rs	0.00%	8 Missing ⚠️
...l-transport/src/handler/nostr_contact_processor.rs	0.00%	8 Missing ⚠️
crates/bcr-ebill-transport/src/lib.rs	0.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Untrusted relay connections Description: The relay refresh mechanism dynamically adds and removes relays based on contact data without validating relay URLs or enforcing an allowlist, which could connect clients to untrusted or malicious relays and expose metadata or messages. nostr.rs [334-401] Referred Code /// Calculate the complete relay set from user relays + contact relays async fn calculate_relay_set(&self) -> Result<HashSet<url::Url>> { // Get contacts from store if available let contacts = if let Some(store) = &self.nostr_contact_store { store.get_all().await.map_err(|e| { error!("Failed to fetch contacts for relay calculation: {e}"); Error::Message("Failed to fetch contacts".to_string()) })? } else { vec![] }; Ok(calculate_relay_set_internal( &self.relays, &contacts, self.max_relays, )) } /// Update the client's relay connections to match the target set async fn update_relays(&self, target_relays: HashSet<url::Url>) -> Result<()> { ... (clipped 47 lines)
	Relay removal risk Description: Removing relays based on computed differences might inadvertently disconnect from essential user-configured relays if calculation logic or contact data is compromised; lacks a safeguard to always keep user-configured relays. nostr.rs [357-386] Referred Code // Get current relays let current_relays: HashSet<url::Url> = client .relays() .await .into_iter() .filter_map(|(_, relay)| relay.url().as_str().parse::<url::Url>().ok()) .collect(); // Add new relays for relay in target_relays.iter() { if !current_relays.contains(relay) { match client.add_relay(relay).await { Ok(_) => debug!("Added relay: {}", relay), Err(e) => warn!("Failed to add relay {}: {}", relay, e), } } } // Remove old relays (relays not in target set) for relay in current_relays.iter() { if !target_relays.contains(relay) { ... (clipped 9 lines)
Ticket Compliance
🟡	🎫 #613 🟢 System must support multiple relays per user, beyond a single default relay. Incorporate relay management derived from contacts while prioritizing trusted/participant relays. Provide configuration to limit the number of relays, with sensible defaults. Update relay connections dynamically when contacts are added or updated. 🔴 Notifications should work when users have multiple relays, with a way to designate or handle a notification relay. File uploads must not rely solely on the first relay; need to handle multiple relays appropriately. Implement or plan for replication between relays. ⚪ None
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing audit logs: Newly added relay management actions (adding/removing relays and refreshing) are not auditable beyond debug/warn/info logs and do not record which user/identity triggered the action, making it unclear who initiated changes to relay connections. Referred Code /// Calculate the complete relay set from user relays + contact relays async fn calculate_relay_set(&self) -> Result<HashSet<url::Url>> { // Get contacts from store if available let contacts = if let Some(store) = &self.nostr_contact_store { store.get_all().await.map_err(|e| { error!("Failed to fetch contacts for relay calculation: {e}"); Error::Message("Failed to fetch contacts".to_string()) })? } else { vec![] }; Ok(calculate_relay_set_internal( &self.relays, &contacts, self.max_relays, )) } /// Update the client's relay connections to match the target set async fn update_relays(&self, target_relays: HashSet<url::Url>) -> Result<()> { ... (clipped 47 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Generic error wrap: In relay set calculation, contact fetch errors are mapped to a generic message "Failed to fetch contacts" losing actionable context like store error kind which may hinder debugging and edge-case handling. Referred Code // Get contacts from store if available let contacts = if let Some(store) = &self.nostr_contact_store { store.get_all().await.map_err(|e| { error!("Failed to fetch contacts for relay calculation: {e}"); Error::Message("Failed to fetch contacts".to_string()) })? } else { vec![] }; Ok(calculate_relay_set_internal( &self.relays, &contacts, self.max_relays, )) } Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Consider a more robust relay management strategy To prevent network instability from excessive connection churn, the relay list refresh triggered by contact updates should be improved. Consider debouncing the refresh calls or adopting a less disruptive update strategy. Examples: crates/bcr-ebill-transport/src/handler/nostr_contact_processor.rs [86-91] // Trigger relay refresh to include new contact's relays if let Some(ref client) = self.nostr_client && let Err(e) = client.refresh_relays().await { warn!("Failed to refresh relays after contact update for {node_id}: {e}"); } crates/bcr-ebill-transport/src/nostr.rs [392-401] pub async fn refresh_relays(&self) -> Result<()> { info!("Refreshing relay connections based on contacts"); let relay_set = self.calculate_relay_set().await?; self.update_relays(relay_set).await?; info!( "Relay refresh complete, connected to {} relays", self.client.relays().await.len() ); Ok(()) } Solution Walkthrough: Before: class NostrContactProcessor { // ... async fn upsert_contact(&self, node_id, contact) { // ... save contact to store // Trigger relay refresh immediately on every update if let Some(ref client) = self.nostr_client { if let Err(e) = client.refresh_relays().await { warn!("Failed to refresh relays: {e}"); } } } } After: class NostrContactProcessor { // ... async fn upsert_contact(&self, node_id, contact) { // ... save contact to store // Schedule a debounced relay refresh instead of immediate execution if let Some(ref client) = self.nostr_client { // This new function would handle the debouncing logic, // executing the refresh only once after a series of rapid calls. client.schedule_relay_refresh(); } } } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that calling refresh_relays() on every contact update can cause excessive network churn and proposes a valid improvement like debouncing to enhance stability and performance.	Medium
Possible issue	✅ Avoid data loss on single failure Suggestion Impact: The commit replaced map+collect::>() with filter_map to skip only failed conversions and added logging of conversion errors; it also imported log::error. code diff: +use log::error; use serde::{Deserialize, Serialize}; use surrealdb::sql::Thing; @@ -70,9 +71,15 @@ let result: Vec<NostrContactDb> = self.db.select_all(Self::TABLE).await?; let values = result .into_iter() - .map(|c| c.to_owned().try_into().ok()) - .collect::<Option<Vec<NostrContact>>>(); - Ok(values.unwrap_or_default()) + .filter_map(|c| match c.try_into() { + Ok(v) => Some(v), + Err(e) => { + error!("Failed to convert NostrContactDb to NostrContact: {e}"); + None + } + }) + .collect::<Vec<NostrContact>>(); + Ok(values) In get_all, use filter_map instead of map and collect::<Option<...>> to prevent a single contact conversion failure from discarding the entire list of contacts. Log errors for failed conversions. crates/bcr-ebill-persistence/src/db/nostr_contact_store.rs [69-76] async fn get_all(&self) -> Result<Vec<NostrContact>> { let result: Vec<NostrContactDb> = self.db.select_all(Self::TABLE).await?; let values = result .into_iter() - .map(|c| c.to_owned().try_into().ok()) - .collect::<Option<Vec<NostrContact>>>(); - Ok(values.unwrap_or_default()) + .filter_map(|c| match c.try_into() { + Ok(contact) => Some(contact), + Err(e) => { + log::error!("Failed to convert NostrContactDb to NostrContact: {}", e); + None + } + }) + .collect(); + Ok(values) } [Suggestion processed] Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a critical flaw where a single malformed contact record would cause all contacts to be discarded, impacting the new relay connection logic. The proposed fix using filter_map is robust and appropriate.	Medium
Learned best practice	Fail fast on relay refresh error Treat a failed initial relay refresh as an initialization error and return early so the caller can decide; don't silently continue in a potentially incorrect state. crates/bcr-ebill-transport/src/lib.rs [110-114] // Initial relay refresh to include contact relays if let Err(e) = client.refresh_relays().await { warn!("Failed initial relay refresh: {}", e); - // Continue anyway - we have user relays at minimum + return Err(Error::Message("Failed initial relay refresh".into())); } Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Early-return on invalid or irrelevant states; handle initialization failures explicitly to avoid proceeding in degraded state.	Low
Update

[zupzup]

LGTM 👍

[codingpeanut157]

minor comments left on the code

LGTM