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

Rename electrum's sync/scan #1154

Closed
danielabrozzoni opened this issue Oct 6, 2023 · 9 comments
Closed

Rename electrum's sync/scan #1154

danielabrozzoni opened this issue Oct 6, 2023 · 9 comments
Labels
api A breaking API change module-blockchain
Milestone

Comments

@danielabrozzoni
Copy link
Member

We can probably have more descriptive names, and that make it easier to distinguish between them (I keep confusing them, maybe it's just me). I don't have any proposal in mind, but I'm opening the issue for future reference

@notmandatory notmandatory added this to BDK Oct 6, 2023
@notmandatory notmandatory moved this to Todo in BDK Oct 6, 2023
@notmandatory notmandatory added this to the 1.0.0-alpha.3 milestone Oct 6, 2023
@notmandatory
Copy link
Member

I added this to the alpha.3 milestone since we need to get questions like this figured out for bdk wallet API also (if we're going to change it).

@vladimirfomene
Copy link
Contributor

Good one @danielabrozzoni ! I usually also confuse them.

@danielabrozzoni
Copy link
Member Author

An explanation of scan and sync from @evanlinjin on Discord:

I also find it confusing actually. Think of scan as a "full sync" - we scan through every spk of each keychain and only stop when there is a "stop gap" count of spks that don't have txs.
sync is the "light sync". We look for transactions/updates which:

  • Spend our UTXOs
  • Send to our unused addresses.
  • Whether our unconfirmed tx gets confirmed.

Maybe fullsync/lightsync might be more explicative names?

@danielabrozzoni
Copy link
Member Author

Copy-pasting from discord to here for tracking purposes (https://discord.com/channels/753336465005608961/753367451319926827/1161271894679486476):

@evanlinjin:
cc @LLFourn , what was the original intention of using scan/sync?

@LLFourn:
The intention with the naming is that sync is the one you should use when you are synchronising local state with the Bitcoin network.
Scan is for when you do not want to use your local state because it is wrong for some reason.

@danielabrozzoni:
Mhhh, what about renaming scan to rescan?

@LLFourn:
The reason why sync/lightsync doesn't make sense to me is that there is nothing light about sync in my view. And with scan you are not really synchronising anything you are just going out and gathering information. That's what I was trying communicate.

@LLFourn:
Yes I like that. The main problem ATM is that the arguments to "scan" are not great. It should only have keychain spks and stop gap.
The main thing we have to succeed at is to communicate that you shouldn't be using "scan" on electrum or esplora all the time.
Ideally youd never use it unless you recover a wallet.
Rescan seems like a better name for this.

@vladimirfomene:
Why rescan though? Scan seems to make more sense to me than rescan. Maybe we just need to better document what each (scan/sync) means

@evanlinjin:
I like rescan better as it has the connotation of scanning from the start, which is exactly what it is doing. sync has the connotation of getting my data up to sync.

@danielabrozzoni:
Yeah, I like rescan because it's obvious from the name that it scans from the start, and thus you shouldn't do it unless you have a specific reason for doing so

@thunderbiscuit
Copy link
Member

thunderbiscuit commented Oct 24, 2023

Just my 2 sats (I don't want to bikeshed too much on this but since we're renaming anyway...).

To me, "rescan" implies an action supplementary to something you've already done. Calling rescan on the first boot of a wallet feels like a misnomer a little bit, and will have me look into the docs for the actual "scan" method (surely I should call the scan before the rescan... it must be there somewhere, or so my thinking would go).

When I think of the relative use of those methods, I see scan/rescan as something you only do once in a blue moon vs sync which you call all the time and maybe even have on some loop. The ratio is like 100:1, and in that sense I feel like the rescan/scan name should reflect the "gravity" of that call. Something like fullscan maybe would really set the methods apart. You call fullscan once, and after that it's all sync.

If "fullscan" or something like that is too much, my second preferred option is actually just a well documented scan. You scan once, then you sync. Rescan doesn't quite do if for me (sorry rescan team!). Just my thoughts.

@realeinherjar
Copy link
Contributor

Also I think we should check electrumx naming since we are defining an API that consumes from electrum.
We should have the user in mind, not us.
And what the user is familiar with.

So given this, we should differentiate scan vs sync, as the scan being the more "deep" one.
Or, in other words, the one that assume less state.
This is in agreement with @thunderbiscuit's You call fullscan once, and after that it's all sync.

These are my "two-sats" on the matter.

@notmandatory
Copy link
Member

my 2 sats, for reasons mentioned above, is we stick with scan and sync with clear docs on when to use each. :-) Is it OK if I just put a little poll on discord, people can multiple vote for all the ones they can live with? Options being:

  • scan, sync
  • fullscan, lightsync
  • rescan, sync
  • fullscan, sync

any others?

@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
@nondiremanuel
Copy link

Priority to be discussed further

@notmandatory notmandatory moved this from Todo to Done in BDK Mar 18, 2024
@notmandatory
Copy link
Member

fixed with with #1235

@notmandatory notmandatory added module-blockchain api A breaking API change labels Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change module-blockchain
Projects
Archived in project
Development

No branches or pull requests

6 participants