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

Add bech32 support for electrum wallet #1170

Closed
wants to merge 34 commits into from
Closed

Conversation

araspitzu
Copy link
Contributor

Add support for p2wpkh scriptPubKeys in our electrum wallet, this will enable eclair-mobile to receive to bech32 addresses. The electrum wallet now accepts a WalletType parameter that is used to decide whether the wallet creates outputs of type p2sh-of-p2wpkh (current type) or the new p2wpkh, the wallet only supports one of the two and they can't be mixed, if started with p2wpkh it won't find unspent outputs of the other type.

@codecov-io
Copy link

codecov-io commented Oct 10, 2019

Codecov Report

Merging #1170 into master will increase coverage by 0.23%.
The diff coverage is 82.69%.

@@            Coverage Diff             @@
##           master    #1170      +/-   ##
==========================================
+ Coverage   76.38%   76.61%   +0.23%     
==========================================
  Files         140      141       +1     
  Lines        9545     9601      +56     
  Branches      378      384       +6     
==========================================
+ Hits         7291     7356      +65     
+ Misses       2254     2245       -9
Impacted Files Coverage Δ
...scala/fr/acinq/eclair/crypto/LocalKeyManager.scala 87.8% <100%> (ø) ⬆️
...blockchain/electrum/db/sqlite/SqliteWalletDb.scala 86.59% <100%> (+2.07%) ⬆️
...ir-core/src/main/scala/fr/acinq/eclair/Setup.scala 71.59% <44.44%> (-1.51%) ⬇️
...nq/eclair/blockchain/electrum/ElectrumWallet.scala 81.81% <67.92%> (+1.06%) ⬆️
...fr/acinq/eclair/blockchain/electrum/KeyStore.scala 93.33% <93.33%> (ø)
...cinq/eclair/blockchain/bitcoind/zmq/ZMQActor.scala 89.74% <0%> (-5.13%) ⬇️
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala 75.14% <0%> (-0.3%) ⬇️
...in/scala/fr/acinq/eclair/channel/Commitments.scala 89.23% <0%> (+0.34%) ⬆️
... and 2 more

@araspitzu araspitzu marked this pull request as ready for review October 11, 2019 10:28
@sstone
Copy link
Member

sstone commented Oct 23, 2019

@araspitzu for reference I've implemented some of the proposed changes in branch https://github.com/ACINQ/eclair/tree/native_segwit_support_fd, including a different way of having specific BIP49/BIP84 tests.

@araspitzu
Copy link
Contributor Author

bf6d7ba adds the clear cache functionality to the electrum wallet, this can be used by eclair-mobile to trigger a full resync with the electrum server by sending a ClearWalletCache to the wallet.

@araspitzu
Copy link
Contributor Author

Eclair-mobile integration with this PR needs to handle wallet restore in a special case. On restore the user will be asked whether he's restoring a bech32 or p2sh wallet, to avoid errors eclair-mobile can use special naming for the backup, this will avoid consistency issue because it forces the channel backup to match the electrum wallet type. However this still leaves room for user errors: if the user selects the wrong type then when eclair-mobile reconnects (i.e to ACINQ node) it will trigger the DLP and the remote will force close the channels. A potential solution to avoid that is to pair the wallet type with the nodeId, however this also have several disadvantages.

@araspitzu
Copy link
Contributor Author

High level interaction between eclair-mobile and eclair-core in the restore scenario

During the restore scenario the user is prompted to choose a wallet-type and depending on the choice eclair-mobile will load a different backup so the following scenarios can happen:

  • User has no existing open channels and no existing backup: in this case whatever wallet-type is selected there are no issues.
  • User has existing open channels from a previous wallet but selects the wrong wallet type: in this case when reconnecting to remote the existing channel will get force-closed and eclair-mobile won't notice it had funds at stake, to resolve this scenario the user can redo the restore procedure choosing the correct wallet-type then eclair should be able to track the existing channel closure and claim the funds.

@dpad85
Copy link
Member

dpad85 commented Nov 8, 2019

The restore scenario for eclair mobile can be tricky and it would be nice to avoid any consistency issues.

When you say User in fact I would say node id, since there is no such thing as a User. So if the node id changes with wallet type, then the remote will not attempt to reestablish the channel. The p2sh and bech32 wallet would be completely isolated and as such we cannot have consistency issues.

I'm not sure I understand why changing the node id would cause any technical issues, since it's virtually the same thing as using a different seed?

However I understand that we would have to write a special case in the code generating the node id and maintain it for a long time, just to prevent a onetime issue in bech32 migration for eclair mobile.

@araspitzu
Copy link
Contributor Author

The argument against changing the nodeid is that it's often used as identifier in authentication prototocols and/or signature schemes, for example both c-lightning and lnd support signing a message with your node key (note that eclair don't support that at the moment); there might be other integrations of which we're aren't aware now. On the other hand if changing the nodeid won't break any existing service/integration then i'm in favor of it.

@araspitzu
Copy link
Contributor Author

2ee9b13 adds the exception to use a different derivation path for the node key if the wallet is electrum in bech32 mode.

# Conflicts:
#	eclair-core/src/test/scala/fr/acinq/eclair/StartupSpec.scala
@t-bast
Copy link
Member

t-bast commented Nov 24, 2021

We've removed support for Electrum entirely, so this PR is now obsolete.

@t-bast t-bast closed this Nov 24, 2021
@t-bast t-bast deleted the native_segwit_support branch December 15, 2021 09:46
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