Skip to content

Support p2tr bitcoin wallet#3026

Merged
sstone merged 14 commits into
masterfrom
support-p2tr-bitcoin-wallet
Mar 18, 2025
Merged

Support p2tr bitcoin wallet#3026
sstone merged 14 commits into
masterfrom
support-p2tr-bitcoin-wallet

Conversation

@sstone

@sstone sstone commented Mar 4, 2025

Copy link
Copy Markdown
Member

This PR makes eclair compatible with bitcoin core wallet that are configured to use p2tr default addresses (even if eclair is actually managing the wallet's private keys).
It is best reviewed commit by commit , da7950b being the most tricky.

We still need to generate p2wpkh scripts to sends funds to when a channel is mutually closed:

  • for static_remote_key channels that do not support anchor outputs
  • for channels that do not support shutdown_anysegwit

This PR supersedes #2873.

@sstone sstone mentioned this pull request Mar 4, 2025
Closed
@t-bast t-bast mentioned this pull request Mar 12, 2025
Merged
t-bast
t-bast reviewed Mar 12, 2025
View reviewed changes

@t-bast t-bast left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That was a complex PR! This is mostly looking good, I don't see any blocking points. I have worked through the code myself to review it correctly, and #3038 contains fixes for my comments.

Comment thread eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala Outdated
Comment thread eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala Outdated
Comment thread eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/OnchainPubkeyRefresher.scala Outdated
Comment thread eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/CommonHandlers.scala
Comment thread eclair-core/src/main/scala/fr/acinq/eclair/io/OpenChannelInterceptor.scala Outdated
Comment thread eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/OnChainKeyManager.scala Outdated
Comment thread eclair-core/src/main/scala/fr/acinq/eclair/channel/fund/InteractiveTxBuilder.scala Outdated
Comment thread eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/LocalOnChainKeyManager.scala Outdated
Comment thread eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreClientSpec.scala Outdated
@t-bast
t-bast marked this pull request as ready for review March 12, 2025 16:31
@t-bast

t-bast commented Mar 13, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

a682a75 is missing a lot of the changes from #3038...why didn't you instead review #3038 and make comments on it so that we can end up merging it into your branch? That would make reviewing a lot easier, because now I need to diff my branch with yours and iterate on adding comments here until the results are good :/

@sstone

sstone commented Mar 13, 2025

Copy link
Copy Markdown
Member Author

a682a75 is missing a lot of the changes from #3038...why didn't you instead review #3038 and make comments on it so that we can end up merging it into your branch? That would make reviewing a lot easier, because now I need to diff my branch with yours and iterate on adding comments here until the results are good :/

Because there are many different types of changes with a single commit and I found it easier to pick the changes that were obvious improvements and iterate on the ones I'm not sure about yet. But you're right reviewing your PR will be quicker here, that what I'll do. I can revert the last commit if it makes things easier for you.

@sstone
sstone force-pushed the support-p2tr-bitcoin-wallet branch from a682a75 to f58a2e5 Compare March 13, 2025 11:34
t-bast added a commit that referenced this pull request Mar 17, 2025
@t-bast
Address t-bast's comments for #3026 (#3038)
c931d22 
- Rework `OnChainAddressRefresher`
- Reformat / clean-up code and comments
- Improve tests
@t-bast

t-bast commented Mar 17, 2025

Copy link
Copy Markdown
Member

I've checked out a snapshot of that branch locally to be able to do the diff after the rebase. Can you:

  • rebase this PR on master
  • make sure that it works e2e on regtest (with or without the eclair signer)

Once that's done I think we can merge this!

sstone and others added 14 commits March 17, 2025 13:36
@sstone
Add an address type parameter to our bitcoin core RPC client
3435e1c 
We defined an new AddressType type that can optionally be passed to calls tp `getReceiveAddress()`.
This type can represent p2wpkh or p2tr addresses.
There are no functional changes, we don't use p2tr addresses yet.
@sstone
Add a "chain hash" property to the bitcoin rpc client
c3626f7
@sstone
GetReceiveAddress(): remove "label" parameter
b98d86e 
It is not used anymore.
@sstone
Replace getP2wpkhPubkeyHashForChange() with getChangePublicKeyScript()
ea3fd72 
We should not hardcode the type of change output that we want but instead use the default change type configured in bitcoin core.
@sstone
Simplify SingleKeyOnChainWallet
e6a6a27 
Use a key manager to generate a local address and sign transactions (instead of signing them manually).
No functional changes.
@sstone
LocalOnChainKeyManager: support signing BIP86 transactions
2b525da
@sstone
make InteractiveTxBuilder compatible with p2tr wallets
c632fe6 
If our wallets adds p2tr inputs, to sign them we need to know all the utxos spent by the tx we're building, not just the
one spent by the tx we're signing.
@sstone
Cache both onchain public key and public key scripts
d3a8739
@sstone
Test bitcoin core change output type selection
e1d50b1 
If the -changetype option is not set, then bitcoin core will fund transactions with inputs that match transaction's outputs (i.e it will
add p2wpkh change outputs if the tx sends to p2wpkh outputs, p2tr change outputs if it sends to p2tr outputs...).
@sstone
Integration tests: change default bitcoin core address type to p2tr
06a6df7
@t-bast @sstone
Address t-bast's comments for #3026 (#3038)
575b567 
- Rework `OnChainAddressRefresher`
- Reformat / clean-up code and comments
- Improve tests
@sstone
sendonchain: unlock utxos if publishing fails
8693447
@sstone
LocalOnChainKeyManager: fix logs
d60175e
@sstone
Add signing test with mixed p2wpkh/p2tr inputs and an eclair-backed b…
8a1f963 
…itcoin core wallet
@sstone
sstone force-pushed the support-p2tr-bitcoin-wallet branch from c931d22 to 8a1f963 Compare March 17, 2025 16:32
@sstone

sstone commented Mar 17, 2025

Copy link
Copy Markdown
Member Author

Rebased on master at b98930b, I've added a test to verify that eclair properly signs transactions with mixed p2tr/p2wpkh inputs when it manages bitcoin core wallet keys.

t-bast
t-bast approved these changes Mar 18, 2025
View reviewed changes

@t-bast t-bast left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, good work, I'm glad we got this done!

@sstone
sstone merged commit 5d6e556 into master Mar 18, 2025
@sstone
sstone deleted the support-p2tr-bitcoin-wallet branch March 18, 2025 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@t-bast t-bast t-bast approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sstone @t-bast