From 8e3f82ad02e2c549df4a24a7971b1d7979f329b8 Mon Sep 17 00:00:00 2001 From: Roman Dolgov Date: Fri, 25 Sep 2026 21:48:12 +0400 Subject: [PATCH] ethereum: fix non-transitive trigger ordering causing sort panics EthereumTrigger::cmp compared Call triggers (and Call/Log pairs) by transaction_index, but compared Log/Log pairs by log_index alone, ignoring transaction_index entirely. Those two comparisons only agree when log_index happens to increase in lockstep with transaction_index across the whole set being sorted, which isn't guaranteed for every trigger source. When it doesn't hold, the combined ordering is not transitive: a Log in an earlier transaction can end up compared as greater than a Log in a later transaction if the earlier one has a higher log_index within its own transaction. This crashes graph-node in production with a panic from Rust's stable sort ("user-provided comparison function does not correctly implement a total order") inside BlockWithTriggers::new_with_triggers, which calls trigger_data.sort() on a freshly-built Vec. Fix the Log/Log comparison to key on transaction_index first, with log_index only as a tie-breaker within the same transaction, matching how Call/Call and Call/Log comparisons already work. Also rewrite the Call/Log and Log/Call arms with Ordering::then to remove the duplicate guarded/unguarded match arm pairs, now that both arms share the same primary-then-secondary-key logic. Added a regression test (test_trigger_ordering_is_transitive) that constructs the minimal counter-example and asserts both the pairwise comparisons and that sorting the resulting Vec doesn't panic. --- chain/ethereum/src/tests.rs | 34 +++++++++++++++++++++++++++ chain/ethereum/src/trigger.rs | 43 ++++++++++++++++++----------------- 2 files changed, 56 insertions(+), 21 deletions(-) diff --git a/chain/ethereum/src/tests.rs b/chain/ethereum/src/tests.rs index a5f0a17bff0..1d0b9efe8c3 100644 --- a/chain/ethereum/src/tests.rs +++ b/chain/ethereum/src/tests.rs @@ -235,3 +235,37 @@ fn test_trigger_dedup() { assert_eq!(block_with_triggers.trigger_data, expected); } + +#[test] +fn test_trigger_ordering_is_transitive() { + // Regression test: `EthereumTrigger::cmp` compared `Call` triggers (and `Call`/`Log` pairs) + // by `transaction_index`, but compared `Log`/`Log` pairs by `log_index` alone, ignoring + // `transaction_index`. Those two comparisons only agree when `log_index` happens to increase + // in lockstep with `transaction_index`, which does not hold for every trigger source (for + // example log/call triggers that were fetched independently and merged). When it doesn't + // hold, the combined ordering isn't transitive, which crashes Rust's sort with "user-provided + // comparison function does not correctly implement a total order". + // + // Here, `log_a` is in an earlier transaction than `log_c` but has a higher `log_index`: + let log_a = EthereumTrigger::Log(LogRef::FullLog(create_log(5, 50), None)); + let call_b = EthereumTrigger::Call(Arc::new(EthereumCall { + transaction_index: 5, + ..Default::default() + })); + let log_c = EthereumTrigger::Log(LogRef::FullLog(create_log(6, 10), None)); + + // `log_a` and `call_b` share a transaction, so `log_a < call_b` (events before calls in the + // same transaction). `call_b` is in an earlier transaction than `log_c`, so `call_b < log_c`. + // Transitivity requires `log_a < log_c`. + assert_eq!(log_a.cmp(&call_b), std::cmp::Ordering::Less); + assert_eq!(call_b.cmp(&log_c), std::cmp::Ordering::Less); + assert_eq!( + log_a.cmp(&log_c), + std::cmp::Ordering::Less, + "transitivity violated: log_a < call_b < log_c but log_a is not < log_c" + ); + + // The actual regression: sorting a `Vec` containing this combination used to panic. + let mut triggers = vec![log_c, call_b, log_a]; + triggers.sort(); +} diff --git a/chain/ethereum/src/trigger.rs b/chain/ethereum/src/trigger.rs index b5d51d9a379..785f5deaa62 100644 --- a/chain/ethereum/src/trigger.rs +++ b/chain/ethereum/src/trigger.rs @@ -378,27 +378,28 @@ impl Ord for EthereumTrigger { // Calls are ordered by their tx indexes (Self::Call(a), Self::Call(b)) => a.transaction_index.cmp(&b.transaction_index), - // Events are ordered by their log index - (Self::Log(a), Self::Log(b)) => a.log_index().cmp(&b.log_index()), - - // Calls vs. events are logged by their tx index; - // if they are from the same transaction, events come first - (Self::Call(a), Self::Log(b)) - if a.transaction_index == b.transaction_index().unwrap() => - { - Ordering::Greater - } - (Self::Log(a), Self::Call(b)) - if a.transaction_index().unwrap() == b.transaction_index => - { - Ordering::Less - } - (Self::Call(a), Self::Log(b)) => { - a.transaction_index.cmp(&b.transaction_index().unwrap()) - } - (Self::Log(a), Self::Call(b)) => { - a.transaction_index().unwrap().cmp(&b.transaction_index) - } + // Events are ordered by their tx index first, and by their log index within a + // transaction. Comparing by tx index first (instead of log index alone) keeps this + // consistent with the Call/Log orderings below, which also key on tx index first; + // log index is not guaranteed to increase in lockstep with tx index for every + // trigger source, and a mismatch between the two would make the overall ordering + // non-transitive. + (Self::Log(a), Self::Log(b)) => a + .transaction_index() + .cmp(&b.transaction_index()) + .then_with(|| a.log_index().cmp(&b.log_index())), + + // Calls vs. events are ordered by their tx index; if they are from the same + // transaction, events come first. + (Self::Call(a), Self::Log(b)) => a + .transaction_index + .cmp(&b.transaction_index().unwrap()) + .then(Ordering::Greater), + (Self::Log(a), Self::Call(b)) => a + .transaction_index() + .unwrap() + .cmp(&b.transaction_index) + .then(Ordering::Less), } } }