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

Delegate Bitcoin Core's private key management to Eclair #2613

Merged
merged 8 commits into from
Sep 21, 2023

Conversation

sstone
Copy link
Member

@sstone sstone commented Mar 16, 2023

We want to use Eclair to manage private keys for Bitcoin Core to protect our onchain funds.
We would create empty Bitcoin Core wallets and then import descriptors generated by Eclair (this is similar to how hardware wallets are used with Bitcoin Core or Electrum).

To validate transactions without a trusted display and without human interaction, we define a very simple policy:

  • we explicitly track all inputs and outputs that belong to our onchain wallet
  • we only sign inputs that are tagged as belonging to us
  • we verify that we can spend all outputs that are tagged as belonging to us

This works when onchain transactions are created by Eclair (funding, dual-funding, RBF, but also with the trusted swaps that we are currently using).

Bitcoin Core would treat Eclair’s onchain wallet as a watch-only wallet and will not be able to sign transactions: “sendtoaddress” and “walletprocesspsbt” RPC call will fail. To send funds to a specific onchain address, users will need to call Eclair’s “sendonchain” API which requires human validation with a trusted Ledger device.

It would also be possible to initialise a Ledger device with Eclair’s onchain seed and keep it as a backup but using it would interfere with Eclair’s operations (both wallets may try and use the same UTXOs).

Wallet descriptors

Descriptors describe how to generate public keys. Bitcoin Core will use these descriptors to create a wallet, track public keys and utxos, fund transactions and provide information about its wallet inputs and outputs (like amounts being spent and BIP32 paths of the wallet public keys) which Eclair will use internally to validate transactions before signing them.

This is a descriptor for a BIP84 wallet:

wpkh([92aeacc2/84'/1'/0']tpubDCyGWF1tZPqSDq7HqKtM31ZDDFoYybL56RDrfKHFQXTmx1rf5Uh6T5aFTE7wZmxaCQNCzfnu6wMv53k8kZNEtmSpgiCAuvBWmv7zjBxZcAF/0/*)#hdngjcua

It includes an “xpub”, a derivation template (/0/*), and the type of script to generate (wpkh)

Implementation

What we need to do in Eclair is:

  • implement a new key manager to manage onchain keys (we’ll follow the BIP84 specs)
  • add an API call to return wallet descriptors
  • modify our Bitcoin Core client to use our keymanager to sign transactions

Signature Workflow

Whenever we need to fund and sign a transaction we:

  • create an non-funded transaction
  • ask bitcoin core to add inputs and outputs, with a specified fee rate
  • mark all inputs and outputs that were not in the non-funded transaction as belonging to us
  • create a PSBT from the funded transaction
  • ask bitcoin core to add metadata (spent amounts, BIP derivations paths) to the PSBT
  • check that the fees for this PSBT match the target feerate
  • for all our inputs, check that we can recompute their pubkey script using the provided BIP32 paths and sign them
  • for all our outputs, check that we can recompute their pubkey script using the provided BIP32 paths

This workflow is safe because:

  • we only sign inputs that we explicitly tag as belonging to us. Even if a hacker manager to control our Bitcoin Node and used our UTXOs to negotiate a dual-funding channel with us, we would not sign these inputs (because during the interactive tx negotiation Eclair will explicitly tag them as “theirs/remote”)
  • we verify that we can actually spend our wallet outputs: Bitcoin Core cannot lie to us about change outputs
  • we are protected against overpaying fees because Bitcoin Core cannot lie about the amounts of the wallet inputs that are spend, because they are included in the hash that is signed (see https://github.com/bitcoin/bips/blob/master/bip-0143.mediawiki#specification)

We check that the full transaction that is spent is included in the PSBT’s non-witness utxo field.

Master Seed Management and Compatibility with other BIP32 wallets

We define a new eclair-signer.conf file that includes BIP32 mnemonic words and passphrase. It should be compatible with any BIP32-compliant wallet.
Descriptors generated by Eclair can also be used to create a watch-only wallet on another Bitcoin Node (we could also simply copy Eclair’s wallet since it does not contain private keys).

And a new “getmasterxpub” API call has been added to eclair, which will return an “xpub” that can be used to create an Electrum watch-only wallet.

@sstone sstone requested review from t-bast, pm47 and remyers March 16, 2023 13:35
@sstone sstone changed the title Implement Bitcoinc Core's Hardware Wallet Interface Implement Bitcoin Core's Hardware Wallet Interface Mar 16, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2023

Codecov Report

Merging #2613 (d3ac588) into master (148fc67) will decrease coverage by 0.15%.
The diff coverage is 80.73%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master    #2613      +/-   ##
==========================================
- Coverage   85.89%   85.74%   -0.15%     
==========================================
  Files         215      216       +1     
  Lines       17854    18013     +159     
  Branches      756      759       +3     
==========================================
+ Hits        15335    15446     +111     
- Misses       2519     2567      +48     
Files Changed Coverage Δ
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala 55.69% <0.00%> (-2.98%) ⬇️
...chain/bitcoind/rpc/BasicBitcoinJsonRPCClient.scala 100.00% <ø> (ø)
...in/bitcoind/rpc/BatchingBitcoinJsonRPCClient.scala 75.00% <0.00%> (-25.00%) ⬇️
...ir/blockchain/bitcoind/rpc/BitcoinCoreClient.scala 88.23% <74.28%> (-10.23%) ⬇️
...q/eclair/channel/publish/ReplaceableTxFunder.scala 85.49% <84.00%> (-3.96%) ⬇️
...air/crypto/keymanager/LocalOnChainKeyManager.scala 87.64% <87.64%> (ø)
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 93.29% <100.00%> (ø)
...ir-core/src/main/scala/fr/acinq/eclair/Setup.scala 75.14% <100.00%> (+0.29%) ⬆️
...ala/fr/acinq/eclair/blockchain/OnChainWallet.scala 100.00% <100.00%> (ø)
...inq/eclair/channel/fund/InteractiveTxBuilder.scala 91.37% <100.00%> (+0.31%) ⬆️

... and 7 files with indirect coverage changes

Copy link
Contributor

@remyers remyers left a comment

Choose a reason for hiding this comment

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

So far I have only found minor issues and noted some questions. I'll look at the tests next.

It will help with review to add something to the release notes and perhaps a new guide to docs\ about how to setup and use the watch-only wallet when in external_signer mode.

@sstone sstone changed the title Implement Bitcoin Core's Hardware Wallet Interface Delegate Bitcoin Core's private key management to Eclair Mar 28, 2023
Copy link
Contributor

@remyers remyers left a comment

Choose a reason for hiding this comment

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

Changes to remove HWI look great. Especially s/WithExternalSigner/WithEclairSigner - this eliminated a major source of cognitive dissonance for me.

While looking at this some more, I noticed it should be possible to remove params from or possibly even migrated getDescriptors to bitcoin-kmp.

Copy link
Contributor

@remyers remyers left a comment

Choose a reason for hiding this comment

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

We should always check that the fee returned in FundTransactionResult matches what we requested - as you do in makeFundingTx and sendToPubkeyScript.

Because we must call signPsbt before we have all of the information this check can't be done in one place. This is not ideal because if we add a new call to fundTransaction we are likely to forget to do the check.

Would it make sense to not return the fee result in FundTransactionResult as a way to remove the chance it could be relied on without confirming it matches what we requested?

@sstone sstone force-pushed the implement-external-signer branch 2 times, most recently from b55f014 to 1036258 Compare April 6, 2023 08:09
Copy link
Contributor

@remyers remyers left a comment

Choose a reason for hiding this comment

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

Just found another issue with fundrawtransaction that looks like we can fix by adding some requires in the bitcoind client.

@sstone sstone force-pushed the implement-external-signer branch 3 times, most recently from c7bfaf4 to 9b1933e Compare April 26, 2023 07:44
@sstone sstone force-pushed the implement-external-signer branch 2 times, most recently from 0414fc3 to cae8eee Compare May 10, 2023 12:33
@sstone sstone force-pushed the implement-external-signer branch from cae8eee to 9dd602c Compare May 15, 2023 16:26
remyers
remyers previously approved these changes May 17, 2023
@sstone sstone force-pushed the implement-external-signer branch 2 times, most recently from ed95c47 to 9ca0db4 Compare June 5, 2023 09:38
@sstone sstone force-pushed the implement-external-signer branch from 9ca0db4 to 425378f Compare June 21, 2023 08:25
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

Concept ACK, the scope of this PR looks good, and you've been able to reduce it to a somewhat small change (mostly just a move to using the PSBT RPCs and verifying the results bitcoind sends back).

I think it can be simplified further (see comments below), but otherwise this looks good. I think this will deserve some clean-up to remove some kotlin<->scala awkwardness and make some functions more readable, but this is a good start!

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

I spent a lot of time staring at the code, playing with it and tweaking it, it looks mostly good to me!
All my comments are addressed by a follow-up PR at #2726, once this is integrated this feel ready 👍

t-bast
t-bast previously approved these changes Aug 29, 2023
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

sstone and others added 4 commits September 18, 2023 11:03
We create an empty watch-only wallet and import public descriptors generated by Eclair.
Bitcoin Core can fund transaction and manage utxos, but can no longer sign transactions.

* Check that spent amounts and utxos are consistent before we sign a PSBT

PSBT utxo fields include the amount that are being spent by the PSBT inputs, but there is a "fee attack"
where using amounts that are lower than what is actually spent may make us sign a tx that spends much more
in fees than we think.

* Check that non-segwit uxto has been provided and inputs are signed with SIGHASH_ALL

* Verify that Bitcoin Core's fee match what we specified

When we call Bitcoin Core's `fundrawtransaction` RPC method, we check that the fee that we pay match the fee rate that we requested.
The fee is computed using the utxo information that Bitcoin Core adds to our PSBT before we sign it.

We can safely used this information because if Bitcoin Core lies about the value of the inputs that we're spending then the signature we
produce will also not be valid (it commits to the value being spent).

When we're adding wallet inputs to "bump" the fees of a parent transaction we need to take the whole package into account when we verify the
actual fee rate, which is why some internal methods were modified to return the package weight that was used as reference when `fundrawtransaction`
was called.

* Check that fundrawtransaction does not add more than 1 change output

* Validate addresses and keys generated by bitcoin core

When eclair manages private keys, make sure that we can re-compute addresses and keys
generated by bitcoin core.

* Add a separate configuration file for Eclair's onchain signer

Eclair's onchain signer now has its own `eclair-signer.conf` configuration file in HOCON format.
It includes BIP39 mnemonic codes and passphrase, a wallet name and a timestamp.

When an `eclair-signer.conf` file is found, Eclair's API will return descriptors that can be imported into an
empty watch-only Bitcoin Wallet.
When wallet name in `eclair-signer.conf` matches the name of the Bitcoin Core wallet defined in `eclair.conf`
(`eclair.bitcoind.wallet`), Eclair will bypass Bitcoin Core and sign onchain transactions directly.
When eclair starts, if it is configured to manage bitcoin core's onchain key, and the configured wallet does not exist yet,
and eclair descriptor's timestamps are less then 2 hours old, eclair will automatically create the configured wallet with the
appropriate options and import its descriptors.
* Fix typos, use more explicit method names, ...

* Use hardcoded ports in tests instead of "first available port"

* User-friendly error message when eclair-baked wallet creation fails at startup

* Simplify ReplaceableTxFunder

* Replace EitherKt with Scala's Either type

* Update signPsbt() to return Try

* Skip validation of local non-change outputs:

Local non-change outputs send to an external address, for splicing out funds for example.
We assume that these inputs are trusted i.e have been created by a trusted API call and our local
onchain key manager will not validate them during the signing process.

* Do not track our wallet inputs/outputs

It is currently easy to identify the wallet inputs/outputs that we added to fund/bump transactions, we don't need to explicitly track them.

* Document why we use a separate, specific file for the onchain key manager

Using a new signer section is eclair.conf would be simpler but "leaks" because it becomes available everywhere
in the code through the actor system's settings instead of being contained to where it is actually needed
and could potentially be exposed through a bug that "exports" the configuration (through logs, ....)
though this is highly unlikely.
Refactor the `BitcoinCoreClient` and `LocalOnChainKeyManager` to:

- rely less on exceptions
- use more idiomatic scala (reduce dependency on kotlin types)
- provide more detailed logs

We also simplify the `useEclairSigner` field in `BitcoinCoreClient`.
The complexity of handling the case where there was an on-chain key
manager but for a different wallet than the one configured isn't
something that should be used, so it wasn't worth supporting.

Some checks were inconsistent and are now unified:

- checking the exact `scriptPubKey` in our outputs in TODO and TODO
- we allow using `fundTransaction` with a tx that already includes a
  change output (which may happen when RBF-ing a transaction)
- `getP2wpkhPubkeyHashForChange` didn't verify the returned key

We completely separate the two cases in `signPsbt`, because otherwise
in the non eclair-backed case, we were calling bitcoind's `processpsbt`
twice for no good reason, which is bad for performance.

We also decouple the `OnChainKeyManager` from the `BitcoinCoreClient`.
This lets users keep running their eclair node with a bitcoin client that
owns the private key while configuring the on-chain key manager for a
future bitcoin client that will leverage this on-chain key manager.

Users can use the eclair APIs to get the master xpub and descriptors to
properly configure their next bitcoin core node, and switch to it once it
has synchronized the descriptors.
t-bast
t-bast previously approved these changes Sep 19, 2023
@t-bast
Copy link
Member

t-bast commented Sep 19, 2023

LGTM, I'll start working on a follow-up PR to simplify the ReplaceableTxFunder

Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

This looks mostly good, and pretty well contained given the significance of the change. I have only minor comments.

My main remark would be on the structure of the PR: the PSBT change should have been made an independent commit at the beginning. It would have made it much easier to assess the impact of the delegated signature, which is the core of the PR.

I'm not familiar with PSBT and haven't reviewed the related code.

Rename `wallet` field to `walletName`
Improve comments related to signature workflow and verification of wallet inputs/outputs
Remove `descriptorChecksum` method: it's provided by bitcoin-kmp and does not need to be exposed
t-bast and others added 2 commits September 20, 2023 17:04
* Refactor `dummySignedCommitTx`

We only need the weight of the signed commit tx, it was error-prone to
provide what looks like a signed commit tx.

* Simplify replaceable tx funding

We were previously signing twice (with makes a call to `bitcoind`),
just to get the final weights and adjust the change outputs. This was
unnecessary, as we can adjust the weights before adding inputs.

We were also duplicating the checks where we verify that `bitcoind` is
malicious. We only need to check that once, during the final signing step.
Remove code to automatically create eclair-managed wallet

This code made the onchain key manager more complex, and for older wallets (when nodes are moved to a new machine for example) we need
to provide a manual process for creating a new empty wallet and importing descriptors generated by Eclair.

=> It is simpler to always use this manual process.
t-bast
t-bast previously approved these changes Sep 21, 2023
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

LGTM, great job I like how small the change ends up being compared to what it accomplishes. The test that randomly fails is unrelated to this PR, it's on my TODO list (it's related to the migration to bitcoind 24.1).

Depending upon the presence of an `eclair-signer.conf` file in Eclair's data directory, and the names of the wallet set in `eclair-signer.conf` and `eclair.conf`, we can have:
- no onchain key manager (which is the default)
- an onchain key manager that is used to generate descriptors through our API but that is not active. This is how you create a new watch-only Bitcoin wallet to be used by Eclair
- an onchain key manager that is used to sign bitcoin wallet transactions
Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

LGTM 🔑

There is some polishing left on the doc, and on namings (there are two many related terms: eclair signer ? managed bitcoin keys? onchain key manager?) but that can be done in a follow up PR.

@sstone sstone merged commit 6f87137 into master Sep 21, 2023
1 check passed
@sstone sstone deleted the implement-external-signer branch September 21, 2023 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants