-
Notifications
You must be signed in to change notification settings - Fork 107
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
add liana
support
#708
base: master
Are you sure you want to change the base?
add liana
support
#708
Conversation
default = "wsh(or_d(pk([0dd8c6f0/48'/1'/0'/2']tpubDFMbZ7U5k5hEfsttnZTKMmwrGMHnqUGxhShsvBjHimXBpmAp5KmxpyGsLx2toCaQgYq5TipBLhTUtA2pRSB9b14m5KwSohTDoCHkk1EnqtZ/<0;1>/*),and_v(v:pkh([d4ab66f1/48'/1'/0'/2']tpubDEXYN145WM4rVKtcWpySBYiVQ229pmrnyAGJT14BBh2QJr7ABJswchDicZfFaauLyXhDad1nCoCZQEwAW87JPotP93ykC9WJvoASnBjYBxW/<0;1>/*),older(65535))))#7nvn6ssc"; | ||
description = mdDoc "The wallet descriptor."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it better to move these hardcoded defaults to example
instead of default
.
I can see easily an user shooting himself in the foot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally this should come from options.services.lianad.*
, and it definitely makes sense to remove this default value, but with the tradeoff that options.services.lianad.main_descriptor
must become a mandatory option.
unfortunately lianad
's CLI is still very limited, so everything needs to be written indo a config.toml
, and lianad
will not start unless there is a valid descriptor inside config.toml
so the challenge here is:
- declare all options from this sample
lianad_config_example.toml
asoptions.services.lianad.*
insidemodules/liana.nix
so they're available assystemd
configurations (which seems to be the most common pattern acrossnix-bitcoin
modules) - make sure these configurations are taken from
options.services.lianad.*
and used to generate aconfig.toml
on the module's output (ideally placed somewhere like/var/lib/lianad
)
I started doing a heredoc inside the preStart
but still unsure whether that is the best approach. It feels a bit dirty tbh.
Also, while the variables are still hardcoded on the heredoc, the idea is to take their actual values from options.services.lianad.*
. There heredoc includes some comments. But again: heredoc might not be the best approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but still in a footgun for the user.
Check my comment below on how to interpolate TOML file contents into a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really, this is a proposal to mitigate the footgun
- the user MUST provide a
options.services.lianad.main_descriptor
on theirconfiguration.nix
, which in turn is used to dynamically generate aconfig.toml
file on disk of the output's datadir - failing to do so blocks the module from ever launching a
systemd
service, because there's no validconfig.toml
in disk
the heredoc is still hardcoding things, but hopefully something along these lines will be a viable approach:
cat << EOF > lianad_config.toml
# these should come from options.services.lianad
- daemon = false
- data_dir = "/var/lib/lianad"
- log_level = "debug"
- main_descriptor = "wsh(or_d(pk([0dd8c6f0/48'/1'/0'/2']tpubDFMbZ7U5k5hEfsttnZTKMmwrGMHnqUGxhShsvBjHimXBpmAp5KmxpyGsLx2toCaQgYq5TipBLhTUtA2pRSB9b14m5KwSohTDoCHkk1EnqtZ/<0;1>/*),and_v(v:pkh([d4ab66f1/48'/1'/0'/2']tpubDEXYN145WM4rVKtcWpySBYiVQ229pmrnyAGJT14BBh2QJr7ABJswchDicZfFaauLyXhDad1nCoCZQEwAW87JPotP93ykC9WJvoASnBjYBxW/<0;1>/*),older(65535))))#7nvn6ssc"
+ daemon = ${options.services.lianad.daemon}
+ data_dir = "${options.services.lianad.data_dir}"
+ log_level = "${options.services.lianad.log_level}"
+ main_descriptor = "${options.services.lianad.main_descriptor}"
I'll give it a try at some time later.
b77d51f
to
a7ebed2
Compare
preStart = '' | ||
cat << EOF > lianad_config.toml | ||
# these should come from options.services.lianad | ||
daemon = false | ||
data_dir = "/var/lib/lianad" | ||
log_level = "debug" | ||
main_descriptor = "wsh(or_d(pk([0dd8c6f0/48'/1'/0'/2']tpubDFMbZ7U5k5hEfsttnZTKMmwrGMHnqUGxhShsvBjHimXBpmAp5KmxpyGsLx2toCaQgYq5TipBLhTUtA2pRSB9b14m5KwSohTDoCHkk1EnqtZ/<0;1>/*),and_v(v:pkh([d4ab66f1/48'/1'/0'/2']tpubDEXYN145WM4rVKtcWpySBYiVQ229pmrnyAGJT14BBh2QJr7ABJswchDicZfFaauLyXhDad1nCoCZQEwAW87JPotP93ykC9WJvoASnBjYBxW/<0;1>/*),older(65535))))#7nvn6ssc" | ||
|
||
# these should come from options.services.lianad | ||
[bitcoin_config] | ||
network = "signet" | ||
poll_interval_secs = 30 | ||
|
||
# these should come from options.services.bitcoind | ||
[bitcoind_config] | ||
addr = "127.0.0.1:38332" | ||
auth = "username:password" | ||
|
||
EOF | ||
''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a builtins.fromTOML
follwed by a toString
, e.g.:
let
tomlFile =./example.toml;
tomlData = builtins.readFile tomlFile;
tomlObj = builtins.fromTOML tomlData;
tomlString = toString tomlObj;
in
tomlString
Then you can interpolate the hardcoded nastiness with options.services.lianad.*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this allow me to dynamically generate the TOML file and write it into the output's datadir?
it seems like a convenient tool for reading configs that already exist on disk, but our use case is the opposite direction:
we need to generate a TOML file with values coming from options.services.lianad.*
, because lianad
only launches with a valid config.toml
coming from disk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can generate a static toml file from a set using pkgs.formats.toml
. E.g:
mySet = { foo = "bar""; };
myTomlFile = (pkgs.formats.toml {}).generate mySet;
But this of course cannot contain secrets such as bitcoind credentials that are not going into the nix store. So then the preStart
rule could execute a command that reads the generated TOML, and appends the additional secrets, writing it to the dataDir for liana.
The lnd module actually already does this:
preStart = ''
install -m600 ${configFile} '${cfg.dataDir}/lnd.conf'
{
echo "bitcoind.rpcpass=$(cat ${secretsDir}/bitcoin-rpcpassword-${rpcUser})"
${optionalString (cfg.getPublicAddressCmd != "") ''
echo "externalip=$(${cfg.getPublicAddressCmd})"
''}
} >> '${cfg.dataDir}/lnd.conf'
default = "wsh(or_d(pk([0dd8c6f0/48'/1'/0'/2']tpubDFMbZ7U5k5hEfsttnZTKMmwrGMHnqUGxhShsvBjHimXBpmAp5KmxpyGsLx2toCaQgYq5TipBLhTUtA2pRSB9b14m5KwSohTDoCHkk1EnqtZ/<0;1>/*),and_v(v:pkh([d4ab66f1/48'/1'/0'/2']tpubDEXYN145WM4rVKtcWpySBYiVQ229pmrnyAGJT14BBh2QJr7ABJswchDicZfFaauLyXhDad1nCoCZQEwAW87JPotP93ykC9WJvoASnBjYBxW/<0;1>/*),older(65535))))#7nvn6ssc"; | ||
description = mdDoc "The wallet descriptor."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but still in a footgun for the user.
Check my comment below on how to interpolate TOML file contents into a string.
Would upstreaming this into nixpkgs make sense? There are a bunch of Bitcoin related packages and services in nixpkgs already. |
I already maintain the |
that's awesome @dunxen thanks for sharing! I actually went for different tradeoffs on the
are you open to PRs expanding |
Sure, thanks! It would be one package definition but with attributes to choose what to build. So I'd imagine that if you install Kind of how bitcoin does it in nixpkgs: So we'd need to define |
Concept ACK on adding a liana module to nix-bitcoin. Thanks! We should also mention the new service in README.md and in examples/configuration.nix and add a test checking that it runs. |
thanks @dunxen
I can't find |
Packages in Apologies to all for the slightly off topic 🙏 |
Liana is a simple Bitcoin wallet with built-in loss protection and inheritance.
Current status: draft.
Roadmap:
pkgs/liana/default.nix
buildslianad
+liana-cli
modules/liana.nix
+ module integration tests (example)cc @darosior