search

wizardsardine / liana

The missing safety net for your coins
https://wizardsardine.com/liana
BSD 3-Clause "New" or "Revised" License
308 stars 55 forks source link

Automatically startup and download `bitcoind` when starting up the GUI #570

Closed darosior closed 1 year ago

darosior commented 1 year ago

If bitcoind is not already running when the GUI is started, we should run it. If we can't find any bitcoind binary, we should download it from https://bitcoincore.org/bin/bitcoin-core-25.0/.

There is a number of things to be taken into account. Here is a non exhaustive list:

darosior commented 1 year ago

@jp1ac4 will take care of this. Anyone feel free to chime in about the approach.

darosior commented 1 year ago

Meta point: the development could be split in two PRs: 1) find and startup bitcoind 2) download bitcoind if we can't find it.

darosior commented 1 year ago

Related: https://github.com/wizardsardine/liana/issues/101.

pythcoiner commented 1 year ago
  • What warning should appear about those in the GUI (like "around 20G of space will be used")? Any other check (like checking available storage)?

do we have the ability to check if the space available is on SSD rather than HDD? (I guess users will not wait several weeks for IBD)

pythcoiner commented 1 year ago
  • What if there is an existing data directory but we can't find any bitcoind binary?

At least ask the user to confirm that we can use data from this directory?

pythcoiner commented 1 year ago
  • What if there is an existing data directory, we can find a bitcoind binary, but not a bitcoin.conf?

under Linux distro using systems, should check if there is some bitcoind.service file in /etc/systemd/system and /lib/systemd/system

edit: still under Linux check under /snap/bitcoin-core (for binary and data I guess)

pythcoiner commented 1 year ago
  • Where should the bitcoin.conf live? In general it's cleaner if we keep the files we write into our own data directory, but then if we don't write the bitcoin.conf within bitcoind's data directory it kills the purpose of having a bitcoin.conf in the first place. (For instance, we download bitcoind, start it up pruned, it syncs, and later the user starts bitcoind again with no knowledge of the bitcoin.conf we used and it starts IBD again because they didn't specify -prune.)

under Linux I guess in ~/.bitcoin

pythcoiner commented 1 year ago
  • Should we go for a more radical option and have the bitcoind data directory live inside our own data directory? So that it would prevent most footguns, but could be a waste of disk space if for some reason the user starts using a bitcoind with a regular data directory.

I don't think so, what if the user want to uninstall Liana and remove (unwantedly) Bitcoin data?

pythcoiner commented 1 year ago
  • Where should we store the bitcoind binary we download?

under linux I guess in /usr/bin

pythcoiner commented 1 year ago

@jp1ac4 will take care of this. Anyone feel free to chime in about the approach.

for Linux using .deb package some stuff can be handled directly there, I can help if needed , @jp1ac4 feel free to ping me

darosior commented 1 year ago

do we have the ability to check if the space available is on SSD rather than HDD?

I haven't looked into it but i assume we'd look for the space available on the partition on which the path we'd store the datadir at resides. So the physical type of the drive shouldn't matter.

(I guess users will not wait several weeks for IBD)

For now they have to anyways. AssumeUtxo (linked above) could help with that.

-------- Original Message -------- On Jul 13, 2023, 9:22 PM, pythcoiner wrote:

  • What warning should appear about those in the GUI (like "around 20G of space will be used")? Any other check (like checking available storage)?

do we have the ability to check if the space available is on SSD rather than HDD? (I guess users will not wait several weeks for IBD)

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

pythcoiner commented 1 year ago

do we have the ability to check if the space available is on SSD rather than HDD? I haven't looked into it but i assume we'd look for the space available on the partition on which the path we'd store the datadir at resides. So the physical type of the drive shouldn't matter. (I guess users will not wait several weeks for IBD) For now they have to anyways. AssumeUtxo (linked above) could help with that. …

my mean, it will be useful to at least hint to users that datadir location on HDD will be the cause of several weeks IBD, so, the user can decide to move to another machine or wait

