Skip to content

Implement option-upfront-shutdown-script#1846

Merged
sstone merged 6 commits into
masterfrom
implement-upfront-shutdown-script
Jul 16, 2021
Merged

Implement option-upfront-shutdown-script#1846
sstone merged 6 commits into
masterfrom
implement-upfront-shutdown-script

Conversation

@sstone
Copy link
Copy Markdown
Member

@sstone sstone commented Jun 23, 2021

No description provided.

@t-bast t-bast mentioned this pull request Jun 23, 2021
Closed
@sstone sstone force-pushed the implement-upfront-shutdown-script branch from 416de58 to d315f1b Compare June 24, 2021 09:42
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 24, 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 91.89189% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.34%. Comparing base (b4183ed) to head (e317405).
Report is 615 commits behind head on master.

Files with missing lines Patch % Lines
...in/scala/fr/acinq/eclair/channel/Commitments.scala 76.92% 3 Missing ⚠️
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala 88.23% 2 Missing ⚠️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 97.36% 1 Missing ⚠️

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

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #1846   +/-   ##
=======================================
  Coverage   87.34%   87.34%           
=======================================
  Files         159      159           
  Lines       11960    11989   +29     
  Branches      475      501   +26     
=======================================
+ Hits        10446    10472   +26     
- Misses       1514     1517    +3
Files with missing lines Coverage Δ
...core/src/main/scala/fr/acinq/eclair/Features.scala 99.00% <100.00%> (+0.03%) ⬆️
...cala/fr/acinq/eclair/channel/ChannelFeatures.scala 100.00% <100.00%> (ø)
...q/eclair/wire/protocol/LightningMessageTypes.scala 96.42% <100.00%> (+0.27%) ⬆️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 86.42% <97.36%> (+0.07%) ⬆️
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala 96.04% <88.23%> (-0.34%) ⬇️
...in/scala/fr/acinq/eclair/channel/Commitments.scala 93.87% <76.92%> (-0.79%) ⬇️

... and 2 files with indirect coverage changes

@sstone sstone force-pushed the implement-upfront-shutdown-script branch 6 times, most recently from d9a5896 to 2d24a57 Compare June 29, 2021 08:07
@sstone
Copy link
Copy Markdown
Member Author

sstone commented Jun 29, 2021

This PR also fixes a bug in how we handle invalid Shutdown messages: we would fore-close the channel instead of failing the connection.
Tested with c-lightning v0.9.3 and lnd v0.13.0-beta

@sstone sstone force-pushed the implement-upfront-shutdown-script branch from 2d24a57 to 8f96786 Compare June 29, 2021 11:38
@sstone
Copy link
Copy Markdown
Member Author

sstone commented Jun 29, 2021

Rebased on top of #1849

@t-bast
Copy link
Copy Markdown
Member

t-bast commented Jun 29, 2021 •
edited
Loading

Can you separate the channel codecs changes from the actual upfront_shutdown_script feature? IMO we should first focus on just the channel codec changes and migration and integrate it into #1849, which will already be quite a big PR on its own.

Then once the channel codecs are changed on master, we can review a separate PR that implements upfront_shutdown_script and stores the remote script in the new placeholder you added in RemoteParams. I tried implementing upfront_shutdown_script a while ago, and I found some subtleties, so I think it's worth reviewing in a dedicated PR.

@sstone sstone force-pushed the implement-upfront-shutdown-script branch 2 times, most recently from c7a82ff to a526940 Compare July 8, 2021 15:49
@sstone sstone changed the title Implement option-upfront-shutdown-script [WIP] Implement option-upfront-shutdown-script Jul 8, 2021
@sstone
Copy link
Copy Markdown
Member Author

sstone commented Jul 8, 2021

Rebased on top of #1849

@sstone sstone force-pushed the implement-upfront-shutdown-script branch 4 times, most recently from 710ac96 to 807d00f Compare July 15, 2021 14:13
pm47 added a commit that referenced this pull request Jul 15, 2021
@pm47
Channels data format migration (#1849)
e9df4ee 
There are three otherwise unrelated changes, that we group together to only have one migration:
- remove local signatures for local commitments (this PR)
- separate internal channel config from channel features (#1848)
- upfront shutdown script (#1846)

We increase database version number in sqlite and postgres to force a full data migration.

The goal of removing local signatures from the channel data is that even if the node database or
a backup is compromised, the attacker won't be able to force close channels from the outside.
@sstone sstone force-pushed the implement-upfront-shutdown-script branch from 807d00f to 0ece233 Compare July 15, 2021 14:48
t-bast
t-bast reviewed Jul 16, 2021
View reviewed changes
Comment thread eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala Outdated
Comment thread eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala Outdated
Comment thread eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala Outdated
Comment thread eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala Outdated
Comment thread eclair-core/src/main/resources/reference.conf Outdated
Comment thread eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala Outdated
Comment thread eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala Outdated
Comment thread eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala Outdated
Comment thread eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala Outdated
Comment thread eclair-core/src/test/scala/fr/acinq/eclair/channel/states/g/NegotiatingStateSpec.scala Outdated
@sstone sstone force-pushed the implement-upfront-shutdown-script branch from ca2232f to ae1df2f Compare July 16, 2021 13:30
@sstone sstone marked this pull request as ready for review July 16, 2021 13:35
t-bast
t-bast reviewed Jul 16, 2021
View reviewed changes
Comment thread eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala Outdated
Comment thread eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala Outdated
Comment thread eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala Outdated
Comment thread eclair-core/src/test/scala/fr/acinq/eclair/channel/states/a/WaitForAcceptChannelStateSpec.scala
Comment thread eclair-core/src/test/scala/fr/acinq/eclair/channel/states/a/WaitForAcceptChannelStateSpec.scala
Comment thread eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala Outdated
@sstone sstone force-pushed the implement-upfront-shutdown-script branch from c85035e to f324eae Compare July 16, 2021 14:31
t-bast
t-bast reviewed Jul 16, 2021
View reviewed changes
Copy link
Copy Markdown
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, I would just add a few test assertions and address this comment and we should be good to go

Comment thread eclair-core/src/test/scala/fr/acinq/eclair/channel/states/a/WaitForAcceptChannelStateSpec.scala
Comment thread eclair-core/src/test/scala/fr/acinq/eclair/channel/states/a/WaitForOpenChannelStateSpec.scala
t-bast
t-bast approved these changes Jul 16, 2021
View reviewed changes
Copy link
Copy Markdown
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 🚀

@sstone sstone merged commit 01b4073 into master Jul 16, 2021
@sstone sstone deleted the implement-upfront-shutdown-script branch July 16, 2021 15:27
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

@sstone @codecov-commenter @t-bast