Skip to content

docs(esplora): replace misleading tx-cache TODO with explanation - #2261

Open
Arowolokehinde wants to merge 1 commit into
bitcoindevkit:masterfrom
Arowolokehinde:fix/esplora-remove-tx-cache-todo
Open

docs(esplora): replace misleading tx-cache TODO with explanation#2261
Arowolokehinde wants to merge 1 commit into
bitcoindevkit:masterfrom
Arowolokehinde:fix/esplora-remove-tx-cache-todo

Conversation

@Arowolokehinde

@Arowolokehinde Arowolokehinde commented Aug 18, 2026

Copy link
Copy Markdown

Description

fetch_txs_with_outpoints carried a misleading // TODO: We should maintain a tx cache (like we do with Electrum). comment in both async_ext.rs and blocking_ext.rs. Electrum needs a cache because its history call returns txids only, forcing a follow-up fetch per tx. Esplora's scan endpoint already returns full tx bodies, so there's no equivalent fetch for a cache to skip — and inserted_txs already dedupes against what the spk phase fetched earlier in the same sync.

This PR replaces the TODO with a comment explaining why, and why the obvious "fix" — using get_tx_status instead of get_tx_info for cache hits — is unsafe: unknown txids return HTTP 200 {"confirmed":false}, indistinguishable from a real unconfirmed tx, so evicted transactions would never be evicted.

Fixes #2260

Notes to the reviewers

The TODO already cost a contributor a 600-line PR (#2254, closing #2250) implementing exactly this cache, which was caught in review for the get_tx_status eviction bug described above. This comment exists to stop that from being rediscovered a third time.

Changelog notice

Removed a misleading TODO in fetch_txs_with_outpoints suggesting a tx cache be added; replaced with a comment explaining why the Electrum-style cache pattern does not apply to Esplora.

Checklists

All Submissions:

@luisschwab luisschwab added the documentation Improvements or additions to documentation label Aug 18, 2026
@luisschwab luisschwab moved this to Needs Review in BDK Chain Aug 18, 2026
@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.36%. Comparing base (456f9b7) to head (1aa2ece).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2261   +/-   ##
=======================================
  Coverage   78.36%   78.36%           
=======================================
  Files          30       30           
  Lines        5945     5945           
  Branches      281      281           
=======================================
  Hits         4659     4659           
  Misses       1210     1210           
  Partials       76       76           
Flag Coverage Δ
rust 78.36% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@luisschwab

Copy link
Copy Markdown
Member

I'd just add a short comment stating that a tx cache does not apply in this scenario, and change the TODO to say that the per-outpoint get_output_status round trips can be optimized.

@Arowolokehinde

Copy link
Copy Markdown
Author

Changes has been made as required to the comment @luisschwab

@luisschwab

Copy link
Copy Markdown
Member

The commits should be squashed

@Arowolokehinde
Arowolokehinde force-pushed the fix/esplora-remove-tx-cache-todo branch from fe79cea to 46015d5 Compare August 24, 2026 20:17
@Arowolokehinde
Arowolokehinde force-pushed the fix/esplora-remove-tx-cache-todo branch from 46015d5 to a81add9 Compare August 24, 2026 20:20
@Arowolokehinde

Copy link
Copy Markdown
Author

i just squashed the commit
Thanks for the review

@Arowolokehinde

Copy link
Copy Markdown
Author

@luisschwab kindly review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

esplora: remove misleading TODO about maintaining a tx cache

2 participants