Fix ChainPosition ordering#2074
Merged
evanlinjin merged 2 commits intobitcoindevkit:masterfrom Jan 21, 2026
Merged
Conversation
affec6a to
d6593da
Compare
ValuedMammal
approved these changes
Dec 26, 2025
d6593da to
72b30d2
Compare
Member
Author
|
I rebased on master and simplified the |
aagbotemi
reviewed
Jan 5, 2026
Contributor
aagbotemi
left a comment
There was a problem hiding this comment.
@evanlinjin thank you for simplifying the Ord logic. I think it would be nice to add a test checking when the first_seen is equal and the last_seen is different, to explicitly verify the tiebreaker logic.
87899cf to
85bfcdc
Compare
Member
Author
|
@aagbotemi Thank you for the suggestion. There was actually a slight bug in the logic. I fixed it and simplified the |
…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
85bfcdc to
1cc7d60
Compare
Member
Author
|
@oleonardolima I rebased on master. Feel free to ACK and merge. |
4 tasks
evanlinjin
added a commit
that referenced
this pull request
Mar 12, 2026
aff800d fix(chain): correct unconfirmed `ChainPosition` `last_seen` tiebreaker (志宇) 9d2dedc fix(chain): correct ChainPosition ordering for wallet transaction display (志宇) Pull request description: ## Description Back-ported from #2074 Depends on #2148 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 ```md ### 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: * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) ### Bugfixes: * [ ] This pull request breaks the existing API * [x] I've added tests to reproduce the issue which are now passing * [ ] I'm linking the issue being fixed by this PR Top commit has no ACKs. Tree-SHA512: eccdf30a47c7e8ef40312f1dd4bf72ce42a3e26a1dd1e6d1f7dc4776350c99029408474439730256414bd0f173e755977a3e3deb81617a930e40ca7962bd490b
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
4 participants
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.
Description
This PR fixes incorrect ordering of
ChainPositionthat 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 derivedOrdimplementation treatingNoneas less thanSome(_).Problem
The derived
Ordimplementation forChainPosition::Unconfirmedcaused incorrect transaction ordering:Unconfirmed { first_seen: None, last_seen: None }(never in mempool)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
OrdandPartialOrdtraits forChainPositionwith the correct ordering:first_seen)Additional ordering rules:
Changelog notice
Fixed
ChainPositionordering so unconfirmed transactions never seen in mempool appear last instead of firstChanged
ChainPosition,CanonicalTx, andFullTxOutnow requireA: Ordfor theirOrdimplementationsFullTxOutordering to only use essential fields (chain_position, outpoint, spent_by)Checklists
All Submissions:
Bugfixes: