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

scan_without_keychain and scan's names should be inverted #1112

Closed
LLFourn opened this issue Sep 5, 2023 · 2 comments · Fixed by #1235
Closed

scan_without_keychain and scan's names should be inverted #1112

LLFourn opened this issue Sep 5, 2023 · 2 comments · Fixed by #1235
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@LLFourn
Copy link
Contributor

LLFourn commented Sep 5, 2023

There are some naming problems in electrum and esplora code.

  • scan should mean without a keychain
  • scan_with_keychain should be the name of the one that scans with a keychain

Same applies to update_tx_graph_without_keychain and update_tx_graph in esplora (which I would likewise rename to scan and scan_with_keychain).

The problem with the current naming is that it makes it seem scan (with a keychain) is the simple default and that scan_without_keychain is the exception. But scanning with a keychain (stop_gap) is very inefficient and should only be used when restoring from seedwords. Mutiny wallet caused a lowkey denial of service attack on mempool.space because they were always scanning with keychain.

  • it needs to be documented on scan_with_keychain that it should be used sparingly and explain the circumstances where it should be used.
  • I would remove all the other arguments from scan_with_keychain other than the keychain_spks in both esplora and electrum. This emphasizes the special case that it should be used for.
  • Perhaps we should change scan to sync instead since that's the nomenclature we have elsewhere.
@LLFourn LLFourn added the enhancement New feature or request label Sep 5, 2023
@LLFourn LLFourn added this to the 1.0.0-alpha.2 milestone Sep 5, 2023
@vladimirfomene
Copy link
Contributor

@LLFourn do you think this can be tagged "good first issue"?

@LLFourn
Copy link
Contributor Author

LLFourn commented Sep 7, 2023

sure but it should be done with high priority anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants