-
Notifications
You must be signed in to change notification settings - Fork 267
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
Conversation
7ff5d62
to
33c1efc
Compare
Codecov Report
❗ 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
|
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.
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.
eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxFunder.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxFunder.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/LocalOnchainKeyManager.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/LocalOnchainKeyManager.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/OnchainKeyManager.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreClientSpec.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoindService.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/LocalOnchainKeyManager.scala
Outdated
Show resolved
Hide resolved
a733b23
to
865d3b2
Compare
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.
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.
eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/LocalOnchainKeyManager.scala
Outdated
Show resolved
Hide resolved
53c3c77
to
9e0746b
Compare
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.
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?
eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxFunder.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxFunder.scala
Outdated
Show resolved
Hide resolved
b55f014
to
1036258
Compare
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.
Just found another issue with fundrawtransaction
that looks like we can fix by adding some requires in the bitcoind client.
eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxFunder.scala
Outdated
Show resolved
Hide resolved
762f19c
to
b6cd39f
Compare
eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala
Show resolved
Hide resolved
c7bfaf4
to
9b1933e
Compare
eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreClientSpec.scala
Show resolved
Hide resolved
0414fc3
to
cae8eee
Compare
cae8eee
to
9dd602c
Compare
ed95c47
to
9ca0db4
Compare
9ca0db4
to
425378f
Compare
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.
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!
eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxFunder.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPrePublisher.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fund/InteractiveTxBuilder.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/LocalOnchainKeyManager.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxFunder.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxFunder.scala
Outdated
Show resolved
Hide resolved
d8708d9
to
f9fa8a7
Compare
90ea2ac
to
728e38d
Compare
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.
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 👍
728e38d
to
9562d30
Compare
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.
LGTM 🚀
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.
72288a0
to
eff698a
Compare
LGTM, I'll start working on a follow-up PR to simplify the |
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 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.
eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/OnChainKeyManager.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/LocalOnChainKeyManager.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/LocalOnChainKeyManager.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fund/InteractiveTxBuilder.scala
Outdated
Show resolved
Hide resolved
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
* 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.
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.
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
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.
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.
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:
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:
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:
Signature Workflow
Whenever we need to fund and sign a transaction we:
This workflow is safe because:
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.