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

Deterministic channel keypath #1086

Closed
wants to merge 5 commits into from
Closed

Deterministic channel keypath #1086

wants to merge 5 commits into from

Conversation

araspitzu
Copy link
Contributor

Summary of the changes

Key path generation

Eclair uses BIP32 key paths to derive all the various keys needed for channel operation, instead of generating a new random keypath when negotiating a channel we now use an external source of entropy to feed randomness in the path construction. The entropy is taken from the funding input's outpoint and in the case we're fundee we use a combination of block height and an incrementing counter to ensure we never reuse keys, the full construction is highlighted below:

Funder scenario:

keyPath = m/47'/2'/SHA256(funding_tx.input[0].outpoint)/0'

Fundee scenario:

fundingPubkey.keyPath = m/47'/2'/SHA256(blockchain_height || counter)/2'
keyPath = m/47'/2'/SHA256(funding_tx.output[channel_output_index].scriptPubkey)/1'

Channel creation changes

Funder scenario : in order to send an open_channel we need to compute the basepoints for the channel operation and indeed we need the key path to do so. This means that before starting the negotiation of the channel opening we must ask the wallet to provide an input for the channel funding, we will use the input outpoint to construct the channel key path. Such particular flow modified the Channel FSM to have some new intermediate states, namely where we ask the wallet to create a funding transaction WAIT_FOR_FUNDING_INTERNAL_CREATED and where we ask the wallet to sign it WAIT_FOR_FUNDING_SIGNED_INTERNAL. Note that after we ask the wallet to make a funding transaction we strip the signatures from it and we later query the wallet again to add new signatures, the wallet will sign the updated transaction with the script containing the funding pubkey from the remote peer (we have the remote funding pubkey only after we receive accept_channel).
Fundee scenario: this scenario uses 2 different keypaths to operate the channel, one for the funding pubkey and one for all the other points. This is necessary because we want to use the scriptPubkey from the funding transaction to construct the keypath but to do so we need to derive out funding pubkey first, in order to do that we use a combination of block height and a counter. This scenario doesn't alter the channel opening flow.

Reference: funder construction and fundee construction

Codecs and Commitments upgrades

The usage of 2 different kinds of keypaths (one for funder scenario, two for fundee) forced to upgrade the LocalParams class to store an Either[KeyPath, KeyPathFundee], we also need to upgrade the codecs to store these new objects and we must do it in a backward compatible manner.
The codec has been modified to support commitment versioning and depending on the version we'll fetch the correct key paths.

Database layer upgrades

There is no db migration needed here but we add a new table to channel db, block_key_counter table. This is used to emulate an atomic auto incrementing counter used for the fundee key path construction, the alternative implementation using the AtomicLong has been ditched because of security concerns.

Testing

The PR has been manual tested against eclair master, there are also new codec tests and backward compatibility test for fundee scenario in DATA_NORMAL: here. To ensure previous version with yet-to-open channels can safely upgrade there are some test where we assert we can do FSM transition like "WAIT_FOR_FUNDING_CONFIRMED -> WAIT_FOR_FUNDING_LOCKED", test here and here.

@araspitzu araspitzu force-pushed the eclair_dkp branch 2 times, most recently from 8d58abc to 785b9d0 Compare July 22, 2019 13:27
@araspitzu araspitzu marked this pull request as ready for review July 22, 2019 14:06
@codecov-io
Copy link

codecov-io commented Jul 23, 2019

Codecov Report

Merging #1086 into master will decrease coverage by 0.02%.
The diff coverage is 89.89%.

@@            Coverage Diff             @@
##           master    #1086      +/-   ##
==========================================
- Coverage    82.7%   82.68%   -0.03%     
==========================================
  Files         101      101              
  Lines        7621     7709      +88     
  Branches      293      291       -2     
==========================================
+ Hits         6303     6374      +71     
- Misses       1318     1335      +17
Impacted Files Coverage Δ
...air/blockchain/electrum/ElectrumEclairWallet.scala 0% <0%> (ø) ⬆️
...a/fr/acinq/eclair/db/sqlite/SqliteChannelsDb.scala 96.92% <100%> (+1.08%) ⬆️
...n/scala/fr/acinq/eclair/channel/ChannelTypes.scala 100% <100%> (ø) ⬆️
...scala/fr/acinq/eclair/crypto/LocalKeyManager.scala 91.3% <100%> (+1.83%) ⬆️
...ain/scala/fr/acinq/eclair/wire/ChannelCodecs.scala 100% <100%> (ø) ⬆️
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala 94.44% <100%> (+0.01%) ⬆️
...nq/eclair/blockchain/electrum/ElectrumWallet.scala 80.66% <100%> (+0.14%) ⬆️
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala 73.61% <100%> (-1.31%) ⬇️
...a/fr/acinq/eclair/wire/LightningMessageTypes.scala 100% <100%> (ø) ⬆️
...in/scala/fr/acinq/eclair/channel/Commitments.scala 87.02% <100%> (ø) ⬆️
... and 6 more

# Conflicts:
#	eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala
#	eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelTypes.scala
#	eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala
#	eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/OfflineStateSpec.scala
@sstone
Copy link
Member

sstone commented Aug 26, 2019

Superseded by #1097

@sstone sstone closed this Aug 26, 2019
@araspitzu araspitzu deleted the eclair_dkp branch November 18, 2019 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants