Skip to content

Only store remote sigs for the local commit#1849

Merged
pm47 merged 5 commits intomasterfrom
only-store-remote-sigs
Jul 15, 2021
Merged

Only store remote sigs for the local commit#1849
pm47 merged 5 commits intomasterfrom
only-store-remote-sigs

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

t-bast
t-bast reviewed Jun 28, 2021
View reviewed changes
@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
@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
@t-bast t-bast force-pushed the only-store-remote-sigs branch from a1e31f5 to 2e074f7 Compare June 29, 2021 17:22
@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 force-pushed the only-store-remote-sigs branch from e88e6c5 to 226b609 Compare July 8, 2021 14:38
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.

pm47
pm47 commented Jul 9, 2021
View reviewed changes
t-bast
t-bast reviewed Jul 12, 2021
View reviewed changes
@pm47 pm47 force-pushed the only-store-remote-sigs branch from dfc5746 to 51b6bd7 Compare July 12, 2021 11:17
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 force-pushed the only-store-remote-sigs branch from 71fb7db to cdb7f4a Compare July 15, 2021 13:43
@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