kloaec commented 1 year ago
  • What options should we start bitcoind with? Pruned? By how much? Lower cache?
  • Pruned to ~1GB or so
  • dbCache to >50% of available RAM on the machine (needs to detect it)

I think both options should be "editable" maybe locked by default, for users who know what they are doing. Regarding the pruning, I think 6GB (1 for the blocks, 5 for UTXOs) is already a lot for basic users. Checking available space BEFORE we start the process is also important imho.

  • What warning should appear about those in the GUI (like "around 20G of space will be used")? Any other check (like checking available storage)?

10GB seems already a lot, unless they are very comfortable with more.

Regarding the HDD vs SSD, I don't think it matters so much as to TELLING the user that it will be long anyways. 2 (full) days with SSD, a lot more with HDD. We could tell the user to move their Liana to ssd entirely if they have both, instead of doing a specific thing for core? not sure. We should also notify the user this will download 500gb of data. This is important if they are on a metered connection (usa and other third world countries :D ).

Should we keep Core running for initial IBD even when Liana is closed?

pythcoiner commented 1 year ago
  • What warning should appear about those in the GUI (like "around 20G of space will be used")? Any other check (like checking available storage)?

don't bitcoind do a full IBD before pruning? in that case, the user needs 550G during IDB and then 20/10G after pruning.

darosior commented 1 year ago

Pruned to ~1GB or so

That's risky. It's like what, 4 days of full blocks? To synchronize with the chain Liana loads a watchonly wallet on Bitcoin Core. Upon loading it will rescan the blocks since it was last loaded. To do this it needs the block data. This means a user needs not keep bitcoind running without the Liana watchonly wallet loaded for more than 4 days and that they must load the Liana watchonly wallet at startup to sync it before the historical data gets deleted. Otherwise they would have to reindex the whole chain with the wallet loaded (which'd take almost as long as an IBD).

I was thinking of something like 20G, which would be around 2 months and a half of full blocks. And i would err more on the side of not being too aggressive: disk space is cheap nowadays, being stuck and having to manually reindex through the command line is not.

dbCache to >50% of available RAM on the machine (needs to detect it)

Why? This is huge and i don't think it is necessary. Rather the contrary, i would try to limit resources consumption (but then it's a tradeoff with writing more to disk, which is the bottleneck for IBD i think).

I think both options should be "editable" maybe locked by default, for users who know what they are doing.

If you are going to start tweaking with bitcoind's parameter you might as well run your own.. I don't think we should have any option for this: we are just trying to find reasonable defaults for most of our non-technical users.

Should we keep Core running for initial IBD even when Liana is closed?

Good question. Without thinking more about it i'd lean more on the side of cleanly shutting it down when Liana is closed, as otherwise a user could just stop their machine without cleanly shutting down bitcoind, which could result in a partially written cache and therefore an even longer IBD.

don't bitcoind do a full IBD before pruning? in that case, the user needs 550G during IDB and then 20/10G after pruning.

Historical data is deleted as we go. Note this also means that a lower prune target makes IBD marginally slower.

pythcoiner commented 1 year ago

Good question. Without thinking more about it i'd lean more on the side of cleanly shutting it down when Liana is closed, as otherwise a user could just stop their machine without cleanly shutting down bitcoind, which could result in a partially written cache and therefore an even longer IBD.

under Linux (systemd) I guess we can handle that using the poweroff.target

jp1ac4 commented 1 year ago

Currently, when installing Liana for the first time on a network, the bitcoind connection check comes towards the end after the descriptors have been defined. On the other hand, if Liana was previously installed on a network, an error will occur when trying to start the Liana daemon if bitcoind is not running. I'm thinking that in both cases, there could be a common first step to check the bitcoind connection and then provide options if it's not running/installed etc.

This could be an extension of the current Bitcoin Core settings page or perhaps a standalone set of steps to run/install bitcoind.

darosior commented 1 year ago

I'm on the fence because while i agree we could have this step in common (if it's reworked a bit), we probably always want to show the bitcoind connection configuration in the installer. Let's see what it could look like if we don't think it's necessary.

