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

Change the nix-bitcoin deployment from forking this repo to importing the module #136

Merged
merged 16 commits into from
Apr 8, 2020

Conversation

jonasnick
Copy link
Member

WIP: because requires doc changes and ideally tests.

@erikarvstedt
Copy link
Collaborator

erikarvstedt commented Feb 25, 2020

Great!
I've added a few relevant commits.

  • extract module 'deployment/nixops.nix'...
    Rationale: The nixops secrets setup belongs to a library module and
    not to the user config.

  • rename 'network' to more precise names
    I've always found the name 'network' confusing because we're just setting up a
    node, not a network.

  • fix formatting
    This, among other things, removes whitespace introduced in this PR.
    Could you setup a whitespace filter in your editor or your git commit hooks?

  • don't import nixpkgs twice
    nixpkgs-pinned doesn't depend on nixpkgs so we should directly import it to
    avoid the nixpkgs import overhead.
    Also, for this reason I think nixpkgs-pinned shouldn't be added to pkgs/default.nix.

  • disable hardened kernel profile in example config
    The performance hit, measured by node boot time and integration test duration, is really significant (~50%). So I don't think this should be the default.

  • add deploy-nixops.sh
    I think it's super helpful to have self-contained example scripts that run
    unattended and without leaving traces on the user's system.
    Also, these scripts can be used in CI testing.
    I've added such a script for nixops vbox deployments.
    The NIXOPS_SSH_OPTS patch is needed for setting SSH opts to avoid interactive queries
    and polluting the user's known_hosts file.
    I'll propose this patch upstream.

  • add deploy-container.sh
    Demo script for container deployment.
    Containers are the fastest and simplest way of tinkering with a node setup.
    extra-container, my container helper that's used by this script, is in the process
    of getting support
    for all systemd-based distros, so this example will be widely compatible.

Here are other relevant changes that I think are sensible:

  • Rename example dir to examples
  • Rename nix-bitcoin.nix to tor-node.nix or basic-node.nix and move it into a
    profiles subdirectory (like <nixos/modules/profiles>).
    We can add a symlink to nix-bitcoin.nix for backwards compat.
    Rationale: nix-bitcoin.nix is an (opinionated) node config, so its name should
    reflect that.
  • Maybe remove <nix-bitcoin/shell.nix> as I don't see how this is needed anymore.
    Or at least remove nixops and the SSH_AUTH_SOCK hack from the file.

@jonasnick
Copy link
Member Author

Looks good!

rename 'network' to more precise names
I've always found the name 'network' confusing because we're just setting up a
node, not a network.

Agreed

fix formatting
This, among other things, removes whitespace introduced in this PR.
Could you setup a whitespace filter in your editor or your git commit hooks?

Thanks, must've turned it off somehow

don't import nixpkgs twice
nixpkgs-pinned doesn't depend on nixpkgs so we should directly import it to
avoid the nixpkgs import overhead.
Also, for this reason I think nixpkgs-pinned shouldn't be added to pkgs/default.nix.

So I guess this would also work with fetchTarball. Agreed.

disable hardened kernel profile in example config
The performance hit, measured by node boot time and integration test duration, is really significant (~50%). So I don't think this should be the default.

I think this should remains the default. We can leave the 50% comment and add a FIXME.

add deploy-nixops.sh
add deploy-container.sh

Nice idea with the self-contained example scripts. All that effort to set up the
node and then only do systemctl status bitcoind? Perhaps with a pruned
bitcoind we can just keep the shell open.

Nixops does seem to leave some traces. My VirtualBox GUI still lists the previous deployments.

of getting support for all systemd-based distros, so this example will be widely compatible.

Ooh, this would be awesome.

Rename example dir to examples
Yeah

Rename nix-bitcoin.nix to tor-node.nix or basic-node.nix and move it into a
profiles subdirectory (like <nixos/modules/profiles>).
We can add a symlink to nix-bitcoin.nix for backwards compat.
Rationale: nix-bitcoin.nix is an (opinionated) node config, so its name should
reflect that.

Good idea. In a different PR?

Maybe remove <nix-bitcoin/shell.nix> as I don't see how this is needed anymore.
Or at least remove nixops and the SSH_AUTH_SOCK hack from the file.

Perhaps the shell would be still useful for development.

@jonasnick
Copy link
Member Author

Perhaps with a pruned bitcoind we can just keep the shell open.

Actually not a good idea because that may lead users to losing coins by opening lightning channels which will be deleted with the file system on exit of the script.

@erikarvstedt
Copy link
Collaborator

erikarvstedt commented Feb 26, 2020

I've re-enabled the hardened profile: erikarvstedt@422ba68

All that effort to set up the node and then only do...

These scripts have no practical purpose, they're just a launchpad for experimenting.
And you can run them unattended to prove that everything works. I've added some
comments to clarify this: erikarvstedt@e1092f0

Nixops does seem to leave some traces

Could confirm this after running the script a few times, happens only sporadically.
Fixed here: erikarvstedt@815208e

Good idea. In a different PR?

Yes.

Perhaps the shell would be still useful for development

OK, but can I delete the nixops input and ssh hack?

I've added branch nix-bitcoin-as-module-ea-squashed which is a squashed version of nix-bitcoin-as-module-ea. If you want you can set this PR to the squashed branch, while re-signing your commits and removing me as a commiter.

@jonasnick jonasnick force-pushed the nix-bitcoin-as-module branch 2 times, most recently from 5550608 to b2cf942 Compare March 8, 2020 15:43
@jonasnick
Copy link
Member Author

