Skip to content

docs: Document TxUpdate temporal context requirements (supports Wizardsardine audit recommendation)#2135

Merged
evanlinjin merged 1 commit intobitcoindevkit:masterfrom
rafaelturon:docs/txupdate-temporal-context
Mar 12, 2026
Merged

docs: Document TxUpdate temporal context requirements (supports Wizardsardine audit recommendation)#2135
evanlinjin merged 1 commit intobitcoindevkit:masterfrom
rafaelturon:docs/txupdate-temporal-context

Conversation

@rafaelturon
Copy link
Copy Markdown
Contributor

@rafaelturon rafaelturon commented Mar 5, 2026

Closes #2133

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).

Description

This primarily affects developers building custom chain sources — anyone constructing Update structs outside of bdk_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 TxUpdate struct

  • anchors or seen_ats are stored in the graph but do not affect the balance.
  • seen_ats collection type: TxUpdate::seen_ats is a BTreeSet<(Txid, u64)>, requiring .insert((txid, timestamp)).

2. Doc comment on Wallet::apply_update()

  • TxGraph apply_update: transactions without temporal context note.
  • IndexedTxGraph apply_update: transactions without temporal context note.

Impact

This is just a documentation fix - no code changes, no breaking changes.

Checklists

All Submissions:

evanlinjin
evanlinjin requested changes Mar 6, 2026
View reviewed changes
Comment thread 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
Copy link
Copy Markdown
Member

@evanlinjin evanlinjin Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread 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.
Copy link
Copy Markdown
Member

@evanlinjin evanlinjin Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Custom chain sources must populate these fields.

Can remove this line. All chain sources should populate those fields.

Comment thread 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)`.
Copy link
Copy Markdown
Member

@evanlinjin evanlinjin Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HashSet hint is unnecessary. The type signature is right there on the field.

Comment thread crates/core/src/tx_update.rs Outdated
Comment on lines +23 to +24
/// - [`anchors`]: for confirmed transactions.
/// - [`seen_ats`]: for unconfirmed transactions.
Copy link
Copy Markdown
Member

@evanlinjin evanlinjin Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rafaelturon
Copy link
Copy Markdown
Contributor Author

rafaelturon commented Mar 6, 2026

Addressed all four review points:

  • Fixed doc links to Self::anchors / Self::seen_ats
  • "built-in chain" → "built-in chain-source crates"
  • Removed "Custom chain sources must populate these fields" (all chain sources should)
  • Removed HashSet type hint (redundant with field signature)

Verified with cargo doc -p bdk_core --no-deps — no new warnings
(the existing warning in spk_client.rs:140 is pre-existing and unrelated).

@rafaelturon rafaelturon marked this pull request as ready for review March 6, 2026 09:21
evanlinjin
evanlinjin requested changes Mar 8, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added docs look good. Can we merge the two commits?

@rafaelturon
Copy link
Copy Markdown
Contributor Author

rafaelturon commented Mar 8, 2026

Added docs look good. Can we merge the two commits?

Happy to squash! Just to confirm — do you want me to squash into a single commit before you merge, or would you prefer to squash-merge from your side?

@evanlinjin
Copy link
Copy Markdown
Member

evanlinjin commented Mar 10, 2026

@rafaelturon can you squash into single commit please.

@rafaelturon rafaelturon force-pushed the docs/txupdate-temporal-context branch from 4d9300f to 8ca6c43 Compare March 11, 2026 15:49
@rafaelturon
Copy link
Copy Markdown
Contributor Author

rafaelturon commented Mar 11, 2026

@evanlinjin Done! Squashed into a single commit.

@rafaelturon rafaelturon force-pushed the docs/txupdate-temporal-context branch 2 times, most recently from f035858 to fee292c Compare March 11, 2026 16:38
@rafaelturon @evanlinjin
docs: document TxUpdate temporal context requirements
b69d7bf 
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.

This fulfills the recommendation outlined in the Wizardsardine
BDK Audit Report (Q4 2024).

Signed-off-by: Rafael Turon <3598269+rafaelturon@users.noreply.github.com>
@evanlinjin evanlinjin force-pushed the docs/txupdate-temporal-context branch from fee292c to b69d7bf Compare March 12, 2026 14:39
evanlinjin
evanlinjin approved these changes Mar 12, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@evanlinjin evanlinjin left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b69d7bf

@rafaelturon I forced push to add more context to the apply_update docs.

@evanlinjin evanlinjin merged commit f2e7a78 into bitcoindevkit:master Mar 12, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@evanlinjin evanlinjin evanlinjin approved these changes

@oleonardolima oleonardolima Awaiting requested review from oleonardolima oleonardolima is a code owner

Assignees

No one assigned

Labels

None yet

Projects

BDK Chain
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

docs: Document TxUpdate temporal context requirements (supports Wizardsardine audit recommendation)

2 participants

@rafaelturon @evanlinjin