docs: Document TxUpdate temporal context requirements (supports Wizardsardine audit recommendation)#2135
Open
rafaelturon wants to merge 2 commits intobitcoindevkit:masterfrom
Open
Conversation
This introduces clear temporal context documentation to the `TxUpdate` struct, explicitly stating that entries must have either `anchors` or `seen_ats` to be considered canonical and contribute to wallet balances. It also explicitly documents the `seen_ats` HashSet signature to prevent usage errors when writing custom chain sources. This fulfills the recommendation outlined in the Wizardsardine BDK Audit Report (Q4 2024).
evanlinjin
requested changes
Mar 6, 2026
crates/core/src/tx_update.rs
Outdated
| /// - [`anchors`]: for confirmed transactions. | ||
| /// - [`seen_ats`]: for unconfirmed transactions. | ||
| /// | ||
| /// The built-in chain (`bdk_electrum`, `bdk_esplora`, `bdk_bitcoind_rpc`) handle this |
Member
There was a problem hiding this comment.
Suggested change
| /// The built-in chain (`bdk_electrum`, `bdk_esplora`, `bdk_bitcoind_rpc`) handle this | |
| /// The built-in chain-source crates (`bdk_electrum`, `bdk_esplora`, `bdk_bitcoind_rpc`) handle this |
crates/core/src/tx_update.rs
Outdated
| /// | ||
| /// The built-in chain (`bdk_electrum`, `bdk_esplora`, `bdk_bitcoind_rpc`) handle this | ||
| /// automatically. Transactions lacking temporal context are stored but ignored by canonicalization. | ||
| /// Custom chain sources must populate these fields. |
Member
There was a problem hiding this comment.
Suggested change
| /// Custom chain sources must populate these fields. |
Can remove this line. All chain sources should populate those fields.
crates/core/src/tx_update.rs
Outdated
| /// provide the `seen_at` value. | ||
| /// | ||
| /// Note: this is a `HashSet<(Txid, u64)>`, not a `HashMap`. | ||
| /// Insert with `.insert((txid, timestamp))`, not `.insert(txid, timestamp)`. |
Member
There was a problem hiding this comment.
The HashSet hint is unnecessary. The type signature is right there on the field.
crates/core/src/tx_update.rs
Outdated
Comment on lines
+23
to
+24
| /// - [`anchors`]: for confirmed transactions. | ||
| /// - [`seen_ats`]: for unconfirmed transactions. |
Member
There was a problem hiding this comment.
These don't look like valid doc links. I'm assuming they should be [`Self::anchors`] and [`Self::seen_ats`].
Run cargo doc to make sure.
Signed-off-by: Rafael Turon <3598269+rafaelturon@users.noreply.github.com>
Author
|
Addressed all four review points:
Verified with |
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
2 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.
Closes #2133
This introduces clear temporal context documentation to the
TxUpdatestruct, explicitly stating that entries must have eitheranchorsorseen_atsto be considered canonical and contribute to wallet balances.It also explicitly documents the
seen_atsHashSet signature to prevent usage errors when writing custom chain sources.This fulfills the recommendation outlined in the Wizardsardine BDK Audit Report (Q4 2024).
Description
This primarily affects developers building custom chain sources — anyone constructing
Updatestructs outside ofbdk_electrum/bdk_esplora/bdk_bitcoind_rpc.As the ecosystem grows (streaming Electrum, Nostr relay sync, compact block filters, custom backends), more developers will encounter this undocumented contract.
What I Changed
1. Doc comment on
TxUpdatestructanchorsorseen_atsare stored in the graph but do not affect the balance.seen_atscollection type:TxUpdate::seen_atsis aBTreeSet<(Txid, u64)>, requiring.insert((txid, timestamp)).2. Doc comment on
Wallet::apply_update()apply_update: transactions without temporal context note.apply_update: transactions without temporal context note.Impact
This is just a documentation fix - no code changes, no breaking changes.
Checklists
All Submissions: