Skip to content

Conversation

@fanquake
Copy link
Member

@fanquake fanquake commented Feb 15, 2021

Performing a series of link checks for a Boost component that is
header-only doesn't make much sense, and currently means we just have
another confusing Boost macro in our tree. I'm not sure why this was
originally done this way; maybe Sjors or luke-jr can elaborate (#15382 (929cda5))?

The macro also has the side-effect of producing confusing error
messages. i.e in #20744, the CI is currently failing with:

checking for boostlib >= 1.58.0 (105800) lib path in "/tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/lib"... yes
checking for boostlib >= 1.58.0 (105800)... yes
checking whether the Boost::Process library is available... yes
configure: error: Could not find a version of the Boost::Process library!

This isn't useful, given there is no such thing as a Boost::Process library.

This PR just removes the macro entirely, but maintains a --with-boost-process
(defaulting to off), flag to configure. Hopefully this will also be
removed, in favour of --enable/disable-external-signer if/when #16546
is merged.

@fanquake fanquake force-pushed the boost_process_nonsense branch from 1b960f0 to ffe3d65 Compare February 15, 2021 08:28
@fanquake fanquake requested review from Sjors and luke-jr February 15, 2021 08:29
@laanwj
Copy link
Member

laanwj commented Feb 15, 2021

Concept ACK

These vague configure errors are very common and often spook users, have done so for years:

configure: error: Could not find a version of the Boost::Process library!

Good to get rid of one instance at least.

Boost Process is header only,

TIL. This is pretty good news. So the 'unique' boost parts that we need: process and multi_index, are both header only.
This means that soon we no longer have to compile boost at all, just copy headers. Assuming of course that header-only boost doesn't depend on libraries deeper in the hierarchy (this is very possible).

This PR just removes the macro entirely, but maintains a --with-boost-process(defaulting to off), flag to configure.

Right! it is better to have configure flags that semantically make sense to users not just 'use implementation specific thing'.

@sipa
Copy link
Member

sipa commented Feb 15, 2021

boost::multi_index depends on the boost serialization library if its serialization functions are used (which we don't). According to the documentation, if you don't, then multi_index is purely headers only.

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 15, 2021

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@practicalswift
Copy link
Contributor

Concept ACK

@Sjors
Copy link
Member

Sjors commented Feb 15, 2021

In #15382 I just copied what I saw for another Boost dependency :-)

Copy link
Member

@Sjors Sjors left a comment

Choose a reason for hiding this comment

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

tACK ffe3d65

I'm using this in #16546 now (will rebase that after this is merged).

@DrahtBot
Copy link
Contributor

Gitian builds

File commit 489030f
(master)
commit 852ca3fc17182225cb5060eeeee97c93f5c0973e
(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz 006c783b5ee9b7e2... fe9fc8a97dd8b6a3...
*-aarch64-linux-gnu.tar.gz f6bef6690ef6ee58... b7523eacb2deeb33...
*-arm-linux-gnueabihf-debug.tar.gz fbc6003d4f87b3f3... 20973d54f06cd1ba...
*-arm-linux-gnueabihf.tar.gz 1e0bc41a2f22e3c8... d49c6a4eee0652ba...
*-osx-unsigned.dmg 2cdfd57d35b2b300... e047362508f6074f...
*-osx64.tar.gz f39dc996a04495fd... 2a362ad99c51700c...
*-powerpc64-linux-gnu-debug.tar.gz d070af570efa110a... e412b10c366d3a6c...
*-powerpc64-linux-gnu.tar.gz 2bc7cef416bf048c... 178aaf5fd05c0346...
*-powerpc64le-linux-gnu-debug.tar.gz 776436d5885dcf6d... 3d005f86d07e2f6a...
*-powerpc64le-linux-gnu.tar.gz affb8ec092f9a7ac... f88b15a62cd678ad...
*-riscv64-linux-gnu-debug.tar.gz 5e8d5a7ead21c571... c4491755d166a5b4...
*-riscv64-linux-gnu.tar.gz 690ced04615f2f50... b554f845614515ba...
*-win64-debug.zip 3c45039ca679a2ce... df47a846952e57ed...
*-win64-setup-unsigned.exe c923d94eb6ad6625... 031985f27a853601...
*-win64.zip 115a276ef2566d76... 5ee192be900ebf46...
*-x86_64-linux-gnu-debug.tar.gz 1b3ea996252c2e3b... 7a30c803cb4806d1...
*-x86_64-linux-gnu.tar.gz 1289b942fddcd737... 182cb9d654f76e1e...
*.tar.gz 5097aaab3eff1d43... c3f11d0514f1fc35...
bitcoin-core-linux-22-res.yml 1dddf5ad4c54df22... 4047756e6aaf810e...
bitcoin-core-osx-22-res.yml e3519662695f59ec... 21a95049dcb3d4ef...
bitcoin-core-win-22-res.yml 4841d2d23b8e1a96... bd0f6bb110823ef0...
linux-build.log 6fa01f16dc2596f6... 0815a978af442895...
osx-build.log c346ccb31425f8cd... 862fa59fc9961dfa...
win-build.log 651a38db57f23a64... 4140bb5fde76348b...
bitcoin-core-linux-22-res.yml.diff b1fb2411aa6dda21...
bitcoin-core-osx-22-res.yml.diff dd6ce9bd434ebd95...
bitcoin-core-win-22-res.yml.diff 7f6d85c65519543f...
linux-build.log.diff 4ee6c877077e1a0e...
osx-build.log.diff 6a502bc8572af803...
win-build.log.diff cd106f020b1fa412...

@practicalswift
Copy link
Contributor

cr ACK ffe3d65aaa18df9413c35e6914b42f7a400bf0b6: patch looks correct

Performing a series of link checks for a Boost component that is
header-only doesn't make much sense, and currently means we just have
another confusing Boost macro in our tree. I'm not sure why this was
originally done this way; maybe Sjors or luke-jr can elaborate
(bitcoin#15382 (929cda5))?

The macro also has the side-effect of producing confusing error
messages. i.e in bitcoin#20744, the CI is currently failing with:
```bash
checking for boostlib >= 1.58.0 (105800) lib path in "/tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/lib"... yes
checking for boostlib >= 1.58.0 (105800)... yes
checking whether the Boost::Process library is available... yes
configure: error: Could not find a version of the Boost::Process library!
```

This isn't useful, given there is no such thing as a `Boost::Process`
library.

This PR just removes the macro entirely, but maintains a `--with-boost-process`
(defaulting to off), flag to configure. Hopefully this will also be
removed, in favour of `--enable-disable-external-signer` if/when bitcoin#16546
is merged.
@fanquake fanquake force-pushed the boost_process_nonsense branch from ffe3d65 to 7bf04e3 Compare February 17, 2021 00:04
@laanwj
Copy link
Member

laanwj commented Feb 17, 2021

ACK 7bf04e3

@laanwj laanwj merged commit 0433608 into bitcoin:master Feb 17, 2021
@fanquake fanquake deleted the boost_process_nonsense branch February 17, 2021 14:10
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 17, 2021
7bf04e3 build: remove mostly pointless BOOST_PROCESS macro (fanquake)

Pull request description:

  Performing a series of link checks for a Boost component that is
  header-only doesn't make much sense, and currently means we just have
  another confusing Boost macro in our tree. I'm not sure why this was
  originally done this way; maybe Sjors or luke-jr can elaborate (bitcoin#15382 (929cda5))?

  The macro also has the side-effect of producing confusing error
  messages. i.e in bitcoin#20744, the CI is currently failing with:
  ```bash
  checking for boostlib >= 1.58.0 (105800) lib path in "/tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/lib"... yes
  checking for boostlib >= 1.58.0 (105800)... yes
  checking whether the Boost::Process library is available... yes
  configure: error: Could not find a version of the Boost::Process library!
  ```

  This isn't useful, given there is no such thing as a `Boost::Process` library.

  This PR just removes the macro entirely, but maintains a `--with-boost-process`
  (defaulting to off), flag to configure. Hopefully this will also be
  removed, in favour of `--enable/disable-external-signer` if/when bitcoin#16546
  is merged.

ACKs for top commit:
  laanwj:
    ACK 7bf04e3

Tree-SHA512: b270a0250f32df2078f986c165b8977967d8c06df80bf2773f3442f74b395a3bfa6544af1024d9b6524d90d47a0f6304194b3aced0e2ecb88e75916da945ccb6
@DrahtBot
Copy link
Contributor

Guix builds

File commit 92fee79
(master)
commit d3c2cced4353898bb361211f97dbff222285caf4
(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz 5a111c6410cbe1b7... 7100acb8809cd131...
*-aarch64-linux-gnu.tar.gz c34ee7b51294fa34... dd76a640115a61e3...
*-arm-linux-gnueabihf-debug.tar.gz 319722a3e897b44d... 2e9669d5e76d89ad...
*-arm-linux-gnueabihf.tar.gz 918dd4eaecbf674e... 560e5f196908e989...
*-osx-unsigned.dmg 3ea1713dd2c2e93d...
*-osx-unsigned.tar.gz c7ef47d2605fc49f...
*-osx64.tar.gz 7d01ab16be9c6551...
*-riscv64-linux-gnu-debug.tar.gz 2f73512e5994d117... e79c7e181aec1645...
*-riscv64-linux-gnu.tar.gz 309229dbee5ca9ff... 6e77c4edd59743c3...
*-win-unsigned.tar.gz ca693e0ccf46f6c0...
*-win64-debug.zip 613ce4f1a39725a8...
*-win64-setup-unsigned.exe 232fd5a4ecc88e13... 8484b9eecb53eb20...
*-win64.zip dc3928950e7dbd51...
*-x86_64-linux-gnu-debug.tar.gz 82122cab5cffa47d... 392a3c646cd41820...
*-x86_64-linux-gnu.tar.gz a5f5834e0adf7c7b... dc613f2e97d0e5f4...
*.tar.gz eb4d9b2072405139... d471b8500a828cee...
guix_build.log 10224f79e4d826e6... 3b5df31e1c7f0832...
guix_build.log.diff 6bc7919cba5576fc...

@laanwj
Copy link
Member

laanwj commented Feb 18, 2021

Looks like the GUIX build failed?
Oh, it's unrelated:

/gnu/store/4yisib9qc6l5a8kbb6pmkmaj0zx7fj67-profile/bin/x86_64-w64-mingw32-objcopy:bitcoin-d3c2cced4353/bin/test_bitcoin.exe.dbg[.debug_loc]: No space left on device

@maflcko
Copy link
Member

maflcko commented Feb 18, 2021

Will increase the disk size soon(tm)(c)(2021)

laanwj added a commit that referenced this pull request Feb 23, 2021
f75e0c1 doc: add external-signer.md (Sjors Provoost)
d4b0107 rpc: send: support external signer (Sjors Provoost)
245b445 rpc: signerdisplayaddress (Sjors Provoost)
7ebc7c0 wallet: ExternalSigner: add GetDescriptors method (Sjors Provoost)
fc5da52 wallet: add GetExternalSigner() (Sjors Provoost)
259f52c test: external_signer wallet flag is immutable (Sjors Provoost)
2655197 rpc: add external_signer option to createwallet (Sjors Provoost)
2700f09 rpc: signer: add enumeratesigners to list external signers (Sjors Provoost)
07b7c94 rpc: add external signer RPC files (Sjors Provoost)
8ce7767 wallet: add ExternalSignerScriptPubKeyMan (Sjors Provoost)
157ea7c wallet: add external_signer flag (Sjors Provoost)
f3e6ce7 test: add external signer test (Sjors Provoost)
8cf543f wallet: add -signer argument for external signer command (Sjors Provoost)
f7eb7ec test: framework: add skip_if_no_external_signer (Sjors Provoost)
87a9794 configure: add --enable-external-signer (Sjors Provoost)

Pull request description:

  Big picture overview in [this gist](https://gist.github.com/Sjors/29d06728c685e6182828c1ce9b74483d).

  This PR lets `bitcoind` call an arbitrary command `-signer=<cmd>`, e.g. a hardware wallet driver,  where it can fetch public keys, ask to display an address, and sign a transaction (using PSBT under the hood).

  It's design to work with https://github.com/bitcoin-core/HWI, which supports multiple hardware wallets. Any command with the same arguments and return values will work. It simplifies the manual procedure described [here](https://github.com/bitcoin-core/HWI/blob/master/docs/bitcoin-core-usage.md).

  Usage is documented in [doc/external-signer.md](
  https://github.com/Sjors/bitcoin/blob/2019/08/hww-box2/doc/external-signer.md), which also describes what protocol a different signer binary should conform to.

  Use `--enable-external-signer` to opt in, requires Boost::Process:

  ```
  Options used to compile and link:
    with wallet     = yes
    with gui / qt   = no
    external signer = yes
  ```

  It adds the following RPC methods:
  * `enumeratesigners`: asks <cmd> for a list of signers (e.g. devices) and their master key fingerprint
  * `signerdisplayaddress <address>`:  asks <cmd> to display an address

  It enhances the following RPC methods:
  * `createwallet`: takes an additional `external_signer` argument and fetches keys from device
  * `send`: automatically sends transaction to device and waits

  Usage TL&DR:
  * clone HWI repo somewhere and launch `bitcoind -signer=../HWI/hwi.py`
  * check if you can see your hardware device: `bitcoin-cli enumeratesigners`
  * create wallet and auto import keys `bitcoin-cli createwallet "hww" true true "" true true true`
  * display address on device: `bitcoin-cli signerdisplayaddress ...`
  * to spend, use `send` RPC and approve transaction on device

  Prerequisites:
  - [x] #21127 load wallet flags before everything else
  - [x] #21182 remove mostly pointless BOOST_PROCESS macro

  Potentially useful followups:
  - GUI support: bitcoin-core/gui#4
  - bumpfee support
  - (automatically) verify (a subset of) keys on the device after import, through message signing

ACKs for top commit:
  laanwj:
    re-ACK f75e0c1

Tree-SHA512: 7db8afd54762295c1424c3f01d8c587ec256a72f34bd5256e04b21832dabd5dc212be8ab975ae3b67de75259fd569a561491945750492f417111dc7b6641e77f
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants