Conversation
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<Trigger>.
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.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6071
What is going on
EthereumTrigger::cmpcomparesCalltriggers (andCall/Logpairs) bytransaction_index, but comparesLog/Logpairs bylog_indexalone, ignoringtransaction_indexentirely:Those two comparisons only agree when
log_indexhappens to increase in lockstep withtransaction_indexfor every trigger in the set being sorted. That is not guaranteed for every trigger source. When it doesn't hold, the combined ordering stops being transitive.Concretely: a
Login an earlier transaction with a highlog_indexcompares as greater than aCallin the same transaction (correct: events come before calls in a transaction), and thatCallcompares as less than aLogin a later transaction (correct: earlier transaction sorts first) -- but the twoLogs compare directly bylog_indexalone, so the earlier-transactionLogcan come out greater than the later-transactionLogif itslog_indexhappens to be higher.a < b,b < c, buta > c.This is what crashes graph-node in production with:
inside
BlockWithTriggers::new_with_triggers, which callstrigger_data.sort()on a freshly builtVec<Trigger>.Fix
Key the
Log/Logcomparison ontransaction_indexfirst, falling back tolog_indexonly as a tie-breaker within the same transaction -- the same primary key already used by theCall/CallandCall/Logcomparisons. Also rewrote theCall/LogandLog/Callarms withOrdering::thento drop the duplicate guarded/unguarded arm pairs, since both arms now share the same primary-then-secondary-key shape.Testing
Added
test_trigger_ordering_is_transitiveinchain/ethereum/src/tests.rs, using the existingcreate_logtest fixture. It constructs the minimal counter-example (aLogin transaction 5 withlog_index50, aCallalso in transaction 5, and aLogin transaction 6 withlog_index10), asserts the three pairwise comparisons are consistent, and asserts sorting theVecdoesn't panic.Confirmed the test fails against the pre-fix code with exactly the expected assertion (
log_a.cmp(&log_c)returnsGreaterinstead ofLess), and passes after the fix.Ran
cargo test -p graph-chain-ethereum --lib(60 passed, including the two pre-existing trigger-ordering tests),cargo fmt -p graph-chain-ethereum -- --check,cargo clippy -p graph-chain-ethereum --lib -- -D warnings, andcargo check -p graph-chain-ethereum --release, all clean.