When arriving to this step:

The reworked "bitcoind configuration" step would have the following look and behaviour. First of all it would look for the bitcoind binary in pre-defined location(s) and attempt to start it. It would likely display a message like "Starting bitcoind.." to the user. If it could find and start it successfully, proceed. If it couldn't find it, tell the user and ask them whether we should download it. If it couldn't start it for whatever reason inform the user with an error message. In both those cases also have the option to set the ip, port and datadir as we do currently to connect to a different bitcoind. But have it behind a dropdown.

What do you think? Does that make sense to jump-through during the installer if a bitcoind's running? Does that make sense to try starting up bitcoind automatically?

jp1ac4 commented 1 year ago

I'm on the fence because while i agree we could have this step in common (if it's reworked a bit), we probably always want to show the bitcoind connection configuration in the installer.

That sounds fine. I think the main thing is to reuse the same screens (where possible) to configure bitcoind in both cases, even if they are shown at different steps depending on installation vs "regular startup".

On a related note, I wonder if at some point it would be worth separating the wallet creation with its descriptors from the bitcoind connection, as you might want to define the wallet without having any connection.

Does that make sense to jump-through during the installer if a bitcoind's running?

I think it would still be worth asking for confirmation from the user that the pre-populated configuration is the required one.

Does that make sense to try starting up bitcoind automatically?

I've been thinking about the following:

Perhaps here we should focus on running a minimal "throwaway" node required for running Liana just in the present moment. This would be mainly for those users who don't intend to run their own node or are not able to connect to their usual node at the present moment and just want to quickly use Liana. We could define a bitcoin.conf file within Liana's data directory and store data there, and ignore any other data/configuration the user may have. This data could still be retained for subsequent occasions so that they don't have to repeat any (pruned) IBD etc. What do you think?

darosior commented 1 year ago

On a related note, I wonder if at some point it would be worth separating the wallet creation with its descriptors from the bitcoind connection, as you might want to define the wallet without having any connection.

Yeah i was initially kinda opposed to this. But i've been recently considering for multisig scenarii where cosigners shouldn't be required to run a bitcoind.

I think it would still be worth asking for confirmation from the user that the pre-populated configuration is the required one.

In my opinion if there is already a configuration, we should use it. If there is none and no datadir we should create our own. But i don't feel too strongly about it.

there are many different configuration options to run bitcoind a user may use different configuration options depending on their location or what actions they wish to perform

Yes and we're compatible with all of them? It's not an issue if the user want to tweak their config, as long as they know what they are doing. Could you expand on why it could come to be an issue?

[..] Use our own minimal bitcoind in our own datadir

This is pretty compelling. One issue i can see in doing this is if they ever want to use a bitcoind for something else, it could clash for ports and it would duplicate disk space. Ports issues are fixable and using a couple dozens GB of disk space shouldn't be treated as an issue IMO. An advantage of this approach is that on Windows we keep our watchonly wallet back into our own datadir.

So i'd say go for this approach. If there is no existing datadir at the default location, create a new bitcoind_datadir folder in our datadir and a bitcoin.conf inside it. Then start bitcoind using this bitcoin.conf. And configure lianad to talk to this bitcoind. The bitcoin.conf should have prune=15000 and non-standard ports should be set for RPC and P2P.

darosior commented 1 year ago

Also @kloaec mentioned we could create watchonly wallets with load_on_startup set to true to avoid the issue i mentioned above re aggressive pruning. Though i still think being too aggressive on pruning wouldn't worth it.

jp1ac4 commented 1 year ago

In my opinion if there is already a configuration, we should use it. If there is none and no datadir we should create our own. But i don't feel too strongly about it.

I'm happy to go with this approach (see also my comments below about technical users and choosing the right configuration options).

Could you expand on why it could come to be an issue?

My main concern was that for more technical users, if we try to find their existing config and start bitcoind for them, we may choose options they don't want at that particular moment, e.g. if we choose the wrong bitcoin.conf file, so these users might prefer to start bitcoind themselves. On the other hand, less technical users may be happy if we manage bitcoind for them and make sensible configuration choices.

