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

[GUI] Don't treat wallets using bitcoind as syncing #1387

Conversation

jp1ac4
Copy link
Collaborator

@jp1ac4 jp1ac4 commented Oct 18, 2024

This is a follow-up to #1370.

If the user has imported a descriptor and is using bitcoind as a local node, then they will need to perform a rescan in order
to see past transactions. Treating the wallet as syncing in this case could mislead the user that a rescan is being performed. Therefore, it's better to keep the past behaviour here to avoid further confusion.

@jp1ac4 jp1ac4 marked this pull request as draft October 19, 2024 07:45
@jp1ac4 jp1ac4 force-pushed the gui-dont-show-syncing-for-local-bitcoind branch from bda97cf to b2ca118 Compare October 19, 2024 11:01
@jp1ac4 jp1ac4 marked this pull request as ready for review October 19, 2024 12:42
@edouardparis
Copy link
Member

edouardparis commented Oct 21, 2024

I think you can modify DaemonBackend for:

use crate::node::NodeType;

#[derive(Debug, Clone, PartialEq, Eq)]
pub enum DaemonBackend {
    EmbeddedLianad(NodeType),
    ExternalLianad,
    RemoteBackend,
}

It may be more practical for the gui, wdyt ?

@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Oct 21, 2024

It may be more practical for the gui, wdyt ?

That looks good, I'll change it to that.

Do you think it would make sense at some point to add "node_type" to getinfo ? In that case, we would also know for an external daemon, but it might not be worth making such a change just for this use case of opening a wallet for the first time.

@edouardparis
Copy link
Member

Do you think it would make sense at some point to add "node_type" to getinfo ? In that case, we would also know for an external daemon, but it might not be worth making such a change just for this use case of opening a wallet for the first time.

Yes, I believe it is not worth it for now.

This is for convenience in case we need to know the node type
being used.

Although we expect there to always be a node type, it is kept as
`Option` to be consistent with the daemon config.
If the user has imported a descriptor and is using bitcoind as
a local node, then they will need to perform a rescan in order
to see past transactions.

Treating the wallet as syncing in this case could mislead the
user that a rescan is being performed. Therefore, it's better
to keep the past behaviour here to avoid further confusion.
@jp1ac4 jp1ac4 force-pushed the gui-dont-show-syncing-for-local-bitcoind branch from b2ca118 to 9208cd1 Compare October 21, 2024 10:24
@jp1ac4
Copy link
Collaborator Author

jp1ac4 commented Oct 21, 2024

I've made the suggested change except I decided to use Option<NodeType> just to be on the safe side as the config has Option<BitcoinBackend>. We could consider changing that in future, but for this current change I didn't want to risk introducing a panic.

Copy link
Member

@edouardparis edouardparis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 9208cd1

@edouardparis edouardparis merged commit 1c8df3e into wizardsardine:master Oct 22, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants