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

Use bitcoin-kmp through bitcoin-lib #2038

Merged
merged 1 commit into from
Apr 5, 2022

Conversation

sstone
Copy link
Member

@sstone sstone commented Oct 28, 2021

We use a version of bitcoin-lib that uses bitcoin-kmp under the hood (see ACINQ/bitcoin-lib#63)
Its API is the same as before, but its package is now fr.acinq.bitcoinscala.

This is an alternative to #1949 that is much less invasive and will allow us to mix eclair and lightning-kmp.

There is a significant breaking change in bitcoin-lib: we only support OSes that are compatible with secp256k1-kmp (i.e we don't fallback to bouncycastle anymore if it cannot be loaded). In practice it means that eclair won't run on some 32 bits OSes and possibly some linux distros.

The version of bitcoin-lib that is used here is 0.20-KMP-SNAPSHOT, which is published to https://oss.sonatype.org/content/repositories/snapshots/. This is why this is a draft PR. If it is accepted I'll publish a new bitcoin-lib release to maven central and update this PR.

EDIT: this PR is now based on bitcoin-lib 0.22 which is available on maven central, supports Linux Arm64 (through bitcoin-kmp 0.8.3) and can be merged as-is.

@sstone sstone requested review from pm47 and t-bast October 28, 2021 15:58
@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch 3 times, most recently from e19d79a to f8b19c2 Compare November 3, 2021 20:26
@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch 2 times, most recently from 4d82148 to e9f9f22 Compare November 8, 2021 15:55
@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch from e9f9f22 to 539c7e5 Compare November 9, 2021 19:20
@codecov-commenter
Copy link

codecov-commenter commented Nov 19, 2021

Codecov Report

Merging #2038 (3b701d5) into master (5af042a) will decrease coverage by 0.06%.
The diff coverage is 67.50%.

@@            Coverage Diff             @@
##           master    #2038      +/-   ##
==========================================
- Coverage   83.92%   83.86%   -0.07%     
==========================================
  Files         186      186              
  Lines       13943    13953      +10     
  Branches      576      565      -11     
==========================================
  Hits        11701    11701              
- Misses       2242     2252      +10     
Impacted Files Coverage Δ
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala 50.84% <ø> (ø)
...air-core/src/main/scala/fr/acinq/eclair/Logs.scala 83.05% <ø> (ø)
.../src/main/scala/fr/acinq/eclair/MilliSatoshi.scala 100.00% <ø> (ø)
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 92.77% <ø> (ø)
.../src/main/scala/fr/acinq/eclair/PluginParams.scala 0.00% <ø> (ø)
...ir-core/src/main/scala/fr/acinq/eclair/Setup.scala 79.00% <ø> (ø)
...n/scala/fr/acinq/eclair/balance/BalanceActor.scala 10.16% <ø> (ø)
...ala/fr/acinq/eclair/balance/ChannelsListener.scala 93.10% <ø> (ø)
...n/scala/fr/acinq/eclair/balance/CheckBalance.scala 66.30% <ø> (ø)
.../acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala 88.13% <ø> (ø)
... and 112 more

@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch 3 times, most recently from 751d533 to d0e642d Compare December 3, 2021 09:58
@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch 4 times, most recently from 05a77e1 to 225da64 Compare December 14, 2021 22:04
@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch 2 times, most recently from 260aa5b to f364d38 Compare December 21, 2021 10:45
@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch from f364d38 to 4464318 Compare January 3, 2022 18:21
@sstone sstone marked this pull request as ready for review January 3, 2022 19:08
@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch 3 times, most recently from 6d597f2 to 9f6e050 Compare January 10, 2022 19:39
@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch from 9f6e050 to cc3df51 Compare January 11, 2022 19:29
@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch 3 times, most recently from 5d8359d to e4c4ba3 Compare January 25, 2022 15:49
@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch from e4c4ba3 to 22af2d3 Compare January 28, 2022 18:19
@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch 2 times, most recently from 1b370ad to bdf319d Compare February 8, 2022 08:19
@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch from bdf319d to d447fd2 Compare February 9, 2022 14:05
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.

Just a quick first pass, I need to play around more with the code

@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch from d447fd2 to 8f77e01 Compare February 10, 2022 11:01
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'm not seeing anything weird after playing with this version on regtest, and the minimal code changes look good to me.

@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch 3 times, most recently from 24d8afc to 5de05d5 Compare February 10, 2022 14:53
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.

ACK 5de05d5

@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch from 5de05d5 to 9bdb20d Compare March 8, 2022 15:26
@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch from 9bdb20d to dfec632 Compare March 16, 2022 13:38
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.

I just put this blocker upstream: ACINQ/bitcoin-lib#63 (review)

@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch from dfec632 to 3b701d5 Compare March 21, 2022 18:08
@sstone
Copy link
Member Author

sstone commented Mar 24, 2022

We now use bitcoin-kmp 0.21-KMP, bitcoin-kmp 0.8.3 and secp256k1-kmp 0.6.3 (which includes JNI bindings for Linux Arm64).

@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch 2 times, most recently from 0665457 to ec1d167 Compare March 29, 2022 11:38
@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch from ec1d167 to 13745c1 Compare April 4, 2022 09:58
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, just one nit

We use a version of bitcoin-lib that uses bitcon-kmp under the hood.
Its API is the same as before, but its package is now fr.acinq.bitcoin.scalacompat
@sstone sstone force-pushed the use-bitcoin-kmp-through-bitcoin-lib branch from 13745c1 to 7583eaa Compare April 5, 2022 08:54
@sstone sstone merged commit 7883bf6 into master Apr 5, 2022
@sstone sstone deleted the use-bitcoin-kmp-through-bitcoin-lib branch April 5, 2022 15:04
pm47 added a commit that referenced this pull request Apr 28, 2022
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.

4 participants