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.

Sign up for GitHub

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

Jump to bottom

bitcoin: 0.20.1 -> 0.21.0 + allow build without wallet #106837

Merged
merged 2 commits into from Jan 17, 2021
Merged

bitcoin: 0.20.1 -> 0.21.0 + allow build without wallet #106837

merged 2 commits into from Jan 17, 2021

Conversation

prusnak
Copy link
Member

@prusnak prusnak commented Dec 13, 2020 •
edited

Motivation for this change
  • version update
  • allow to build bitcoin{,d} without wallet via --disable-wallet configure option
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@prusnak prusnak changed the title bitcoin: introduce withWallet flag, allow build without wallet bitcoin: 0.20.1 -> 0.21.0 + allow build without wallet Jan 16, 2021
roconnor
roconnor reviewed Jan 16, 2021
View reviewed changes
pkgs/applications/blockchains/bitcoin.nix Outdated
@@ -58,6 +60,8 @@ stdenv.mkDerivation rec {
] ++ optionals (!doCheck) [
"--disable-tests"
"--disable-gui-tests"
] ++ optionals (!withWallet) [
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks improperly indented.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indent fixed in 672607c

roconnor
roconnor reviewed Jan 16, 2021
View reviewed changes
pkgs/applications/blockchains/bitcoin.nix
@@ -36,15 +37,16 @@ stdenv.mkDerivation rec {
"https://bitcoincore.org/bin/bitcoin-core-${version}/bitcoin-${version}.tar.gz"
"https://bitcoin.org/bin/bitcoin-core-${version}/bitcoin-${version}.tar.gz"
];
sha256 = "4bbd62fd6acfa5e9864ebf37a24a04bc2dcfe3e3222f056056288d854c53b978";
sha256 = "1a91202c62ee49fb64d57a52b8d6d01cd392fffcbef257b573800f9289655f37";
Copy link
Contributor

Choose a reason for hiding this comment

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

This hash matches the value signed by Bitcoin Core binary release signing key at https://bitcoincore.org/bin/bitcoin-core-0.21.0/SHA256SUMS.asc and I have verified the signature in that file.

@ofborg ofborg bot requested a review from roconnor January 16, 2021 17:24
@roconnor
Copy link
Contributor

We need to add an sqlite dependency (when wallets are enabled) for descriptor wallet support.

@prusnak
Copy link
Member Author

prusnak commented Jan 16, 2021

We need to add an sqlite dependency (when wallets are enabled) for descriptor wallet support.

Added in 727d4a5d5713408adece68915269fdaf2c901e03

@roconnor
Copy link
Contributor

ping @jb55 in case he wants to also do a review.

@roconnor
Copy link
Contributor

I'm not sure if we should add ? null to the db48 and sqlite parameters. In practice it probably doesn't matter whether we do or not.

@erikarvstedt
Copy link
Member

@prusnak, can you squash bitcoin: add sqlite to buildInputs (if wallet is enabled) into bitcoin: 0.20.1 -> 0.21.0?
This makes it clear that this is a new requirement for 0.21.0.

@prusnak
bitcoin: 0.20.1 -> 0.21.0
f324c9a 
+ add sqlite to buildInputs (if wallet is enabled)
@prusnak
Copy link
Member Author

prusnak commented Jan 16, 2021

I'm not sure if we should add ? null to the db48 and sqlite parameters. In practice it probably doesn't matter whether we do or not.

I don't see that as necessary.

@prusnak
Copy link
Member Author

prusnak commented Jan 16, 2021

can you squash bitcoin: add sqlite to buildInputs (if wallet is enabled) into bitcoin: 0.20.1 -> 0.21.0?

Done in f324c9a

erikarvstedt
erikarvstedt approved these changes Jan 16, 2021
View reviewed changes
Copy link
Member

@erikarvstedt erikarvstedt left a comment •
edited

Choose a reason for hiding this comment

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

Tests bitcoind, blockbook-frontend succeed.
All other relevant tests (prometheus-exporters, wasabibackend) were already broken before.

roconnor
roconnor approved these changes Jan 17, 2021
View reviewed changes
Copy link
Contributor

@roconnor roconnor left a comment

Choose a reason for hiding this comment

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

LGTM.

Someday we should look into perhaps not-wrapping non-qt binaries.

@prusnak prusnak merged commit 50cbe8b into NixOS:master Jan 17, 2021
@prusnak prusnak deleted the bitcoin-no-wallet branch January 17, 2021 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roconnor roconnor roconnor approved these changes

@erikarvstedt erikarvstedt erikarvstedt approved these changes

@mmahut mmahut Awaiting requested review from mmahut mmahut is a code owner

@RaghavSood RaghavSood Awaiting requested review from RaghavSood RaghavSood is a code owner

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@prusnak @roconnor @erikarvstedt