bdk icon indicating copy to clipboard operation
bdk copied to clipboard

Implement default index_tx method

Open vladimirfomene opened this issue 1 year ago • 4 comments

Description

This PR fixes #1097

Changelog

This PR implements a default implementation for the Indexer's index_tx method. It was recommended on one of the reviews of #1097.

Checklists

All Submissions:

  • [x] I've signed all my commits
  • [x] I followed the contribution guidelines
  • [x] I ran cargo fmt and cargo clippy before committing

Bugfixes:

  • [x] I'm linking the issue being fixed by this PR

vladimirfomene avatar Oct 12 '23 16:10 vladimirfomene

Can we include a # Changelog section in the PR description? Also please remove the checklist items that aren't needed for this PR (or mark them as completed).

evanlinjin avatar Oct 31 '23 03:10 evanlinjin

@LLFourn was your thinking here that we have a default implementation so that we can get rid of the implementation on KeychainTxOutIndex and SpkTxOutIndex? Sorry I should have asked this before even creating this PR.

vladimirfomene avatar Oct 31 '23 11:10 vladimirfomene

@LLFourn was your thinking here that we have a default implementation so that we can get rid of the implementation on KeychainTxOutIndex and SpkTxOutIndex? Sorry I should have asked this before even creating this PR.

Yes. Please reuse the default implementation where possible/viable.

evanlinjin avatar Nov 01 '23 21:11 evanlinjin

I want to register a last-minute Concept-NACK on this.

  • index_txout is really index_dangling_txout because it's only called when you insert a dangling txout. Dangling txouts trade off soundess for convenience. They are unsound because the server that told you about them may be fabricating them. They are a convenient feature since they can be used to figure out fees so you can have a consistent display to the user (ever tx in the list has a feerate). Furthermore, you can of course have a policy of only turning actual transactions into dangling txouts by first downloading the real transaction and then only keeping the txouts you're interested in.
  • This PR changes the defualt implementation of index_tx to index the txouts of a transaction the same way as you do for dangling txouts. This is a weird default. The implementer of trait should be asking themselves "how do I want to index txouts that I'm not sure even exist" but this PR hides this decision.
  • Our current indexer implementations do index dangling txout as if they existed and you could spend them -- this could be a mistake . At the very least I think there could be a boolean passed to the txout indexers to turn this off on and on depending on whether you want to fully trust the dangling utxos in your wallet.
  • You might object that this isn't a concern because servers like electrum can always lie about full transactions too but this isn't the case in theory -- you can request merkle proofs from electrum which have to be in a block and you can verify the proof of work of the block and even receive proof that the block is in the main chain. We need an issue to tackle this at some point.

LLFourn avatar Feb 05 '24 22:02 LLFourn