From 68f86d9cca5a232fb29bc79e07d2d6c8668a9686 Mon Sep 17 00:00:00 2001 From: wisdom2030-dotcom Date: Tue, 20 Oct 2026 10:18:32 +0100 Subject: [PATCH] fix: hash network cursor ID and rule identity into alert_id (#252) alert_id() only hashed contract_id, tx_hash and rule_triggered, so two rules with identical labels on one contract produced the same ID and were deduplicated away by receivers. Hash the network cursor ID, rule type and the rule's index within the contract's rule list instead. --- CHANGELOG.md | 1 + README.md | 2 +- crates/notifier/src/lib.rs | 3 + crates/poller/src/lib.rs | 3 + crates/poller/tests/integration.rs | 8 + crates/rules/benches/evaluate.rs | 1 + crates/rules/src/lib.rs | 281 ++++++++++++++++++++++------- crates/rules/tests/attack_tests.rs | 1 + docs/alert-rules.md | 13 +- docs/configuration.md | 12 +- 10 files changed, 250 insertions(+), 75 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 665d51f..fe6e2d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Rule evaluation errors are rate-limited: only the first occurrence and every 100th recurrence are logged (#60). - `SourceAccount`'s `allow`/`deny` fields are deprecated in favour of `mode` + `accounts`; both previously fired on list *membership*, which reversed the expected "alert on unknown caller" behaviour (#250). - `NoActivity` incidents now carry a unique `alert_id` per incident, and the recovery alert reuses its incident's `alert_id` so the two can be correlated (#254, #255). +- `alert_id` now hashes the network cursor ID, the rule type, the rule label and the rule's index within its contract's `rules` list — previously it only hashed `contract_id`, `tx_hash` and `rule_triggered`, so two rules with identical labels on one contract (e.g. two `LargeTransfer(>=100XLM)` rules routed to different webhooks) produced the same ID and were deduplicated away by receivers (#252). **IDs change**: receivers that persist `alert_id` for deduplication will see new values for alerts generated after upgrading, so old entries can no longer be matched against new ones. `EvalContext` gains a `network_id` field and `check_no_activity` gains a `rule_index` parameter. ### Removed diff --git a/README.md b/README.md index ed3982f..2683fcd 100644 --- a/README.md +++ b/README.md @@ -291,7 +291,7 @@ service. See [Multiple destinations](docs/configuration.md#multiple-destinations **Fields:** - `schema_version` — integer version of this payload shape (currently `1`). Additive changes (new optional fields) keep the same version; breaking changes (field removals or renames) bump it. Receivers should use this to detect incompatible changes. -- `alert_id` — stable, deterministic identifier derived from `(network, contract_id, tx_hash, rule_type, rule_triggered)` via SHA-256 prefix (32 hex chars). Identical for every retry of the same alert. Receivers should deduplicate on this value. +- `alert_id` — stable, deterministic identifier derived from `(network, contract_id, tx_hash, rule_type, rule_triggered, rule_index)` via SHA-256 prefix (32 hex chars), where `network` is the network cursor ID (e.g. `custom@https://…` for custom networks) and `rule_index` is the rule's position in the contract's `rules` list. Identical for every retry of the same alert, but never shared between two rules — even two rules with identical labels get different IDs (#252). Receivers should deduplicate on this value. - `rule_type` — stable machine-readable rule variant (e.g. `"LargeTransfer"`, `"HighFee"`); use this for programmatic routing - `rule_triggered` — human-readable rule description with parameters (e.g. `"LargeTransfer(>=10000XLM)"`); use this for display - `function_name` — the first invoked Soroban function name, or `null` for non-Soroban transactions. diff --git a/crates/notifier/src/lib.rs b/crates/notifier/src/lib.rs index 04564d3..3d68b79 100644 --- a/crates/notifier/src/lib.rs +++ b/crates/notifier/src/lib.rs @@ -368,12 +368,15 @@ pub fn test_payload_with_network( schema_version: 1, // Unique per send, so repeated tests are not de-duplicated by receivers. alert_id: txwatch_rules::alert_id( + network, TEST_CONTRACT_ID, tx_hash, + rule_type, &format!( "TestWebhook@{}", now.timestamp_nanos_opt().unwrap_or_default() ), + 0, ), label: label.to_string(), contract_id: TEST_CONTRACT_ID.into(), diff --git a/crates/poller/src/lib.rs b/crates/poller/src/lib.rs index b424dd8..4746f63 100644 --- a/crates/poller/src/lib.rs +++ b/crates/poller/src/lib.rs @@ -1535,6 +1535,7 @@ async fn poll_contract( label: &contract.label, contract_id: &contract.contract_id, network: contract.network.as_str(), + network_id: contract.network.cursor_id(), horizon_base: canonical_base, explorer_base: contract.network.explorer_base_url(), }; @@ -1779,6 +1780,7 @@ async fn poll_contract( } if let Some(payload) = check_no_activity( &rule.rule, + idx, state.last_seen, now, &mut state.no_activity_states[idx], @@ -2061,6 +2063,7 @@ fn evaluate_contract( label: &contract.label, contract_id: &contract.contract_id, network: contract.network.as_str(), + network_id: contract.network.cursor_id(), horizon_base, explorer_base: contract.network.explorer_base_url(), }; diff --git a/crates/poller/tests/integration.rs b/crates/poller/tests/integration.rs index fc20f9d..ba433ec 100644 --- a/crates/poller/tests/integration.rs +++ b/crates/poller/tests/integration.rs @@ -272,6 +272,7 @@ async fn any_transaction_fires_webhook() { label: &contract.label, contract_id: &contract.contract_id, network: contract.network.as_str(), + network_id: contract.network.cursor_id(), horizon_base: &horizon.uri(), explorer_base: Some("https://stellar.expert/explorer/testnet"), }; @@ -377,6 +378,7 @@ async fn transaction_failed_rule_fires_only_on_failure() { label: &contract.label, contract_id: &contract.contract_id, network: contract.network.as_str(), + network_id: contract.network.cursor_id(), horizon_base: &horizon.uri(), explorer_base: Some("https://stellar.expert/explorer/testnet"), }; @@ -439,6 +441,7 @@ async fn large_transfer_fires_above_threshold() { label: &contract.label, contract_id: &contract.contract_id, network: contract.network.as_str(), + network_id: contract.network.cursor_id(), horizon_base: "https://horizon-testnet.stellar.org", explorer_base: Some("https://stellar.expert/explorer/testnet"), }, @@ -527,6 +530,7 @@ async fn function_called_rule_fires_on_exact_match() { label: &contract.label, contract_id: &contract.contract_id, network: contract.network.as_str(), + network_id: contract.network.cursor_id(), horizon_base: "https://horizon-testnet.stellar.org", explorer_base: Some("https://stellar.expert/explorer/testnet"), }, @@ -737,6 +741,7 @@ async fn high_fee_rule_fires_on_fee_charged() { label: &contract.label, contract_id: &contract.contract_id, network: contract.network.as_str(), + network_id: contract.network.cursor_id(), horizon_base: &horizon.uri(), explorer_base: Some("https://stellar.expert/explorer/testnet"), }, @@ -1757,6 +1762,7 @@ async fn large_transfer_only_counts_transfers_involving_the_contract() { label: &contract.label, contract_id: &contract.contract_id, network: contract.network.as_str(), + network_id: contract.network.cursor_id(), horizon_base: "https://horizon-testnet.stellar.org", explorer_base: Some("https://stellar.expert/explorer/testnet"), }; @@ -1834,6 +1840,7 @@ async fn large_transfer_respects_direction_option() { label: &contract.label, contract_id: &contract.contract_id, network: contract.network.as_str(), + network_id: contract.network.cursor_id(), horizon_base: "https://horizon-testnet.stellar.org", explorer_base: Some("https://stellar.expert/explorer/testnet"), }; @@ -1907,6 +1914,7 @@ async fn contract_upgraded_rule_fires_with_wasm_hashes() { label: &contract.label, contract_id: &contract.contract_id, network: contract.network.as_str(), + network_id: contract.network.cursor_id(), horizon_base: "https://horizon-testnet.stellar.org", explorer_base: Some("https://stellar.expert/explorer/testnet"), }, diff --git a/crates/rules/benches/evaluate.rs b/crates/rules/benches/evaluate.rs index 8c36a81..1d7e7db 100644 --- a/crates/rules/benches/evaluate.rs +++ b/crates/rules/benches/evaluate.rs @@ -67,6 +67,7 @@ fn bench_evaluate(c: &mut Criterion) { label: "bench", contract_id: "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABSC4", network: "testnet", + network_id: "testnet".into(), horizon_base: "https://horizon-testnet.stellar.org", explorer_base: Some("https://stellar.expert/explorer/testnet"), }; diff --git a/crates/rules/src/lib.rs b/crates/rules/src/lib.rs index 80b0dea..462f24c 100644 --- a/crates/rules/src/lib.rs +++ b/crates/rules/src/lib.rs @@ -541,8 +541,9 @@ pub struct AlertPayload { /// breaking changes (field removals or renames) bump it. pub schema_version: u32, /// Stable, deterministic alert identifier derived from - /// (network, contract_id, tx hash, rule_type, rule params) via SHA-256 - /// (issue #57). Receivers should deduplicate on this value. + /// (network cursor ID, contract_id, tx hash, rule_type, rule_triggered, + /// rule index) via SHA-256 (issues #57, #252). Receivers should + /// deduplicate on this value. /// Used as the PagerDuty `dedup_key`. #[serde(default)] pub alert_id: String, @@ -647,15 +648,37 @@ pub struct AlertPayload { } /// Deterministic alert ID: the first 32 hex characters (128 bits) of -/// SHA-256 over `contract_id`, `transaction_hash` and `rule_triggered`. -pub fn alert_id(contract_id: &str, transaction_hash: &str, rule_triggered: &str) -> String { +/// SHA-256 over the network cursor ID, `contract_id`, `transaction_hash`, +/// `rule_type`, `rule_triggered` and the rule's index within its contract's +/// rule list. +/// +/// Both the network and the rule identity are hashed (#252): two rules with +/// identical labels on one contract (e.g. two `LargeTransfer(>=100XLM)` rules +/// routed to different webhooks) must produce different IDs, otherwise +/// receivers deduplicate one of them away. `rule_index` is the position of the +/// rule in its contract's `rules` list, so it stays stable for a given config. +pub fn alert_id( + network: &str, + contract_id: &str, + transaction_hash: &str, + rule_type: &str, + rule_triggered: &str, + rule_index: usize, +) -> String { use sha2::{Digest, Sha256}; let mut hasher = Sha256::new(); - for part in [contract_id, transaction_hash, rule_triggered] { + for part in [ + network, + contract_id, + transaction_hash, + rule_type, + rule_triggered, + ] { hasher.update(part.as_bytes()); // Separator so ("ab", "c") and ("a", "bc") differ. hasher.update([0u8]); } + hasher.update(rule_index.to_le_bytes()); let mut id = hex::encode(hasher.finalize()); id.truncate(32); id @@ -705,6 +728,10 @@ pub struct EvalContext<'a> { pub contract_id: &'a str, /// Network the contract lives on (`"mainnet"`, `"testnet"`, …). pub network: &'a str, + /// Network cursor ID ([`txwatch_config::Network::cursor_id`]): the stable + /// per-network identity used in `alert_id` (#252). Unlike `network`, this + /// tells two custom networks apart (`"custom@https://…"`, see issue #252). + pub network_id: String, /// Horizon base URL used to build transaction links. pub horizon_base: &'a str, /// Explorer base URL for the network; `None` for custom networks that @@ -725,6 +752,7 @@ impl<'a> EvalContext<'a> { label: &contract.label, contract_id: &contract.contract_id, network: contract.network.as_str(), + network_id: contract.network.cursor_id(), horizon_base, explorer_base: contract.network.explorer_base_url(), } @@ -808,49 +836,61 @@ pub fn evaluate + RuleOverrides>( rules .iter() - .filter(|entry| entry.enabled()) - .filter_map(|entry| { + .enumerate() + .filter(|(_, entry)| entry.enabled()) + .filter_map(|(rule_index, entry)| { let rule: &AlertRule = entry.as_ref(); match eval_rule(rule, tx) { - Ok(true) => Some(AlertPayload { - schema_version: 1, - alert_id: alert_id(contract_id, &tx.hash, &rule.label()), - label: label.to_string(), - contract_id: contract_id.to_string(), - network: network.to_string(), - // #44: Use the single canonical implementations from txwatch-config. - rule_type: rule.rule_type().to_string(), - rule_triggered: rule.label(), - transaction_hash: tx.hash.clone(), - function_name: tx.function_names.first().cloned(), - function_names: tx.function_names.clone(), - function_args: tx.invocations.clone(), - amount_xlm, - amount_stroops: tx.amount_stroops, - amount_xlm_decimal: amount_xlm_decimal.clone(), - fee_charged_stroops: tx.fee_charged_stroops, - source_account: tx.source_account.clone(), - // Per-rule delivery overrides, absent unless the rule sets them. - severity: entry.severity().map(|s| s.to_string()), - effective_webhook_url: entry.webhook_url().map(str::to_owned), - effective_webhook_secret: entry.webhook_secret().map(str::to_owned), - timestamp, - timestamp_iso: timestamp_iso.clone(), - horizon_link: horizon_link.clone(), - explorer_link: explorer_link.clone(), - ledger: tx.ledger, - memo: tx.memo.clone(), - memo_type: tx.memo_type.clone(), - operation_count: tx.operation_count, - resolved: false, - matched_events: matched_events(rule, tx), - suppressed_count: 0, - observed_count: None, - test: false, - old_wasm_hash: upgrade_hashes(rule, tx).and_then(|(o, _)| o), - new_wasm_hash: upgrade_hashes(rule, tx).and_then(|(_, n)| n), - enrichment_incomplete: tx.enrichment_incomplete, - }), + Ok(true) => { + let rule_type = rule.rule_type().to_string(); + let rule_triggered = rule.label(); + Some(AlertPayload { + schema_version: 1, + alert_id: alert_id( + &ctx.network_id, + contract_id, + &tx.hash, + &rule_type, + &rule_triggered, + rule_index, + ), + label: label.to_string(), + contract_id: contract_id.to_string(), + network: network.to_string(), + // #44: Use the single canonical implementations from txwatch-config. + rule_type, + rule_triggered, + transaction_hash: tx.hash.clone(), + function_name: tx.function_names.first().cloned(), + function_names: tx.function_names.clone(), + function_args: tx.invocations.clone(), + amount_xlm, + amount_stroops: tx.amount_stroops, + amount_xlm_decimal: amount_xlm_decimal.clone(), + fee_charged_stroops: tx.fee_charged_stroops, + source_account: tx.source_account.clone(), + // Per-rule delivery overrides, absent unless the rule sets them. + severity: entry.severity().map(|s| s.to_string()), + effective_webhook_url: entry.webhook_url().map(str::to_owned), + effective_webhook_secret: entry.webhook_secret().map(str::to_owned), + timestamp, + timestamp_iso: timestamp_iso.clone(), + horizon_link: horizon_link.clone(), + explorer_link: explorer_link.clone(), + ledger: tx.ledger, + memo: tx.memo.clone(), + memo_type: tx.memo_type.clone(), + operation_count: tx.operation_count, + resolved: false, + matched_events: matched_events(rule, tx), + suppressed_count: 0, + observed_count: None, + test: false, + old_wasm_hash: upgrade_hashes(rule, tx).and_then(|(o, _)| o), + new_wasm_hash: upgrade_hashes(rule, tx).and_then(|(_, n)| n), + enrichment_incomplete: tx.enrichment_incomplete, + }) + } Ok(false) => None, Err(e) => { // A broken rule would otherwise warn on every poll cycle. @@ -1247,6 +1287,9 @@ pub enum NoActivityState { /// Check the `NoActivity` rule for a single contract after one poll cycle. /// /// * `rule` — must be `AlertRule::NoActivity { minutes }`. +/// * `rule_index` — position of this rule in the contract's `rules` +/// list, hashed into `alert_id` so two identical `NoActivity` rules on one +/// contract get distinct IDs (#252). /// * `last_seen` — the timestamp of the most recent transaction seen, /// or `None` if we have never seen any transaction. /// * `now` — current time (injectable for testing). @@ -1264,6 +1307,7 @@ pub enum NoActivityState { /// - Activity present and never breached → returns `None`. pub fn check_no_activity( rule: &AlertRule, + rule_index: usize, last_seen: Option>, now: DateTime, state: &mut NoActivityState, @@ -1298,7 +1342,14 @@ pub fn check_no_activity( // Threshold just exceeded for the first time → fire incident. (NoActivityState::Active, true) => { let rule_triggered = format!("NoActivity({}min)", minutes); - let incident_id = alert_id(ctx.contract_id, &ts.to_string(), &rule_triggered); + let incident_id = alert_id( + &ctx.network_id, + ctx.contract_id, + &ts.to_string(), + "NoActivity", + &rule_triggered, + rule_index, + ); *state = NoActivityState::Alerting { incident_alert_id: incident_id.clone(), }; @@ -1345,9 +1396,12 @@ pub fn check_no_activity( (NoActivityState::Alerting { incident_alert_id }, false) => { let recovery_alert_id = if incident_alert_id.is_empty() { alert_id( + &ctx.network_id, ctx.contract_id, &ts.to_string(), + "NoActivity", &format!("NoActivity({}min) resolved", minutes), + rule_index, ) } else { incident_alert_id.clone() @@ -1547,7 +1601,7 @@ impl CooldownTracker { let mut summaries = Vec::new(); - for rule_cfg in rules { + for (rule_index, rule_cfg) in rules.iter().enumerate() { if !rule_cfg.cooldown_summary { continue; } @@ -1573,7 +1627,14 @@ impl CooldownTracker { let ts = now.timestamp(); let ts_iso = now.format("%Y-%m-%dT%H:%M:%SZ").to_string(); - let alert_id = alert_id(ctx.contract_id, &ts.to_string(), &rule_trig); + let alert_id = alert_id( + &ctx.network_id, + ctx.contract_id, + &ts.to_string(), + rule_cfg.rule.rule_type(), + &rule_trig, + rule_index, + ); summaries.push(AlertPayload { label: ctx.label.to_string(), @@ -1825,6 +1886,7 @@ mod tests { label: "Label", contract_id: "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABSC4", network: "testnet", + network_id: "testnet".into(), horizon_base: "https://horizon-testnet.stellar.org", explorer_base: Some("https://stellar.expert/explorer/testnet"), }; @@ -1988,6 +2050,7 @@ mod tests { label: "L", contract_id: "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", network: "testnet", + network_id: "testnet".into(), horizon_base: "https://horizon-testnet.stellar.org", explorer_base: Some("https://stellar.expert/explorer/testnet"), }; @@ -2245,6 +2308,7 @@ mod tests { label: "L", contract_id: "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABSC4", network: "testnet", + network_id: "testnet".into(), horizon_base, explorer_base, }; @@ -3088,6 +3152,7 @@ mod prop_tests { label: "PropTest", contract_id: "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", network: "testnet", + network_id: "testnet".into(), horizon_base: "https://horizon-testnet.stellar.org", explorer_base: Some("https://stellar.expert/explorer/testnet"), }; @@ -3167,6 +3232,7 @@ mod prop_tests { label: "PropTest", contract_id: "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", network: "testnet", + network_id: "testnet".into(), horizon_base: "https://horizon-testnet.stellar.org", explorer_base: Some("https://stellar.expert/explorer/testnet"), }; @@ -3190,6 +3256,7 @@ mod no_activity_tests { label: "Oracle", contract_id: "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", network: "testnet", + network_id: "testnet".into(), horizon_base: "https://horizon-testnet.stellar.org", explorer_base: Some("https://stellar.expert/explorer/testnet"), } @@ -3211,7 +3278,7 @@ mod no_activity_tests { // Last seen 6 minutes ago — threshold is 5 min, so this should fire. let last_seen = ts(12, 0); let now = ts(12, 6); - let payload = check_no_activity(&rule, Some(last_seen), now, &mut state, &eval_ctx); + let payload = check_no_activity(&rule, 0, Some(last_seen), now, &mut state, &eval_ctx); assert!(payload.is_some(), "should fire when quiet for > threshold"); let p = payload.unwrap(); @@ -3239,7 +3306,7 @@ mod no_activity_tests { // Last seen 4 minutes ago — below threshold. let last_seen = ts(12, 0); let now = ts(12, 4); - let payload = check_no_activity(&rule, Some(last_seen), now, &mut state, &eval_ctx); + let payload = check_no_activity(&rule, 0, Some(last_seen), now, &mut state, &eval_ctx); assert!(payload.is_none()); assert_eq!(state, NoActivityState::Active); @@ -3255,7 +3322,7 @@ mod no_activity_tests { // Exactly 5 minutes gap — should fire. let last_seen = ts(12, 0); let now = ts(12, 5); - let payload = check_no_activity(&rule, Some(last_seen), now, &mut state, &eval_ctx); + let payload = check_no_activity(&rule, 0, Some(last_seen), now, &mut state, &eval_ctx); assert!(payload.is_some(), "should fire at exactly threshold"); } @@ -3272,7 +3339,7 @@ mod no_activity_tests { // Still quiet — already alerting, so no second payload. let last_seen = ts(12, 0); let now = ts(12, 20); - let payload = check_no_activity(&rule, Some(last_seen), now, &mut state, &eval_ctx); + let payload = check_no_activity(&rule, 0, Some(last_seen), now, &mut state, &eval_ctx); assert!( payload.is_none(), @@ -3292,9 +3359,12 @@ mod no_activity_tests { let eval_ctx = contract; let rule = AlertRule::NoActivity { minutes: 5 }; let incident_id = alert_id( + &eval_ctx.network_id, eval_ctx.contract_id, &ts(12, 0).timestamp().to_string(), + "NoActivity", "NoActivity(5min)", + 0, ); let mut state = NoActivityState::Alerting { incident_alert_id: incident_id.clone(), @@ -3303,7 +3373,7 @@ mod no_activity_tests { // Activity just happened (1 minute ago) while we were in Alerting state. let last_seen = ts(12, 10); let now = ts(12, 11); - let payload = check_no_activity(&rule, Some(last_seen), now, &mut state, &eval_ctx); + let payload = check_no_activity(&rule, 0, Some(last_seen), now, &mut state, &eval_ctx); assert!( payload.is_some(), @@ -3330,7 +3400,7 @@ mod no_activity_tests { let last_seen_1 = ts(12, 0); let now_1 = ts(12, 6); let payload_1 = - check_no_activity(&rule, Some(last_seen_1), now_1, &mut state, &eval_ctx).unwrap(); + check_no_activity(&rule, 0, Some(last_seen_1), now_1, &mut state, &eval_ctx).unwrap(); assert!(!payload_1.resolved); let incident_id_1 = payload_1.alert_id.clone(); assert_eq!( @@ -3344,7 +3414,7 @@ mod no_activity_tests { let last_seen_2 = ts(12, 10); let now_2 = ts(12, 11); let recovery_1 = - check_no_activity(&rule, Some(last_seen_2), now_2, &mut state, &eval_ctx).unwrap(); + check_no_activity(&rule, 0, Some(last_seen_2), now_2, &mut state, &eval_ctx).unwrap(); assert!(recovery_1.resolved); assert_eq!( recovery_1.alert_id, incident_id_1, @@ -3355,7 +3425,7 @@ mod no_activity_tests { // 3. Incident 2: quiet again at 12:20 let now_3 = ts(12, 20); let payload_2 = - check_no_activity(&rule, Some(last_seen_2), now_3, &mut state, &eval_ctx).unwrap(); + check_no_activity(&rule, 0, Some(last_seen_2), now_3, &mut state, &eval_ctx).unwrap(); assert!(!payload_2.resolved); let incident_id_2 = payload_2.alert_id.clone(); assert_eq!( @@ -3375,7 +3445,7 @@ mod no_activity_tests { let last_seen_3 = ts(12, 25); let now_4 = ts(12, 26); let recovery_2 = - check_no_activity(&rule, Some(last_seen_3), now_4, &mut state, &eval_ctx).unwrap(); + check_no_activity(&rule, 0, Some(last_seen_3), now_4, &mut state, &eval_ctx).unwrap(); assert!(recovery_2.resolved); assert_eq!( recovery_2.alert_id, incident_id_2, @@ -3393,7 +3463,7 @@ mod no_activity_tests { // No transactions ever seen — treated as infinite quiet period. let now = ts(12, 0); - let payload = check_no_activity(&rule, None, now, &mut state, &eval_ctx); + let payload = check_no_activity(&rule, 0, None, now, &mut state, &eval_ctx); assert!( payload.is_some(), @@ -3453,13 +3523,84 @@ mod suppressor_tests { #[test] fn alert_id_is_deterministic_and_distinguishes_inputs() { - let id = alert_id("CA", "tx1", "AnyTransaction"); - assert_eq!(id, alert_id("CA", "tx1", "AnyTransaction")); + let id = alert_id("testnet", "CA", "tx1", "AnyTransaction", "AnyTransaction", 0); + assert_eq!( + id, + alert_id("testnet", "CA", "tx1", "AnyTransaction", "AnyTransaction", 0) + ); assert_eq!(id.len(), 32); assert!(id.chars().all(|c| c.is_ascii_hexdigit())); - assert_ne!(id, alert_id("CA", "tx2", "AnyTransaction")); - assert_ne!(id, alert_id("CA", "tx1", "TransactionFailed")); - assert_ne!(alert_id("ab", "c", "r"), alert_id("a", "bc", "r")); + assert_ne!(id, alert_id("testnet", "CA", "tx2", "AnyTransaction", "AnyTransaction", 0)); + assert_ne!(id, alert_id("testnet", "CA", "tx1", "TransactionFailed", "TransactionFailed", 0)); + assert_ne!(id, alert_id("mainnet", "CA", "tx1", "AnyTransaction", "AnyTransaction", 0)); + assert_ne!(id, alert_id("testnet", "CA", "tx1", "LargeTransfer", "AnyTransaction", 0)); + assert_ne!(id, alert_id("testnet", "CA", "tx1", "AnyTransaction", "AnyTransaction", 1)); + assert_ne!( + alert_id("testnet", "ab", "c", "r", "r", 0), + alert_id("testnet", "a", "bc", "r", "r", 0) + ); + } + + /// Two rules with identical labels on one contract (e.g. two + /// `LargeTransfer(>=100XLM)` rules routed to different webhooks) must get + /// distinct `alert_id`s, otherwise receivers deduplicate one of them away + /// (#252). + #[test] + fn identical_rules_on_one_contract_do_not_collide() { + let tx = EnrichedTransaction { + hash: "deadbeef".into(), + timestamp: "2024-01-15T12:00:00Z".parse().unwrap(), + successful: true, + paging_token: "1".into(), + function_names: vec![], + amount_stroops: Some(500_000_000_000), + fee_charged_stroops: None, + source_account: None, + fee_account: None, + ledger: None, + memo: None, + memo_type: None, + operation_count: None, + watched_contract_id: "CAAA".into(), + events: vec![], + contract_transfers: vec![ContractTransfer { + from: "CAAA".into(), + to: "GBUYER".into(), + amount_stroops: 500_000_000_000, + }], + wasm_upgrade: None, + enrichment_incomplete: false, + }; + let ctx = EvalContext { + label: "L", + contract_id: "CAAA", + network: "testnet", + network_id: "testnet".into(), + horizon_base: "https://h", + explorer_base: Some("https://e"), + }; + let same_rule = || AlertRule::LargeTransfer { + threshold_xlm: 100, + direction: "any".into(), + threshold_stroops: 1_000_000_000, + }; + let payloads = evaluate(&ctx, &[same_rule(), same_rule()], &tx, None); + assert_eq!(payloads.len(), 2, "both duplicate rules must fire"); + assert_eq!(payloads[0].rule_triggered, payloads[1].rule_triggered); + assert_eq!(payloads[0].alert_id, alert_id( + "testnet", "CAAA", "deadbeef", "LargeTransfer", + &payloads[0].rule_triggered, 0, + )); + assert_ne!( + payloads[0].alert_id, payloads[1].alert_id, + "identical rules at different indexes must not share an alert_id" + ); + + // The same rule on a different network must not collide either. + let mut other = ctx.clone(); + other.network_id = "mainnet".into(); + let other_payloads = evaluate(&other, &[same_rule()], &tx, None); + assert_ne!(payloads[0].alert_id, other_payloads[0].alert_id); } #[test] @@ -3489,6 +3630,7 @@ mod suppressor_tests { label: "L", contract_id: "CAAA", network: "testnet", + network_id: "testnet".into(), horizon_base: "https://h", explorer_base: Some("https://e"), }; @@ -3501,7 +3643,14 @@ mod suppressor_tests { assert_eq!(payloads.len(), 2); assert_eq!( payloads[0].alert_id, - alert_id("CAAA", "deadbeef", "AnyTransaction") + alert_id( + "testnet", + "CAAA", + "deadbeef", + "AnyTransaction", + "AnyTransaction", + 0, + ) ); assert_ne!(payloads[0].alert_id, payloads[1].alert_id); } @@ -3634,6 +3783,7 @@ mod suppressor_tests { label: "Label", contract_id: "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABSC4", network: "testnet", + network_id: "testnet".into(), horizon_base: "https://horizon-testnet.stellar.org", explorer_base: Some("https://stellar.expert/explorer/testnet"), }; @@ -4091,6 +4241,7 @@ mod suppressor_tests { label: "Oracle", contract_id: "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", network: "testnet", + network_id: "testnet".into(), horizon_base: "https://horizon-testnet.stellar.org", explorer_base: Some("https://stellar.expert/explorer/testnet"), } diff --git a/crates/rules/tests/attack_tests.rs b/crates/rules/tests/attack_tests.rs index 65f6a80..b7d0dfd 100644 --- a/crates/rules/tests/attack_tests.rs +++ b/crates/rules/tests/attack_tests.rs @@ -33,6 +33,7 @@ fn test_ctx() -> EvalContext<'static> { label: "Escrow", contract_id: "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", network: "testnet", + network_id: "testnet".into(), horizon_base: "https://horizon-testnet.stellar.org", explorer_base: Some("https://stellar.expert/explorer/testnet"), } diff --git a/docs/alert-rules.md b/docs/alert-rules.md index b3186d3..5444518 100644 --- a/docs/alert-rules.md +++ b/docs/alert-rules.md @@ -576,9 +576,16 @@ All matching enabled rules fire; there is no short-circuit. ### `alert_id` and deduplication -`alert_id` is derived deterministically from `(network, contract_id, tx_hash, rule_type, rule_triggered)` -via SHA-256 (first 16 bytes → 32 hex chars). The same alert always produces the same `alert_id`, -so receivers can safely deduplicate retries and cursor replays by storing and checking this value. +`alert_id` is derived deterministically from +`(network cursor ID, contract_id, tx_hash, rule_type, rule_triggered, rule_index)` via SHA-256 +(first 16 bytes → 32 hex chars), where `rule_index` is the rule's position in its contract's +`rules` list. The same alert always produces the same `alert_id`, so receivers can safely +deduplicate retries and cursor replays by storing and checking this value. + +Because the network and the rule identity (type, label and index) are part of the hash, two rules +with identical labels on one contract — e.g. two `LargeTransfer(>=100XLM)` rules routed to +different webhooks — never collapse into the same `alert_id` (#252). + It is also sent as the `X-TxWatch-Alert-Id` request header, enabling deduplication without parsing the JSON body. diff --git a/docs/configuration.md b/docs/configuration.md index 886336c..105c9a9 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -758,12 +758,12 @@ changes before parsing other fields. - `schema_version` — integer version of this payload shape. Use this to detect breaking changes. - `alert_id` — stable, deterministic identifier (32 hex chars) derived from - `(network, contract_id, tx_hash, rule_type, rule_triggered)` via SHA-256. Identical for every - retry of the same alert. Receivers should deduplicate on this value. Also sent as the - `X-TxWatch-Alert-Id` request header for deduplication without parsing the body. -- `alert_id` — stable 32-character hex ID derived from the contract, transaction and rule; the same - alert always has the same ID, so receivers can de-duplicate redeliveries. PagerDuty uses it as the - `dedup_key`. + `(network cursor ID, contract_id, tx_hash, rule_type, rule_triggered, rule_index)` via SHA-256, + where `rule_index` is the rule's position in its contract's `rules` list. Identical for every + retry of the same alert, but distinct for every rule — two rules with identical labels on one + contract never share an ID (#252). Receivers should deduplicate on this value; PagerDuty uses it + as the `dedup_key`. Also sent as the `X-TxWatch-Alert-Id` request header for deduplication + without parsing the body. - `rule_type` — stable machine-readable rule variant (e.g. `"LargeTransfer"`); use it for routing. - `rule_triggered` — human-readable rule description including parameters. - `amount_xlm` — whole-XLM transfer amount (truncated integer), or `null` when the transaction has none. Kept for backward compatibility — use `amount_xlm_decimal` for precise accounting.