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

Implement option-upfront-shutdown-script #1846

Merged
merged 6 commits into from
Jul 16, 2021
Merged

Implement option-upfront-shutdown-script #1846

merged 6 commits into from
Jul 16, 2021

Conversation

sstone
Copy link
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

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
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
Member Author

sstone commented Jun 29, 2021

Rebased on top of #1849

@t-bast
Copy link
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
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
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/Helpers.scala Outdated Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala Outdated Show resolved Hide resolved
eclair-core/src/main/resources/reference.conf Outdated Show resolved Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala Outdated Show resolved Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala Outdated Show resolved Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala Outdated Show resolved Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala Outdated Show resolved Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/channel/states/g/NegotiatingStateSpec.scala Outdated Show resolved Hide resolved
@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
@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
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

eclair-core/src/test/scala/fr/acinq/eclair/channel/states/a/WaitForAcceptChannelStateSpec.scala Show resolved Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/channel/states/a/WaitForOpenChannelStateSpec.scala Show resolved Hide resolved
t-bast
t-bast approved these changes Jul 16, 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 🚀

@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