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.

Sign up for GitHub

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

Jump to bottom

Only store remote sigs for the local commit #1849

Merged
merged 5 commits into from
Jul 15, 2021
Merged

Only store remote sigs for the local commit #1849

merged 5 commits into from
Jul 15, 2021

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.

t-bast
t-bast reviewed Jun 25, 2021
View reviewed changes
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).

eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala Outdated Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala Outdated Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala Outdated Show resolved Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/channel/states/h/ClosingStateSpec.scala Show resolved Hide resolved
@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
t-bast
t-bast reviewed Jun 28, 2021
View reviewed changes
eclair-core/src/main/scala/fr/acinq/eclair/wire/internal/channel/version3/ChannelCodecs3.scala Outdated Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/wire/internal/channel/version3/ChannelCodecs3.scala Outdated Show resolved Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/channel/states/StateTestsHelperMethods.scala Outdated Show resolved Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/channel/states/StateTestsHelperMethods.scala Outdated Show resolved Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/channel/states/h/ClosingStateSpec.scala Outdated Show resolved Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/channel/states/h/ClosingStateSpec.scala Outdated Show resolved Hide resolved
@pm47 pm47 force-pushed the only-store-remote-sigs branch 3 times, most recently from 9242f49 to 2e074f7 Compare June 28, 2021 16:58
t-bast
t-bast reviewed Jun 29, 2021
View reviewed changes
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.

@sstone sstone mentioned this pull request Jun 29, 2021
Merged
@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2021 •
edited
Loading

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 98.00499% with 8 lines in your changes missing coverage. Please review.

Project coverage is 87.27%. Comparing base (547d7e7) to head (cdb7f4a).
Report is 631 commits behind head on master.

Files with missing lines Patch % Lines
...ain/scala/fr/acinq/eclair/db/pg/PgChannelsDb.scala 73.33% 4 Missing ⚠️
...a/fr/acinq/eclair/db/sqlite/SqliteChannelsDb.scala 83.33% 2 Missing ⚠️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 97.67% 1 Missing ⚠️
...q/eclair/crypto/keymanager/ChannelKeyManager.scala 75.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            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
Files with missing lines Coverage Δ
...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%> (ø)
...n/scala/fr/acinq/eclair/channel/ChannelTypes.scala 100.00% <ø> (+2.85%) ⬆️
...in/scala/fr/acinq/eclair/channel/Commitments.scala 94.66% <100.00%> (+1.09%) ⬆️
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala 95.94% <100.00%> (-0.01%) ⬇️
...clair/channel/publish/ReplaceableTxPublisher.scala 85.79% <100.00%> (ø)
...scala/fr/acinq/eclair/crypto/WeakEntropyPool.scala 94.73% <100.00%> (ø)
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala 89.58% <100.00%> (+0.05%) ⬆️
... and 12 more

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

@t-bast t-bast mentioned this pull request Jul 7, 2021
Merged
pm47
pm47 commented Jul 8, 2021
View reviewed changes
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
View reviewed changes
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
View reviewed changes
@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
View reviewed changes
@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
View reviewed changes
pm47 and others added 5 commits July 15, 2021 15:42
@pm47
only store remote sigs for local commitment
e2b8b19 
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.
@t-bast @pm47
Separate internal channel config from features (#1852)
67d9a40 
* 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
backporting changes
fe1c90a
@pm47
force migration to new codecs
399771e
@pm47
better json serializer for unknown features
cdb7f4a
@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
Reviewers

@t-bast t-bast t-bast approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pm47 @codecov-commenter @t-bast