From 3aedfb0547d2c13e7cb218de04bfa7fea6b4f48e Mon Sep 17 00:00:00 2001 From: grumbach Date: Thu, 6 Aug 2026 14:18:04 +0900 Subject: [PATCH] fix(payment): require an explicit tolerance to enforce the price floor Setting ANT_PRICE_FLOOR_ENFORCE=1 was enough on its own to enforce the receiver-side price floor, with the tolerance falling back to a compiled-in default when the operator had not set one. The tolerance is not a formatting detail: it decides which already-settled, unrefundable payments a node refuses, and production measurement of the previous floor put that at 0.377% of honest stores. Enforcing at a percentage nobody chose is therefore the wrong default for the one variable that decides who gets rejected. Its meaning also depends on what the floor is priced against, and that reference has already changed once, from the receiver's own commitment-bound price to the close-group median. A node still carrying an enforcement opt-in from before that change would silently resume enforcing the new rule at a default it never selected, which is not a decision an upgrade should make on an operator's behalf. Enforcement now requires ANT_PRICE_FLOOR_TOLERANCE_PERCENT to name a valid percentage. Anything else, absent or unparseable or out of range or not readable as Unicode, fails closed to shadow mode and logs an error naming which of those states it is in, because the fixes differ. Shadow mode is unchanged: it still evaluates at the default tolerance, since producing telemetry is its entire purpose, and it is what every node runs today. This makes the function honour the rule its own comment already stated, that it should never enforce a tolerance the operator did not actually specify. Two of the three unusable states were previously exempt from it. The diagnostic separates an unset variable from one that is set but unreadable. That distinction does not change the enforcement decision here, since neither parses to a valid percentage, so it is stated as an operator-facing message rather than as a second safety property. Extends the existing environment test, which owns these variables, rather than adding a sibling that would race its cleanup. The new cases were confirmed to fail against the previous behaviour. --- src/payment/verifier.rs | 127 +++++++++++++++++++++++++++++++++------- 1 file changed, 105 insertions(+), 22 deletions(-) diff --git a/src/payment/verifier.rs b/src/payment/verifier.rs index fd550c77..865eb1ad 100644 --- a/src/payment/verifier.rs +++ b/src/payment/verifier.rs @@ -28,6 +28,7 @@ use saorsa_core::identity::node_identity::peer_id_from_public_key_bytes; use saorsa_core::identity::PeerId; use saorsa_core::P2PNode; use std::collections::HashMap; +use std::env::VarError; use std::num::NonZeroUsize; use std::sync::Arc; use std::time::Instant; @@ -193,10 +194,18 @@ impl GroupSample { /// Absent or any other value means shadow mode: the floor is computed and /// logged, never enforced. Per-node so enforcement can roll out canary-first; /// unsetting it (and restarting) is the kill switch. +/// +/// Not sufficient on its own: [`PRICE_FLOOR_TOLERANCE_ENV`] must also name a +/// valid tolerance, or [`PriceFloorConfig::from_env`] stays in shadow mode. pub const PRICE_FLOOR_ENFORCE_ENV: &str = "ANT_PRICE_FLOOR_ENFORCE"; -/// Environment variable overriding the price-floor tolerance percentage -/// (`0..=100`). Invalid or absent values use the default. +/// Environment variable setting the price-floor tolerance percentage +/// (`0..=100`). +/// +/// Required to enforce, optional to measure. In shadow mode an absent or +/// unreadable value falls back to the default so telemetry still has a number. +/// Under [`PRICE_FLOOR_ENFORCE_ENV`] there is no fallback: an absent or +/// unreadable value fails closed to shadow mode. pub const PRICE_FLOOR_TOLERANCE_ENV: &str = "ANT_PRICE_FLOOR_TOLERANCE_PERCENT"; /// Receiver-side price floor for single-node store admissions. @@ -244,46 +253,67 @@ impl PriceFloorConfig { /// Build from the environment (see [`PRICE_FLOOR_ENFORCE_ENV`] and /// [`PRICE_FLOOR_TOLERANCE_ENV`]). Never panics. /// - /// An unset tolerance uses the default. A tolerance that is *present but - /// invalid* (unparseable or `> 100`) is an operator error: rather than - /// silently enforce at the default, this **fails closed to shadow mode** - /// (`enforce = false`) and logs a prominent error, so a fat-fingered - /// tolerance can never enforce an unintended floor against real payments. + /// **Enforcement requires an explicitly specified, valid tolerance.** + /// Anything else (unset, unparseable, `> 100`, or not readable as Unicode) + /// **fails closed to shadow mode** (`enforce = false`) and logs a prominent + /// error. Shadow mode still evaluates, at the default tolerance, because its + /// whole job is to produce telemetry. + /// + /// The tolerance is not a formatting detail, it is the policy: it decides + /// which already-settled, unrefundable payments get refused. Its meaning also + /// depends on what the floor is priced against, and that reference has + /// changed once already, so a node carrying an older enforcement opt-in must + /// not silently start enforcing a newer rule at a default nobody chose for + /// it. Requiring the operator to name a tolerance is what makes enabling + /// enforcement a deliberate act under the rule that is actually in force. #[must_use] pub fn from_env() -> Self { let enforce_requested = std::env::var(PRICE_FLOOR_ENFORCE_ENV) .is_ok_and(|v| matches!(v.trim(), "1" | "true" | "TRUE" | "True")); - let tolerance_raw = std::env::var(PRICE_FLOOR_TOLERANCE_ENV).ok(); + // `var` separates three cases and only ONE of them means the operator + // said nothing. A value that is set but not readable as Unicode is still + // a value someone set, so it must not be mistaken for an absent one: + // reading it through `.ok()` would collapse it onto "unset" and hand it + // the default. + let tolerance_raw = std::env::var(PRICE_FLOOR_TOLERANCE_ENV); + let tolerance_unset = matches!(tolerance_raw, Err(VarError::NotPresent)); let valid_tolerance = tolerance_raw .as_deref() + .ok() .map(str::trim) .and_then(|s| s.parse::().ok()) .filter(|percent| *percent <= 100); - // A tolerance that is set but does not parse to `0..=100` is an operator - // error. Fail CLOSED: never enforce a tolerance the operator did not - // actually specify. - let tolerance_present_but_invalid = tolerance_raw.is_some() && valid_tolerance.is_none(); - if tolerance_present_but_invalid { + // Name WHICH unusable state the tolerance is in, as a value rather than + // as duplicated log arms. The two states take different fixes (set it + // versus correct it), and telling an operator it is unset when they did + // set it, to something this process cannot read, sends them to the wrong + // one. + let tolerance_problem = match (valid_tolerance.is_some(), tolerance_unset) { + (true, _) => None, + (false, true) => Some("is unset"), + (false, false) => Some("is not readable as an integer in 0..=100"), + }; + + if let Some(problem) = tolerance_problem { if enforce_requested { crate::logging::error!( - "{PRICE_FLOOR_TOLERANCE_ENV}={tolerance_raw:?} is not an integer in 0..=100; \ - refusing to enforce the price floor with an unspecified tolerance. \ - Price-floor enforcement DISABLED (shadow mode). Fix the value and restart \ - to enable enforcement." + "Price-floor enforcement was requested but {PRICE_FLOOR_TOLERANCE_ENV} \ + {problem}; refusing to enforce a tolerance nobody specified. Price-floor \ + enforcement DISABLED (shadow mode). Set {PRICE_FLOOR_TOLERANCE_ENV} to an \ + integer in 0..=100 and restart to enable enforcement." ); - } else { + } else if !tolerance_unset { crate::logging::warn!( - "{PRICE_FLOOR_TOLERANCE_ENV}={tolerance_raw:?} is not an integer in 0..=100; \ - using the default {PRICE_FLOOR_DEFAULT_TOLERANCE_PERCENT}% for shadow-mode \ - telemetry." + "{PRICE_FLOOR_TOLERANCE_ENV} {problem}; using the default \ + {PRICE_FLOOR_DEFAULT_TOLERANCE_PERCENT}% for shadow-mode telemetry." ); } } Self { - enforce: enforce_requested && !tolerance_present_but_invalid, + enforce: enforce_requested && valid_tolerance.is_some(), tolerance_percent: valid_tolerance.unwrap_or(PRICE_FLOOR_DEFAULT_TOLERANCE_PERCENT), } } @@ -5140,6 +5170,59 @@ mod tests { let cfg = PriceFloorConfig::from_env(); assert!(!cfg.enforce); + // Enforce on, tolerance UNSET: disabled too. This is the upgrade case — + // a node carrying an enforcement opt-in from an older release, where the + // floor was priced against a different reference, must not resume + // enforcing under the current rule at a default it never chose. + std::env::remove_var(PRICE_FLOOR_TOLERANCE_ENV); + let cfg = PriceFloorConfig::from_env(); + assert!( + !cfg.enforce, + "enforcement must require an explicitly specified tolerance, not inherit a default" + ); + assert_eq!( + cfg.tolerance_percent, PRICE_FLOOR_DEFAULT_TOLERANCE_PERCENT, + "shadow mode still needs a number to measure with" + ); + + // Enforce on, tolerance set but NOT READABLE as Unicode: disabled too. + // + // Stated precisely, because the stronger claim is tempting and wrong: + // requiring a valid tolerance ALREADY covers this, since an unreadable + // value cannot parse and so is not valid. This case is not what makes + // enforcement safe here; it pins that the unreadable path stays on the + // fail-closed side if the "explicitly specified" rule is ever relaxed + // back toward a default, which is when `NotUnicode` collapsing onto + // `NotPresent` would start deciding the outcome again. The value of + // separating them TODAY is the operator-facing message: telling someone + // the tolerance is unset when they did set it, to something this process + // cannot read, sends them to the wrong fix. + // + // Kept inside this test rather than beside it: this test owns the + // `PRICE_FLOOR_*` variables, and a sibling test setting them would race + // its cleanup rather than measure anything. + #[cfg(unix)] + { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + + // Lone continuation byte: a real value, and not valid UTF-8. + std::env::set_var(PRICE_FLOOR_TOLERANCE_ENV, OsStr::from_bytes(&[0x80])); + assert!( + std::env::var_os(PRICE_FLOOR_TOLERANCE_ENV).is_some(), + "the value must really be set for this case to discriminate" + ); + assert!( + std::env::var(PRICE_FLOOR_TOLERANCE_ENV).is_err(), + "and it must really be unreadable as Unicode" + ); + let cfg = PriceFloorConfig::from_env(); + assert!( + !cfg.enforce, + "an unreadable tolerance is a SET value, not an absent one, and must fail closed" + ); + } + std::env::remove_var(PRICE_FLOOR_ENFORCE_ENV); std::env::remove_var(PRICE_FLOOR_TOLERANCE_ENV); }