I suppose it boils down to identifying different user personas and understanding their requirements. If we focus here on less technical users who don't want to manage bitcoind themselves, it may simplify the design and implementation.

So i'd say go for this approach. If there is no existing datadir at the default location, create a new bitcoind_datadir folder in our datadir and a bitcoin.conf inside it. Then start bitcoind using this bitcoin.conf. And configure lianad to talk to this bitcoind. The bitcoin.conf should have prune=15000 and non-standard ports should be set for RPC and P2P.

That sounds good to me.

we could create watchonly wallets with load_on_startup set to true to avoid the issue i mentioned above re aggressive pruning

I'll take a look at that.

darosior commented 1 year ago

if we try to find their existing config and start bitcoind for them

Yeah my thinking was that we'd use their own bitcoin.conf if a bitcoind datadir already exist, and would bail out if we can't find it. But really the "segregated minimal bitcoind" approach seems best to me.

I'll take a look at that.

Sure, it's a bit orthogonal but should also be trivial (basically setting an argument to true in our call to importdescriptors in src/bitcoin/d/mod.rs).

darosior commented 1 year ago

we could create watchonly wallets with load_on_startup set to true to avoid the issue i mentioned above re aggressive pruning

I'll take a look at that.

Sure, it's a bit orthogonal but should also be trivial (basically setting an argument to true in our call to importdescriptors in src/bitcoin/d/mod.rs).

I'll take care of this, it's starting to be pressing as i'd like to have it in a 1.1 bugfix release.

jp1ac4 commented 1 year ago

OK thanks.

Regarding the other changes, I'm working in this branch: https://github.com/jp1ac4/liana/tree/start-bitcoind-from-liana-gui.

I've updated the installer so that it creates bitcoin.conf and starts bitcoind from an existing binary (will provide download option in separate PR). It's still in progress and will need refactoring, but you can get the main idea.

I'm using the toml crate for writing to bitcoin.conf, but that outputs the datadir value with quotes, whereas we need it without quotes so I may need to use a different approach.

I was planning to use a single bitcoin.conf file for all networks with corresponding network sections, or do you think it would be better to use a separate file for each network?

darosior commented 1 year ago

I'm using the toml crate for writing to bitcoin.conf, but that outputs the datadir value with quotes, whereas we need it without quotes so I may need to use a different approach.

Yeah bitcoin.conf uses the .ini syntax, not toml. In my opinion you'd rather write to the file directly instead of using any library. It's only a couple of lines anyways.

I was planning to use a single bitcoin.conf file for all networks with corresponding network sections, or do you think it would be better to use a separate file for each network?

Good question. I'd say a single bitcoin.conf / Bitcoin datadir for all networks is the cleanest, but if it's harder to work out it's no big deal to use 3 different folders.

jp1ac4 commented 1 year ago

In my opinion you'd rather write to the file directly instead of using any library. It's only a couple of lines anyways.

Yep, I think this will be the easiest approach.

I'd say a single bitcoin.conf / Bitcoin datadir for all networks is the cleanest, but if it's harder to work out it's no big deal to use 3 different folders.

I agree. I'll continue with a single file (~/.liana/bitcoind_datadir/bitcoin.conf) for now and see how it goes.

BenWestgate commented 1 year ago

do we have the ability to check if the space available is on SSD rather than HDD? (I guess users will not wait several weeks for IBD)

Yeah, absolutely, perform a random 4k read test on the data directory, really chainstate directory's device. If it's a HDD you will get 100-200 max IOPS, and if it is a SSD or even good USB flash drive you will get thousands.

The random read performance of storage often becomes the bottle neck when the machine doesn't have enough RAM to keep the chainstate database entirely in memory.

You can sync very fast on a HDD if you have 16 or 24GB of memory and a high -dbcache. But with 4 or 8 it will become glacial once the disk chainstate needs to be read to connect most blocks.

jp1ac4 commented 1 year ago

@darosior For parsing an existing config file, it might be convenient to use https://github.com/zonyitoo/rust-ini, or would you prefer not to add any dependencies for this?

jp1ac4 commented 1 year ago

Note that if we use a separate bitcoin.conf file for each network, it might not be necessary to read any existing file as existing values for that network could just be overwritten (e.g. this might happen if a user deletes their Liana wallet for a given network without removing their bitcoin.conf). If we use a single bitcoin.conf for all networks, we will need to retain existing values for other networks when updating the file for the selected network.

darosior commented 1 year ago

From a quick skim rust-ini looks reasonable. So i think it's better to use it rather than reinventing the wheel.

darosior commented 1 year ago

@jp1ac4 what's your ETA for the PR? I'd like to proceed with a feature freeze for v2 around the 15th of August and what you're working on is a must have. :)

jp1ac4 commented 1 year ago

I think I'll be ready to create a draft PR within a few days with the main logic in place for configuring and starting an already-installed bitcoind.

I have an updated Liana installer currently "working" in https://github.com/jp1ac4/liana/tree/start-bitcoind-from-liana-gui. The main things missing are error handling and tests, plus general refactoring, but you should be able to get an idea of how it's working and if any major changes in approach are required.

darosior commented 1 year ago

We might be interested in using daemonwait in place of daemon (see https://github.com/bitcoin/bitcoin/pull/21007). This allows for catching startup errors more easily.

kloaec commented 1 year ago

I am leaving my comments about the current state of the PR, as wording and maybe order of things needs to change a bit.

On this: image

"Liana requires a Bitcoin node running. Do you prefer to: a- Manage your own Bitcoin node b- Have Liana manage and run a dedicated Bitcoin node

Info: a- Liana will connect to your existing instance of bitcoind. You will have to make sure Bitcoin is running when you use Liana. (Use this if you already have a full node on your machine, and don't need a new instance) b- Liana will run its own instance of Bitcoin Core. This will use a pruned node, and perform the syncronization in the Liana folder. (Use this if you don't want to deal with Bitcoin Core yourself, or need a new, dedicated instance for Liana)

I would also prefer to have a detection step of Core present on the machine or not BEFORE this step, so we can avoid overwhelming newbies with technical questions. If core is detected, offer these 2 choices with probably a default in "a". If core isn't detected, probably just ask if they want to install it and go for "b". Sorry this is a bit in-between 2 different issues (starting, and installing).

darosior commented 1 year ago

Thanks for the comments.

On this:

That's not the current state of the PR. This is: image

Info:

You mean those sentences should be in place of the greyed-out text atop the buttons?

I would also prefer to have a detection step of Core present on the machine or not BEFORE this step, so we can avoid overwhelming newbies with technical questions.

  1. I agree, we can track this is another issue for a future release.
  2. It's already a big leap in the right direction. "Liana needs a Bitcoin node. Would you like me to manage it for you? :hugs:" is much better than the existing immediate "YO WHATS YOUR BITCOIND IP PORT AND COOKIE FILE".

Sorry this is a bit in-between 2 different issues (starting, and installing).

That's totally appropriate here, keep them coming!

Something else we discussed offline with Kevin: would be nice to link from the manual bicoind connection setup step to the "managed bitcoind" step if no connection to bitcoind is detected.

kloaec commented 1 year ago

Ok, the current one is pretty good too. Up you you guys to choose.

For the extra info, I am not sure how to display it. Maybe instead of "buttons" we can have 2 big ass rectangles, with all the info in it (kinda like when you choose a pricing on a website, which "tier" do you want to use?)

jp1ac4 commented 1 year ago

For the extra info, I am not sure how to display it. Maybe instead of "buttons" we can have 2 big ass rectangles, with all the info in it (kinda like when you choose a pricing on a website, which "tier" do you want to use?)

My initial idea was to have a radio button / checkbox on the left-hand side, with the corresponding description on the right-hand side. So there would be two rows, one for each option, which should give more space for longer descriptions. But I'm also happy to go with your suggestion.

darosior commented 1 year ago

The first part (bitcoind management within the GUI) was merged. Here are a couple thoughts about the second part (download of the bitcoind binary if missing).

You'll need a way of working with Zip and Tar archives. You'll need a way to SHA256 a file, you can probably read the file content and reuse rust-bitcoin's sha256 implementation? If it's too involved feel free to suggest otherwise.

Anything i could be missing here? cc @edouardparis @kloaec.

jp1ac4 commented 1 year ago

Where should we store the binary? I would suggest storing it in our data directory.

That sounds good. Would you suggest in bitcoind_datadir or one level up in Liana's data directory?

You'll need a way of working with Zip and Tar archives. You'll need a way to SHA256 a file, you can probably read the file content and reuse rust-bitcoin's sha256 implementation?

Yep, I think I can use https://github.com/rust-bitcoin/rust-bitcoin/tree/master/hashes.

darosior commented 1 year ago

I'd say one level up.

kloaec commented 1 year ago

I'll create a separate issue about "updates" of the software. But regarding Bitcoin Core, we should think about how we choose versions, and what happens if/when some are obsolete (happens when there is a bug and it's replaced by a patch version typically).

