Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement default index_tx method #1176

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vladimirfomene
Copy link
Contributor

@vladimirfomene vladimirfomene commented Oct 12, 2023

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:

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

Bugfixes:

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

@vladimirfomene vladimirfomene marked this pull request as draft October 12, 2023 16:23
@vladimirfomene vladimirfomene self-assigned this Oct 12, 2023
@vladimirfomene vladimirfomene force-pushed the impl_default_index_tx branch 3 times, most recently from 585986c to 90676b7 Compare October 13, 2023 17:46
@vladimirfomene vladimirfomene marked this pull request as ready for review October 13, 2023 17:49
@nondiremanuel nondiremanuel added this to the 1.0.0-alpha.3 milestone Oct 24, 2023
@evanlinjin
Copy link
Member

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

@vladimirfomene
Copy link
Contributor Author

vladimirfomene commented Oct 31, 2023

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

@evanlinjin
Copy link
Member

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

@notmandatory notmandatory modified the milestones: 1.0.0-alpha.3, 1.0.0-alpha.4 Nov 13, 2023
@nondiremanuel nondiremanuel modified the milestones: 1.0.0-alpha.4, 1.0.0 Jan 6, 2024
Co-authored-by: 志宇 <hello@evanlinjin.me>
Copy link
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.

self-ACK 680fe94

@LLFourn
Copy link
Contributor

LLFourn commented Feb 5, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module-blockchain new feature New feature or request
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

Indexer::index_tx can have a default impl in the trait.
5 participants