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

Only store remote sigs for the local commit #1849

Merged
merged 5 commits into from
Jul 15, 2021
Merged

Conversation

pm47
Copy link
Member

@pm47 pm47 commented Jun 24, 2021

That way, even if the node database or a backup is compromised, the attacker won't be able to force close channels from the outside.

This is best reviewed commit by commit.

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 haven't reviewed the migration / codecs, I just focused on the logic changes for now.
I think the migration stuff won't be very hard (tedious, but not hard).

@pm47 pm47 marked this pull request as ready for review June 28, 2021 14:00
@pm47 pm47 marked this pull request as draft June 28, 2021 14:17
@pm47 pm47 force-pushed the only-store-remote-sigs branch 3 times, most recently from 9242f49 to 2e074f7 Compare June 28, 2021 16: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.

ACK 2e074f7

I'll rebase #1848 on top of this branch.

@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2021

Codecov Report

Merging #1849 (cdb7f4a) into master (547d7e7) will increase coverage by 0.22%.
The diff coverage is 98.00%.

@@            Coverage Diff             @@
##           master    #1849      +/-   ##
==========================================
+ Coverage   87.04%   87.27%   +0.22%     
==========================================
  Files         156      159       +3     
  Lines       11724    11935     +211     
  Branches      476      466      -10     
==========================================
+ Hits        10205    10416     +211     
  Misses       1519     1519              
Impacted Files Coverage Δ
...n/scala/fr/acinq/eclair/channel/ChannelTypes.scala 100.00% <ø> (+2.85%) ⬆️
...ain/scala/fr/acinq/eclair/db/pg/PgChannelsDb.scala 90.24% <73.33%> (-2.42%) ⬇️
...q/eclair/crypto/keymanager/ChannelKeyManager.scala 87.50% <75.00%> (ø)
...a/fr/acinq/eclair/db/sqlite/SqliteChannelsDb.scala 94.04% <83.33%> (-1.85%) ⬇️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 86.21% <97.67%> (+0.02%) ⬆️
...n/scala/fr/acinq/eclair/balance/CheckBalance.scala 58.82% <100.00%> (ø)
.../fr/acinq/eclair/blockchain/fee/FeeEstimator.scala 100.00% <100.00%> (ø)
.../scala/fr/acinq/eclair/channel/ChannelConfig.scala 100.00% <100.00%> (ø)
...cala/fr/acinq/eclair/channel/ChannelFeatures.scala 100.00% <100.00%> (ø)
...in/scala/fr/acinq/eclair/channel/Commitments.scala 94.66% <100.00%> (+1.09%) ⬆️
... and 16 more

t-bast
t-bast previously approved these changes Jul 6, 2021
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 🚀

Copy link
Member Author

@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.

Rebased on #1860.

t-bast
t-bast previously approved these changes Jul 12, 2021
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, there's only this comment that hasn't been addressed: #1849 (comment)

t-bast
t-bast previously approved these changes Jul 12, 2021
@pm47 pm47 force-pushed the only-store-remote-sigs branch 2 times, most recently from 6b0d185 to 2c2078e Compare July 12, 2021 15:36
t-bast
t-bast previously approved these changes Jul 12, 2021
@pm47 pm47 marked this pull request as ready for review July 12, 2021 16:02
t-bast
t-bast previously approved these changes Jul 13, 2021
pm47 and others added 5 commits July 15, 2021 15:42
That way, even if the node database or a backup is compromised, the attacker
won't be able to force close channels from the outside.
* Separate internal channel config from features

Our current ChannelVersion field mixes two unrelated concepts: channel
features (as defined in Bolt 9) and channel internals (such as custom key
derivation). It is more future-proof to separate these two unrelated concepts
and will make it easier to implement channel types (see
lightning/bolts#880).

* Add shutdown script to channel remote params

This will be necessary when we implement `upfront_shutdown_script`.
This field can be set to `None` for all current channels as we don't support
the feature yet.
@pm47 pm47 merged commit e9df4ee into master Jul 15, 2021
@pm47 pm47 deleted the only-store-remote-sigs branch July 15, 2021 14:25
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