Cherry-picked @erikarvstedt's commits, renamed example/ dir to examples/, cleaned up development shell.nix and tested a nixops deployment using the example files. Seems like we're only missing documentation and then this PR should be good to go.

@jonasnick
Copy link
Member Author

I noticed that hardcoding a url/sah256 for fetchTarball in examples/shell.nix doesn't work because obviously it can not refer to its own hash. Therefore I added a package that allows to get the hash of the latest release hash from github and verifies its signature. Compared to before this simplifies updates quite a bit. We have to do releases often now but at least this is a good place to put migration etc notes.

The resulting examples/shell.nix is a bit ugly and installation not exactly straightforward (need to enter two different nix-shells and copy things around) but we can improve on that. Overall I like this approach. Let me know what you think.

@jonasnick jonasnick force-pushed the nix-bitcoin-as-module branch 2 times, most recently from daefffa to 9e75f4f Compare March 24, 2020 21:42
jonasnick and others added 5 commits March 24, 2020 21:43
… the module

Instead of forking this repo, it is now recommended that users simply import the
nix-bitcoin module. This commit adds an example directory that contains the
network/ examples and a shell.nix for deployment with nixops.
@jonasnick
Copy link
Member Author

Note to self: need to add nix-bitcoin-release to travis.

@erikarvstedt
Copy link
Collaborator

Looks great! Will do a proper review tomorrow.

@erikarvstedt
Copy link
Collaborator

Maybe a QEMU VM example which would only require nix as the sole dependency could be helpful. What do you think?

@jonasnick
Copy link
Member Author

If that means we can remove the reliance on VirtualBox for the tutorials that would be super helpful!

@erikarvstedt
Copy link
Collaborator

Sorry, this took longer than expected.

Here are the fixes I'd propose.

I think we should revert docs: Distinguish between terminal input and output

  • It makes it harder to copy and paste the shell commands into a terminal (or execute them from within your editor.)
  • It doesn't resolve any ambiguities, because the shell snippets contain no output at all, only input.
    Output could be formatted like this instead:
    mycmd
    # => output
    

@erikarvstedt
Copy link
Collaborator

Note: Commit examples/shell.nix: don't run shellHook on subsequent nix-shells is needed for commit fixup! move nix-bitcoin-release pkg to self-contained script.

@jonasnick jonasnick force-pushed the nix-bitcoin-as-module branch 2 times, most recently from 457b0b9 to af7ca3d Compare March 29, 2020 20:15
@jonasnick
Copy link
Member Author

Thank you very much for your suggestions! I updated the branch and replaced the release.

I think we should revert docs: Distinguish between terminal input and output

Good point about copy and pasting. There are in fact some lines with output (the hash of the nixos release for example). I changed it to your suggestion.

add deploy-qemu-vm.sh example

Cool! I added a fixup to chmod the SSH secret key, otherwise it wouldn't work for me.

fixup! nix-bitcoin-release.sh: simplify gpg error handling

Haven't picked that one yet because I intended to output the GPG error in case of failure (but then apparently forgot).

[temporary] use jonasnick/nix-bitcoin for testing nix-bitcoin-release.sh

I didn't pick this, and instead just fixed the release I already made in this repo. It didn't work anymore because I had set the "pre-release" flag.

fixup! shell.nix: simplify unpacking nix-bitcoin-src

Ah, nice, didn't know about runCommand.

fixup! move nix-bitcoin-release pkg to self-contained script

As per the installation instructions you copy the examples/shell.nix file into your own directory and if you want to update you run the release command. This wouldn't work if we hard code a path to the script. I didn't pick this commit as I don't see a way around having this as a package, but perhaps you have thoughts on how to improve this?

fixup! rename key-jonasnick.gpg -> key-jonasnick.asc

My understanding is in the gpg world asc is ASCII armored and gpg is binary. I prefer binary in this case because it's smaller.

@erikarvstedt
Copy link
Collaborator

fixup to chmod the SSH secret key, otherwise it wouldn't work for me

Thanks for catching this, missed this in my local testing due to not re-checking out the worktree from the repo (git only stores the executable bit).

asc is ASCII armored and gpg is binary

Sorry, I missed you were using a binary format. What about key-jonasnick.bin or just key-jonasnick? .gpg is really only for encrypted stuff.

but perhaps you have thoughts on how to improve this?

Fixed in erikarvstedt@1663185 in my new fixup branch.

@jonasnick
Copy link
Member Author

jonasnick commented Mar 30, 2020

Fixed in erikarvstedt/nix-bitcoin@1663185 in my new fixup branch.

That was easier than expected. Pushed an updated release.

erikarvstedt and others added 6 commits March 30, 2020 10:56
This avoids an extra delay and the unexpected creation of secrets when
run in another dir.

Needed for the 'fetch-release' script introduced in a later commit.
Prepares, signs and pushes a release to github.
@erikarvstedt
Copy link
Collaborator

ACK!
I've added minor fixups here.
A squashed version of this branch is here.

This allows getting the hash of the latest (or some other) release
using github releases and gpg verification.
Now you clone nix-bitcoin and start out from the examples.
@jonasnick jonasnick changed the title WIP: Change the nix-bitcoin deployment from forking this repo to importing the module Change the nix-bitcoin deployment from forking this repo to importing the module Apr 8, 2020
@jonasnick jonasnick merged commit 9239268 into master Apr 8, 2020
@jonasnick
Copy link
Member Author

Also pushed a release with notes here: https://github.com/fort-nix/nix-bitcoin/releases/tag/v0.0.1

@jonasnick jonasnick deleted the nix-bitcoin-as-module branch April 8, 2020 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants