Skip to content

Conversation

@evanlinjin
Copy link
Member

Description

This PR fixes incorrect ordering of ChainPosition that affected how wallet transactions are displayed. Previously, unconfirmed transactions that were never seen in the mempool would incorrectly appear before transactions with mempool timestamps due to the derived Ord implementation treating None as less than Some(_).

Problem

The derived Ord implementation for ChainPosition::Unconfirmed caused incorrect transaction ordering:

  • Unconfirmed { first_seen: None, last_seen: None } (never in mempool)
  • Would appear before Unconfirmed { first_seen: Some(10), last_seen: Some(10) } (seen in mempool)

This resulted in a confusing wallet display where transactions never broadcast would appear before pending mempool transactions.

Solution

Implemented manual Ord and PartialOrd traits for ChainPosition with the correct ordering:

  1. Confirmed transactions (earliest first by block height)
  2. Unconfirmed transactions seen in mempool (ordered by first_seen)
  3. Unconfirmed transactions never seen (these come last)

Additional ordering rules:

  • At the same confirmation height, transitive confirmations come before direct confirmations (as they may actually be confirmed earlier)
  • Tie-breaking uses transaction IDs for deterministic ordering

Changelog notice

Fixed

  • Fixed ChainPosition ordering so unconfirmed transactions never seen in mempool appear last instead of first

Changed

  • ChainPosition, CanonicalTx, and FullTxOut now require A: Ord for their Ord implementations
  • Simplified FullTxOut ordering to only use essential fields (chain_position, outpoint, spent_by)

Checklists

All Submissions:

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

…play

Previously, unconfirmed transactions never seen in mempool would appear
before those with mempool timestamps due to derived Ord implementation.

Changes:
- Manual Ord impl: confirmed < unconfirmed < never-in-mempool
- At same height: transitive confirmations < direct (potentially earlier)
- Simplify FullTxOut ordering to only essential fields
- Add comprehensive tests and documentation
- Update CanonicalTx and FullTxOut to use manual Ord with A: Ord bound
@evanlinjin evanlinjin self-assigned this Nov 21, 2025
@evanlinjin evanlinjin added the bug Something isn't working label Nov 21, 2025
@evanlinjin evanlinjin force-pushed the fix/better-chain-position-ord branch from affec6a to d6593da Compare November 21, 2025 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant