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

Explain how and why we use bitcoin core #2473

Merged
merged 3 commits into from
Nov 3, 2022
Merged

Explain how and why we use bitcoin core #2473

merged 3 commits into from
Nov 3, 2022

Conversation

sstone
Copy link
Member

@sstone sstone commented Nov 2, 2022

Explain why we chose to delegate most onchain tasks to bitcoin core (including onchain wallet management), the additional requirements that it creates and also the benefits in terms of security.

@sstone sstone requested review from pm47 and t-bast November 2, 2022 09:56
Explain why we chose to delegate most onchain tasks to bitcoin core (including onchain wallet management), the additional requirements that it creates and also the benefits in terms of security.
@sstone sstone marked this pull request as draft November 2, 2022 10:12
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.

A few nits, otherwise LGTM

@codecov-commenter
Copy link

Codecov Report

Merging #2473 (9a21baa) into master (3b12475) will increase coverage by 0.06%.
The diff coverage is 93.98%.

@@            Coverage Diff             @@
##           master    #2473      +/-   ##
==========================================
+ Coverage   84.92%   84.99%   +0.06%     
==========================================
  Files         198      198              
  Lines       15783    15812      +29     
  Branches      637      685      +48     
==========================================
+ Hits        13404    13439      +35     
+ Misses       2379     2373       -6     
Impacted Files Coverage Δ
...in/scala/fr/acinq/eclair/channel/ChannelData.scala 98.11% <ø> (ø)
.../main/scala/fr/acinq/eclair/db/DualDatabases.scala 11.27% <0.00%> (+0.32%) ⬆️
.../src/main/scala/fr/acinq/eclair/db/NetworkDb.scala 100.00% <ø> (ø)
.../eclair/wire/protocol/LightningMessageCodecs.scala 100.00% <ø> (ø)
...in/scala/fr/acinq/eclair/channel/fsm/Channel.scala 86.27% <66.66%> (+0.11%) ⬆️
...main/scala/fr/acinq/eclair/router/Validation.scala 95.65% <87.23%> (+0.22%) ⬆️
.../scala/fr/acinq/eclair/message/OnionMessages.scala 81.81% <90.00%> (+5.45%) ⬆️
...src/main/scala/fr/acinq/eclair/router/Router.scala 94.20% <93.33%> (+0.02%) ⬆️
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala 52.10% <100.00%> (+1.56%) ⬆️
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 93.28% <100.00%> (+0.02%) ⬆️
... and 21 more

t-bast
t-bast previously approved these changes Nov 2, 2022
Copy link
Contributor

@remyers remyers left a comment

Choose a reason for hiding this comment

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

LGTM!

It might be useful to collaborate on a best-practices document to explore the interface between Lightning and on-chain where we could get into more technical details about the trade-offs.

Copy link
Contributor

@remyers remyers left a comment

Choose a reason for hiding this comment

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

Looks good!

@sstone sstone marked this pull request as ready for review November 3, 2022 08:02
@sstone sstone merged commit 7c8bdb9 into master Nov 3, 2022
@sstone sstone deleted the readme-patch-1 branch November 3, 2022 08:04
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.

6 participants