feat: add Nostr trusted-node price provider (#697) - #841
Conversation
Lets mostrod source BTC/fiat quotes by subscribing to kind-30078 rate events from trusted Mostro nodes over Nostr instead of an HTTP API, for operators in regions where price APIs are network-blocked (e.g. Yadio DNS-blocked in Venezuela/Cuba). Reuses the process-wide Nostr client already connected to [nostr]'s relays; with several trusted_nodes configured, the freshest valid event wins over a cross-node combine. Also scopes ProviderConfig::validate()'s trusted_nodes exemption to the nostr provider only, so it can no longer mask a missing url on any other provider.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a ChangesNostr price provider
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PriceManager
participant NostrProvider
participant NostrClient
participant TrustedNodeRelays
PriceManager->>NostrProvider: fetch()
NostrProvider->>NostrClient: query kind-30078 events
NostrClient->>TrustedNodeRelays: filter trusted authors and mostro-rates
TrustedNodeRelays-->>NostrClient: matching events
NostrClient-->>NostrProvider: event list
NostrProvider-->>PriceManager: freshest valid quotes
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/price/providers/nostr.rs (2)
24-28: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
RELAY_QUERY_TIMEOUTis decoupled from the sharedprovider_timeout_seconds.This 8s constant is independent of the operator-configurable
provider_timeout_seconds(default 10, nofallback_urlsfor this provider ⇒ manager'spoll_budget=provider_timeout_seconds + 1). At the default that's only ~3s of slack; if an operator lowersprovider_timeout_secondsglobally (e.g. to speed up other HTTP providers) below ~9s, this provider's ticks would consistently hit the manager's outer timeout instead of completing (or gracefully erroring) on their own — which is especially unfortunate given this provider exists specifically for higher-latency/blocked-network operators.Consider deriving the relay-query timeout from the provider's own budget (or documenting/enforcing a minimum safe
provider_timeout_secondswhennostris enabled) rather than a fixed constant untied to the shared knob.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/price/providers/nostr.rs` around lines 24 - 28, Update the Nostr provider timeout configuration around RELAY_QUERY_TIMEOUT so the relay query derives from the shared provider_timeout_seconds budget and retains sufficient outer-timeout slack, rather than using a fixed 8-second duration. Ensure lowered global budgets do not cause fetch() to be consistently terminated by PriceManager::poll_budget before the provider completes or reports its own error.
102-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
fetch()'s filter construction is untested outside the ignored live-relay test.The
kind/authors/identifierfilter shape and the relay-error/no-event mapping infetch()are only exercised bylive_relay_fetch_returns_real_rates, which is#[ignore]d and never runs in CI. A regression here (wrong kind constant, wrongidentifier, etc.) would go unnoticed until a manual run against a live relay.Consider extracting the
Filter::new()...construction into a smallpub(crate) fn build_filter(&self) -> Filter(mirroringparse_content/select_freshest's testability split), so its shape can be asserted in a plain unit test without a relay.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/price/providers/nostr.rs` around lines 102 - 130, The fetch() filter construction is not covered by runnable unit tests. Extract the existing Filter::new() chain from NostrProvider::fetch into a pub(crate) build_filter(&self) -> Filter method, preserving the kind, trusted-node authors, and "mostro-rates" identifier values, then update fetch() to use it and add a non-live unit test asserting the filter shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/price/providers/nostr.rs`:
- Around line 24-28: Update the Nostr provider timeout configuration around
RELAY_QUERY_TIMEOUT so the relay query derives from the shared
provider_timeout_seconds budget and retains sufficient outer-timeout slack,
rather than using a fixed 8-second duration. Ensure lowered global budgets do
not cause fetch() to be consistently terminated by PriceManager::poll_budget
before the provider completes or reports its own error.
- Around line 102-130: The fetch() filter construction is not covered by
runnable unit tests. Extract the existing Filter::new() chain from
NostrProvider::fetch into a pub(crate) build_filter(&self) -> Filter method,
preserving the kind, trusted-node authors, and "mostro-rates" identifier values,
then update fetch() to use it and add a non-live unit test asserting the filter
shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bf834974-afcf-4195-9b1e-03d2452244a2
📒 Files selected for processing (12)
docs/PRICE_PROVIDERS.mdsettings.tpl.tomlsrc/price/config.rssrc/price/manager.rssrc/price/provider.rssrc/price/providers/blockchain.rssrc/price/providers/coingecko.rssrc/price/providers/currency_api.rssrc/price/providers/eltoque.rssrc/price/providers/mod.rssrc/price/providers/nostr.rssrc/price/providers/yadio.rs
…stroP2P#697) CodeRabbit review on PR MostroP2P#841 flagged two nitpicks: - RELAY_QUERY_TIMEOUT was a fixed 8s constant, decoupled from the operator-configurable provider_timeout_seconds. Lowering that setting below ~9s would make the nostr provider's ticks consistently hit PriceManager::poll_budget's outer timeout instead of completing or failing on their own. NostrProvider now derives query_timeout from provider_timeout_seconds (threaded through build_provider), matching the 1s of slack every other provider already gets. - fetch()'s Filter construction was only exercised by the #[ignore]d live-relay test. Extracted into NostrProvider::build_filter(), unit-tested for kind/authors/identifier without touching a relay.
|
Pushed `b7d5e4c` addressing both CodeRabbit nitpicks:
`cargo build`, `cargo test` (11 passed, 1 ignored), `cargo clippy --all-targets --all-features -- -D warnings`, `cargo fmt --check`: all clean. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/price/providers/nostr.rs`:
- Line 136: Update the event-processing flow around select_freshest and the
fetch_events call to remove trusted mostro-rates events whose NIP-40 expiration
tag is in the past before selecting the newest event. Preserve trusted-author
and created_at selection behavior for non-expired events, and add coverage for a
newest expired event versus an older valid event and for trusted nodes with only
expired events.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c4eed9e2-8d37-4ffa-a1b9-b8850bdc0a46
📒 Files selected for processing (2)
src/price/manager.rssrc/price/providers/nostr.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/price/manager.rs
Discard trusted-node kind-30078 events older than max_price_staleness_seconds so zombie relay data cannot refresh the local cache clock. Co-authored-by: Cursor <cursoragent@cursor.com>
Maintainer follow-up: discard stale Nostr rate eventsPushed Problem. Fix. Wiring. Tests. Unit coverage for discard-older, all-stale → none, and exact max-age boundary. |
|
@ToRyVand tACK and good job! I posted a fix to avoid staleness of nostr event that was not checked, take a look on your side and then for me it's green for merge. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/price/providers/nostr.rs`:
- Around line 141-146: Update the event filtering in the rate-selection flow to
reject trusted events whose created_at timestamp is later than now before
applying the max_age freshness check. Preserve selection of the newest valid
current event in max_by_key, and add a test covering a future-dated trusted
event alongside a current event, asserting the current event is selected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 19928e09-a972-48af-93e8-af55064bfa7a
📒 Files selected for processing (2)
src/price/manager.rssrc/price/providers/nostr.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/price/manager.rs
A forged or clock-skewed trusted-node event with created_at > now would saturate the age check to 0 and always win max_by_key, bypassing the staleness gate entirely. Reject created_at > now before the age check. Addresses CodeRabbit finding on PR MostroP2P#841 after arkanoider's dff055e.
|
Thanks @arkanoider — the TTL fix in CodeRabbit caught one more edge in that same commit worth fixing before merge: Pushed Should be good for another look whenever you get a chance. |
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Strict review — changes requested
Reviewed the full PR history and exact head e52f93fb32f347eedabfb33a8c3f91b380b511de.
Blocking
-
Expired NIP-40 events are still eligible.
select_freshest(src/price/providers/nostr.rs:141-149) checks only author andcreated_at; it never checksEvent::is_expired_at. The created-at TTL is not equivalent to the event's signed expiration. With the defaults, this daemon publishes rate events that expire after 600 seconds (src/price/manager.rs:523-535) while the consumer accepts theircreated_atfor 1,800 seconds, so a relay retaining expired events can keep them eligible for another 20 minutes. I reproduced this on the current head: a newer expired event wins over an older non-expired event. The existing unresolved CodeRabbit thread remains valid. Please reject expired events before selection and cover both “newest expired vs older valid” and “all expired”. -
Nostr-derived prices can be recursively republished with a fresh timestamp, bypassing staleness indefinitely. The provider discards upstream age/provenance when converting the selected event into ordinary quotes (
src/price/providers/nostr.rs:170-180); the manager then stamps the aggregate with localnowand publishes a new kind-30078 event with a newcreated_at/expiration (src/price/manager.rs:311-319,523-537). If a node trusts itself, or two relay-mode nodes trust each other, old rates can circulate and be re-signed forever during the original source outage.max_price_staleness_secondsthen never trips. Add an explicit anti-loop/provenance policy and regression coverage for self-trust and a two-node relay-only cycle.
Prior review points
- Fixed on current head: shared timeout derivation, unit-tested filter construction, stale
created_atrejection, and future-dated event rejection. - Still open: NIP-40 expiration handling, as detailed above.
Verification
cargo fmt --check— passedcargo clippy --all-targets --all-features -- -D warnings— passedcargo test price::— 123 passed, 1 ignored- Focused expiration mutation probe — failed as described (the expired event was selected)
- Checkout clean and still at the reviewed head
There was a problem hiding this comment.
Supplemental strict-review findings
The parallel audits completed after my initial review. I verified these additional blockers against the same live head, e52f93fb32f347eedabfb33a8c3f91b380b511de:
-
The newest malformed/empty event suppresses valid redundant nodes.
select_freshestchooses one event before content validation (src/price/providers/nostr.rs:135-149,170-180). If the newest trusted event has malformed JSON,fetch()returns a parse error without trying the next-newest fresh event. If its body is{"BTC":{}}or contains only unusable values, it returnsOk(empty)and still does not try another trusted node. This contradicts the documented “highestcreated_atamong the trusted, valid ones” redundancy guarantee (docs/PRICE_PROVIDERS.md:873-881). Please order eligible events newest-first and select the first one that parses to a non-empty usable quote map, with tests for malformed and empty newest events versus an older valid event. -
An already-aggregated remote event is counted as an independent local vote. The Nostr adapter discards the event's
sourceprovenance and emits one ordinaryQuote::PerBtcper currency (src/price/providers/nostr.rs:113-125). The manager then combines it equally with local HTTP providers (src/price/manager.rs:273-292;src/price/aggregate.rs:129-183). A remote aggregate can contain the same Yadio/CoinGecko inputs already present locally, double-counting correlated data and defeating the median/outlier guard. Concrete example under the current 5% rule: local[Yadio=100, Blockchain=200]aggregates to150; adding a remote aggregate derived from Yadio at100produces[100,100,200], rejects200as an outlier, and returns100. Make Nostr an explicit fallback rather than a concurrent independent sample, or retain enough upstream provenance to deduplicate overlapping contributors before aggregation. Add a regression test for this correlated-source case.
These are separate from the NIP-40 and recursive-republication blockers in the initial review.
|
@ToRyVand the bot seems ranting again, take a look and verify if it's a real issue, reading quickly they seem correct. I missed in my fixed the |
CodeRabbit flagged that select_freshest never checked a rate event's own NIP-40 expiration tag, only the max_price_staleness_seconds TTL against this node's clock. A relay that keeps serving an expired kind-30078 event past its self-declared expiry could still win max_by_key. Filter with Event::is_expired_at before selecting the freshest event.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/price/providers/nostr.rs (1)
146-154: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate candidate events before using their quotes.
select_freshestcurrently trusts any event whose pubkey matchestrusted_nodes, without rechecking the Nostr envelope (kind=30078,identifier("mostro-rates")) or event authenticity. Since the relay-side subscription filter can be spoofed or bypassed and JSON deserialization does not verify signatures, callEvent::verify()on before-selection candidates, reject malformed envelopes, and add tests for an invalid signature and wrong envelope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/price/providers/nostr.rs` around lines 146 - 154, Update select_freshest to validate each trusted candidate with Event::verify() before selecting its quote, and require the expected Nostr envelope: kind 30078 with identifier "mostro-rates". Reject candidates with invalid signatures, malformed envelopes, or mismatched metadata while preserving the existing freshness and expiration filters, and add tests covering invalid signatures and wrong envelopes.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/price/providers/nostr.rs`:
- Around line 146-154: Update select_freshest to validate each trusted candidate
with Event::verify() before selecting its quote, and require the expected Nostr
envelope: kind 30078 with identifier "mostro-rates". Reject candidates with
invalid signatures, malformed envelopes, or mismatched metadata while preserving
the existing freshness and expiration filters, and add tests covering invalid
signatures and wrong envelopes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c21c5c0f-443e-45c0-91ae-4560a51180d1
📒 Files selected for processing (1)
src/price/providers/nostr.rs
There was a problem hiding this comment.
Strict re-review — changes still requested
Reviewed the full PR history and exact current head c4938447b15d6f39627b55455264b776a2b60cac, including the delta from e52f93f.
Fixed
- NIP-40 expiration: fixed correctly.
select_freshestnow rejectsEvent::is_expired_at, and the two new tests cover newest-expired/older-valid and all-expired cases.
Still blocking
-
Nostr-derived prices can still be republished indefinitely. The consumer drops upstream age/provenance (
nostr.rs:115-125,175-185), then the manager stores and republishes the aggregate with a fresh local timestamp and expiration (manager.rs:311-319,523-537). Self-trust or an A↔B trust cycle can therefore keep an old quote fresh forever. Prevent republishing Nostr-derived-only aggregates as fresh observations, or preserve and enforce the oldest origin timestamp/provenance. -
The newest malformed/empty event still suppresses a valid redundant node. Selection happens before parsing (
nostr.rs:139-154,175-185). I reproduced this on the current head: a newer trustednot jsonevent was selected over an older valid trusted event, and the regression probe failed.{"BTC":{}}likewise returns an empty success. Rank candidates newest-first, but choose the first one that parses to a non-empty usable quote map. -
The remote aggregate is still counted as an independent local vote. The adapter drops the event's
sourcetag, andaggregate_ticktreatsProviderId::Nostras independent from local HTTP inputs (nostr.rs:115-125;manager.rs:273-292;aggregate.rs:129-193). A remote Yadio-derived100combined with local Yadio100and Blockchain200produces100instead of the local two-source result150. Make Nostr fallback-only when direct providers have usable quotes, or preserve and deduplicate complete upstream provenance. -
Kind/
dare not revalidated client-side. The expected envelope exists only inbuild_filter(nostr.rs:106-110);select_freshestdoes not checkkind == 30078ord == mostro-rates. In the lockednostr-relay-pool 0.44.1,verify_subscriptionsdefaults tofalse, andconnect_nostr()does not enable it, so a noncompliant relay can return a validly signed trusted-author event outside the requested filter. Validate the exact envelope before ranking candidates. I am not carrying CodeRabbit's signature-verification claim as a blocker: the SDK relay pipeline already callsevent.verify()before delivering newly received events.
Verification
- Branch head:
cargo fmt --checkpassed; focused Nostr tests passed (17 passed, 1 ignored); fullcargo test price::passed (125 passed, 1 ignored); Clippy with-D warningspassed. - Synthetic integration with live
origin/main(94e736a7d3fad24a548d13f4541f4af9eb44ace6): merge clean; price suite and Clippy passed. git diff --checkpassed.
The NIP-40 fix is good, but the other previously requested safety/correctness fixes are not present in this commit.
|
@ToRyVand the bot are bad guys, still ranting on other points... |
Addresses ermeme's re-review (CHANGES_REQUESTED, commit c493844): - Revalidate the event envelope (kind 30078, d=mostro-rates) client-side in rank_candidates instead of trusting the relay-side query filter — nostr-relay-pool's verify_subscriptions defaults to false, so a noncompliant relay could hand back a validly-signed event from a trusted pubkey under a different kind/d. - Split select_freshest into rank_candidates (all valid candidates, newest first) + pick_first_usable, so a malformed or empty body from the newest trusted event no longer suppresses a good, still-fresh body from an older one. - Make the Nostr provider strictly fallback-only in aggregate_tick (restrict_nostr_to_fallback): a trusted node's rate is itself already an aggregate of its own direct sources, so blending it into this node's median/mean alongside those same kinds of sources double-counts correlated information. - Stop republishing currencies whose only surviving contributor is Nostr (republishable_rates): re-stamping a relayed rate with a fresh created_at/expiration erases how old the underlying quote really is, which a trust cycle could otherwise ride indefinitely.
|
Pushed 2 & 4 (nostr.rs) — mechanical, fully closed:
3 (manager.rs) — Nostr made fallback-only: 1 (manager.rs) — republish guard, partial: Verification: |
|
tACK for me! Let's wait a pass from a bot to complete it. |
Local verification of ermeme blockers (head
|
| # | Blocker (ermeme) | Status on d5e91de |
Where |
|---|---|---|---|
| 0 | NIP-40 expired events still eligible | Fixed (already on c493844) |
rank_candidates filters !e.is_expired_at(&now); tests cover newest-expired vs older-valid and all-expired |
| 1 | Indefinite republish / trust-cycle freshness | Fixed (option A from the review) | republishable_rates drops currencies whose only surviving contributor is Nostr before publish_rates_to_nostr |
| 2 | Newest malformed/empty event suppresses a valid redundant node | Fixed | rank_candidates (newest-first) + pick_first_usable; tests for malformed, empty, and all-unusable |
| 3 | Remote aggregate counted as an independent local vote | Fixed (fallback-only) | restrict_nostr_to_fallback in aggregate_tick keeps Nostr only for currencies no other provider covered that tick |
| 4 | Kind / d not revalidated client-side |
Fixed | rank_candidates requires kind == 30078 and d == mostro-rates; tests for wrong-kind and wrong-d |
Notes
- Blocker 1 is closed via the review's first alternative ("prevent republishing Nostr-derived-only aggregates as fresh observations"), not full origin-timestamp provenance. That matches the author's stated scope split; a protocol-level origin age tag remains a reasonable follow-up for multi-hop Nostr-only operators, but it is not required to clear this review item as written.
- Blocker 3's concrete double-count example is addressed by dropping overlapping Nostr quotes before aggregation (direct sources alone decide the value when they cover the currency).
Verification run locally
cargo test price::→ 134 passed, 0 failed, 1 ignored
tACK from my side on the correctness items above — the remaining open review from ermeme[bot] looks stale relative to d5e91de.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/price/manager.rs (1)
508-515: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider recomputing the
sourcetag from the republished set.
republishable_ratesdrops Nostr-only currencies, butsuccesses(fromreport.contributorsat Line 326) still covers every aggregate. If the only currency thatNostrcontributed to is dropped here, the publishedsourcetag still namesnostrwhile no published value came from it. Deriving the tag from the surviving currencies keeps the tag consistent with the payload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/price/manager.rs` around lines 508 - 515, Update the republishing flow around republishable_rates and the published source-tag construction to derive source contributors from the surviving rates rather than the full successes set. Exclude Nostr when its contributed currencies are all removed, while preserving source tags for contributors represented in the republished payload.src/price/providers/nostr.rs (1)
275-277: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the stale
select_freshestreferences.The function is now
rank_candidates. The doc comments at Lines 276 and 294 still nameselect_freshest, and the tests at Lines 314-422 keep theselect_freshest_*prefix. Rename them torank_candidates_*so the tests point at the function they exercise.As per coding guidelines: "use descriptive test names such as `handles_expired_hold_invoice`".♻️ Example rename
- /// Like `signed_event`, but carrying a NIP-40 `expiration` tag — for - /// exercising `select_freshest`'s `is_expired_at` gate independently of - /// the `max_age` gate. + /// Like `signed_event`, but carrying a NIP-40 `expiration` tag — for + /// exercising `rank_candidates`'s `is_expired_at` gate independently of + /// the `max_age` gate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/price/providers/nostr.rs` around lines 275 - 277, Rename the stale select_freshest references in the documentation comments near the signed-event helpers to rank_candidates, and rename every select_freshest_* test in the affected test block to the rank_candidates_* prefix. Keep the test behavior unchanged and use descriptive names for each specific scenario.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/price/manager.rs`:
- Around line 591-613: Update restrict_nostr_to_fallback so currency keys from
both non-Nostr coverage and Nostr filtering are normalized to uppercase before
comparison, matching aggregate_tick behavior. Add a regression test covering a
lowercase non-Nostr currency code and an uppercase Nostr code for the same
currency, verifying the Nostr quote is excluded.
---
Nitpick comments:
In `@src/price/manager.rs`:
- Around line 508-515: Update the republishing flow around republishable_rates
and the published source-tag construction to derive source contributors from the
surviving rates rather than the full successes set. Exclude Nostr when its
contributed currencies are all removed, while preserving source tags for
contributors represented in the republished payload.
In `@src/price/providers/nostr.rs`:
- Around line 275-277: Rename the stale select_freshest references in the
documentation comments near the signed-event helpers to rank_candidates, and
rename every select_freshest_* test in the affected test block to the
rank_candidates_* prefix. Keep the test behavior unchanged and use descriptive
names for each specific scenario.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 01b93be7-38dd-4571-a644-11c9e4bc38cd
📒 Files selected for processing (2)
src/price/manager.rssrc/price/providers/nostr.rs
|
@ToRyVand i think we are ready for merge, fix last coderabbit nipticks and we should be ok! |
`restrict_nostr_to_fallback` compared raw currency keys, but `aggregate_tick` upper-cases every code before grouping. Not every adapter normalises on the way out — Yadio forwards the API's codes verbatim — so a direct `usd` and a Nostr `USD` slipped past the filter and then landed in the same aggregate, producing exactly the double-count the function exists to prevent. Fold both sides of the comparison before matching, and cover the case with a regression test that fails on the raw comparison.
Two review nitpicks from the last CodeRabbit pass: `publish_rates_to_nostr` tagged the event with the tick-wide contributor list while `republishable_rates` may drop currencies from the body. A provider whose every contribution was filtered out was still named in the `source` tag of a payload carrying none of its data. Derive the tag from the surviving currencies instead, which makes the `successes` parameter redundant. Also rename the stale `select_freshest` references left over from the rename to `rank_candidates`, matching the prefix the envelope tests already use.
CodeRabbit items addressed — plus one new finding worth your callPushed two commits on top of
The Gates: New finding — an unresolvable
|
There was a problem hiding this comment.
Strict re-review — changes still requested
Reviewed the full PR conversation and exact current head 4c58f070a92df5158f8eaeabc75010b0cb87d0f7.
Fixed from the previous review
- NIP-40 expiration, future-date, and max-age event gates.
- Malformed/empty newest-event fallback.
- Client-side
kind/denvelope validation. - Direct Nostr-only republication guard.
- Correlated-source fallback, including case-insensitive currency matching.
- Published
sourcetag derivation and stale naming nitpicks.
Blocking
-
An unresolved
PerBasequote suppresses a usable Nostr fallback (src/price/manager.rs:298-300, helper at:595-617). Coverage is decided from raw quote keys before anchor resolution. If El Toque returnsCUPasPerBase { base: "USD", ... }, Nostr returns a usable directCUP, and no USD anchor exists, the helper drops Nostr's CUP first; aggregation then drops the unresolved El Toque quote and produces no CUP price. I reproduced this on the current head with a temporary regression test: expectedSome(30_000_000.0), gotNone. Determine non-Nostr coverage after resolution, and add this regression test. -
Fiat-cross resolution drops Nostr provenance before the republication guard (
src/price/aggregate.rs:141-163,src/price/manager.rs:628-653). When Nostr supplies the USD anchor and El Toque suppliesCUP/USD, the resulting CUP is attributed only to El Toque, sorepublishable_ratesrepublishes a Nostr-derived value with a fresh timestamp andsource=eltoque. Preserve/track anchor provenance (or Nostr-derived taint) through cross resolution and test Nostr USD → El Toque CUP. -
Relay responses are collected without a local event-count bound before validation (
src/price/providers/nostr.rs:218-226). With subscription-filter verification disabled in the locked relay pool, a malicious relay can stream arbitrarily many validly signed irrelevant events until timeout;fetch_events()force-inserts them before local author/envelope/freshness filtering and sorting. The timeout bounds time, not memory/work. Enforce a hard local bound while receiving and reject irrelevant events before unbounded collection; add an adversarial test.
Verification
cargo fmt --check: passedcargo test price::: 136 passed, 1 ignored- Full
cargo test: 1,056 passed, 2 ignored across targets cargo clippy --all-targets --all-features -- -D warnings: passedgit diff --check: passed
The previous blockers are substantially addressed, but these paths still need correction before merge.
Determine Nostr fallback coverage after PerBase resolution, taint crosses that embed a Nostr anchor so they are not republished, and bound streamed relay events before ranking. Co-authored-by: Cursor <cursoragent@cursor.com>
Addressed ermeme's three remaining blockers (
|
There was a problem hiding this comment.
Strict re-review — one blocker remains
Reviewed the full conversation and exact current head a8a6aef23871aee42712204abcc190fb6650364a.
Fixed
- The unresolved
PerBasepath no longer suppresses a usable Nostr fallback. - Nostr anchor dependence is now tracked through fiat-cross resolution and excluded from republication.
Still blocking
The new application-level cap does not bound memory in the locked relay-pool stream implementation (src/price/providers/nostr.rs:266-285).
Client::stream_events delegates to nostr-relay-pool 0.44.1's pooled stream. Before events reach this loop, RelayPool::stream_events_targeted inserts every unique event ID into an unbounded HashSet<EventId> (pool/mod.rs:1271-1295). Subscription-filter verification is still disabled, so a malicious relay can send arbitrarily many unique, validly signed but irrelevant events. The application stops inspecting after 256 frames, but dropping its receiver does not stop the spawned pool driver: it keeps consuming until timeout, keeps inserting IDs, and ignores failed sends. Therefore the previous remote memory/work DoS remains below the new cap.
Please bound/reject at or before the relay-pool driver (for example, enable and verify subscription enforcement with mismatch handling, or use an actually bounded relay-level receive path), and add a test that exercises the production stream boundary rather than only the pure collect_bounded_candidates helper.
Verification
cargo fmt --check: passedcargo test price::: 141 passed, 1 ignored- Full
cargo test: 1,061 passed, 2 ignored across targets cargo clippy --all-targets --all-features -- -D warnings: passedgit diff --check: passed
The two aggregation fixes are good; only the stream-boundary DoS remains.
|
Follow-up from two independent audits of
The two primary aggregation blockers from the previous review are otherwise fixed. The relay-pool unbounded dedup state remains the main blocker in the formal review. |
3 participants
Summary
Closes #697. Adds a
nostrprice provider somostrodcan source BTC/fiatquotes by subscribing to kind-30078 (NIP-33) rate events published by
trusted Mostro nodes, instead of hitting an HTTP price API — for operators
in regions where those APIs are network-blocked (the motivating case:
api.yadio.iois DNS-blocked for Venezuelan ISPs; PR #685 already solvedthis for mobile clients, this closes the gap for the node operator).
It slots into the existing multi-source
PriceProviderregistry(
src/price/,docs/PRICE_PROVIDERS.md) as one more adapter — no changesto the aggregation core, the store, the scheduler, or any order handler.
src/price/providers/nostr.rs(new):NostrProviderqueries theprocess-wide Nostr client (already connected to
[nostr]'s relays — nonew connections) for
kind=30078, authors=trusted_nodes, d=mostro-ratesonce per tick, re-verifies each returned event's pubkey against
trusted_nodesclient-side, and takes the freshest valid event asthe tick's source. With several
trusted_nodesconfigured this isredundancy (if one is down/stale, a fresher one wins), not a statistical
combine across nodes — kept deliberately simple, see
docs/PRICE_PROVIDERS.md§11.7 for the rationale.
src/price/manager.rs,src/price/providers/mod.rs: registry wiring(
build_providerarm + module declaration).settings.tpl.toml: commented[price.providers.nostr]template block.docs/PRICE_PROVIDERS.md: new §11.7 appendix section (the code alreadyreferenced it) + a note on the Phase table (§8) that this is an
out-of-band addition to the original publish-only 5-phase plan.
src/price/config.rs: fixesProviderConfig::validate()— thetrusted_nodesexemption from requiring aurlwas previously genericacross any provider id, so e.g. copy-pasting
trusted_nodesonto[price.providers.yadio]would silently pass startup validation and onlyfail later with a confusing per-tick HTTP error against an empty URL. Now
scoped to
nostronly, with a regression test.Test plan
cargo build— clean.cargo test price::— 117 passed (includes newparse_content,select_freshest, constructor, and registry-wiring tests).cargo clippy --all-targets --all-features— no issues.cargo fmt --check— clean.#[ignore]d test(
price::providers::nostr::tests::live_relay_fetch_returns_real_rates,not run in CI) exercises the real
fetch()path againstwss://relay.mostro.networkusing the two exampletrusted_nodespubkeys from the issue. Run manually:
cargo test price::providers::nostr::tests::live_relay_fetch_returns_real_rates -- --ignored --exact— returned 140 live currencies (USD, EUR, CUP, MLC, ARS, …),
confirming the whole path (client → filter → freshest-selection →
parse) works end-to-end against production infrastructure, not just
fixtures.
Summary by CodeRabbit
30078“mostro-rates” events from configured trusted nodes.nostras a selectable price-provider option.