The option of hardcoding the SHA256 prevents us to deal with this, without releasing a new Liana version. If the binaries are removed from Bitcoincore.org, our software would break.

An alternative would be to check against a bunch of signatures from coredevs, and we grab the Shasum file + latest release on bitcoincore.org . We can hardcode the pubkeys of some coredevs in our release, similar to hardcoding the shasum in terms of security.

The issue with this alternative option is that we may break other things, if a release of Core isn't compatible with Liana for some reason. Maybe we could restrict this to minor releases of Core (i.e., looking for 25.x only, not 26+) for each Liana release.

darosior commented 1 year ago

If the binaries were removed from bitcoincore.org, this functionality would be unavailable (which is for the better: we wouldn't want people to download a vulnerable binary). I don't think it's worth trying to be smart by guessing what the latest release is (how would we even do that, Github API?) nor checking GPG sigs. Just checking a hash is lower tech and the probability of a binary being removed is IMO far lower than the risk of installing an incompatible new version of Bitcoin Core.

However there may be a point to nudge users to update Liana if the download fails. But then again presumably at this step they just downloaded the software. So to run into the error they would have to either:

  1. Download an ancient version of Liana pointing to an old version of Bitcoin Core + this version of Bitcoin Core is removed from the download page
  2. Download Liana, not install it for months (years?), then use the installer + the version of Bitcoin Core was removed

This seems quite unrealistic to me.

kloaec commented 1 year ago

It could also be that it's bad timing, latest version of Liana but with a Core version that doesn't exist anymore. That would cause problems for however long we need to put the fix, probably less than 24h?

If looking for a latest version of Core is out of question, we could also have a "fall back" Core version, assuming the bug is only with the latest release. Maybe not useful though, i'm just trying to think about edge cases.

I assume Bitcoincore.org being unreachable is covered already? Should we look at mirrors or other sources before throwing an error?

darosior commented 1 year ago

The fallback to a lower version could make sense, although it sounds a bit overthought to me. (Again, this is super not probable.)

Mirrors definitely make sense, although it's not necessary for now. We should create a dedicated issue for this to be addressed in a later version.

darosior commented 1 year ago

Following up on the text edits from @kloaec.

For the extra info, I am not sure how to display it. Maybe instead of "buttons" we can have 2 big ass rectangles, with all the info in it (kinda like when you choose a pricing on a website, which "tier" do you want to use?)

My initial idea was to have a radio button / checkbox on the left-hand side, with the corresponding description on the right-hand side. So there would be two rows, one for each option, which should give more space for longer descriptions. But I'm also happy to go with your suggestion.

Both are fine by me. I feel like Kevin's idea would be a bit fancier, if that's not too hard to implement. But the current screen is also fine. Let's keep it this way for v2 unless we really need to change it.

darosior commented 1 year ago

All: anything else here that should be addressed for v2 after #630?