Skip to content

fix: use the validStart null flag in TransactionId.compareTo - #2921

Open
the2015 wants to merge 1 commit into
hiero-ledger:mainfrom
the2015:fix/2740-transactionid-compareto-validstart-null
Open

fix: use the validStart null flag in TransactionId.compareTo#2921
the2015 wants to merge 1 commit into
hiero-ledger:mainfrom
the2015:fix/2740-transactionid-compareto-validstart-null

Conversation

@the2015

@the2015 the2015 commented Aug 29, 2026

Copy link
Copy Markdown

Problem

TransactionId.compareTo has a copy-paste error in the validStart null-handling branch: it
returns based on thisAccountIdIsNull instead of thisStartIsNull.

var thisStartIsNull = (validStart == null);
var otherStartIsNull = (o.validStart == null);
if (thisStartIsNull != otherStartIsNull) {
    return thisAccountIdIsNull ? -1 : 1;   // wrong flag
}

Once the two ids agree on accountId nullness (which is required to reach this branch),
thisAccountIdIsNull is the same value for both sides, so the comparison returns the same sign in
both directions. That breaks the Comparable contract: sgn(a.compareTo(b)) == -sgn(b.compareTo(a)).

Fixes #2740

Fix

Use the flag that the branch is actually about:

if (thisStartIsNull != otherStartIsNull) {
    return thisStartIsNull ? -1 : 1;
}

Why existing tests did not catch it

TransactionIdTest.compare() has two cases where only one side has a validStart, but neither
reaches the buggy branch in a way that exposes it:

  • one case has accountId == null on both sides, so thisAccountIdIsNull happens to be true and
    -1 is coincidentally correct;
  • the other case has differing accountId nullness, so it returns earlier in the accountId block.

Verification

Added compareToIsAntisymmetricWhenOnlyOneValidStartIsNull, which asserts both directions with and
without an accountId.

As a negative control I temporarily reverted the one-line fix and re-ran the new test: it fails with
org.opentest4j.AssertionFailedError: expected: -1 but was: 1 (13 tests completed, 1 failed). With
the fix in place :sdk:test --tests '*TransactionIdTest*' is green (13 tests, 0 failures, 0 errors)
and :sdk:qualityCheck passes.
I also extracted the comparator logic into a standalone harness and exhaustively compared all
ordered pairs of the 18 distinct value combinations
(accountId in {null, 0.0.1, 0.0.2} x validStart in {null, t=100, t=200} x scheduled in {false, true}),
i.e. 324 pairs:

build antisymmetry violations transitivity violations
before 24 20
after 0 0

Concrete failures on the current main:

  • (null, null, false) vs (null, t=100, false) returns -1 in both directions.
  • (0.0.1, null, false) > (0.0.1, t=100, false) > (0.0.1, null, false) while
    (0.0.1, null, false).compareTo((0.0.1, null, false)) == 0, i.e. an intransitive cycle.

This matters in practice because TransactionId is Comparable and can end up in TreeSet,
TreeMap or List.sort, where an inconsistent comparator can silently drop entries or throw
IllegalArgumentException: Comparison method violates its general contract!.

Notes (deliberately out of scope)

  • compareTo does not consider nonce, while equals does, so compareTo == 0 does not imply
    equals. Comparable only recommends consistency with equals, and TransactionIdTest
    currently asserts the existing behaviour, so I left it alone.
  • Happy to split the test into a parameterized contract test if you prefer that style.

The validStart null-handling branch returned based on thisAccountIdIsNull instead of thisStartIsNull. Since that branch is only reachable when both ids agree on accountId nullness, the comparison returned the same sign in both directions, violating the Comparable antisymmetry and transitivity contracts.

Fixes hiero-ledger#2740

Signed-off-by: the2015 <35129808+the2015@users.noreply.github.com>
@the2015
the2015 force-pushed the fix/2740-transactionid-compareto-validstart-null branch from 78a37fc to 04be7ed Compare August 29, 2026 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TransactionId.compareTo() can violate Comparable contract when only one validStart is null

1 participant