Implement option-upfront-shutdown-script by sstone · Pull Request #1846 · ACINQ/eclair

sstone · 2021-06-23T15:54:35Z

No description provided.

codecov-commenter · 2021-06-24T09:56:48Z

⚠️ Please install the 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 · 2021-06-29T09:15:21Z

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 · 2021-06-29T11:42:56Z

Rebased on top of #1849

t-bast · 2021-06-29T17:30:54Z

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 · 2021-07-08T16:03:34Z

Rebased on top of #1849

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.

eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala

eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala

eclair-core/src/main/resources/reference.conf

eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala

eclair-core/src/test/scala/fr/acinq/eclair/channel/states/g/NegotiatingStateSpec.scala

We now advertise the optional feature bit for this option.

Users will need to explicitly activate it.

…ig file

eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala

eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala

eclair-core/src/test/scala/fr/acinq/eclair/channel/states/a/WaitForAcceptChannelStateSpec.scala

eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala

t-bast

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

eclair-core/src/test/scala/fr/acinq/eclair/channel/states/a/WaitForOpenChannelStateSpec.scala

t-bast

LGTM 🚀

t-bast mentioned this pull request Jun 23, 2021

Explicit channel type negotiation #1847

Closed

sstone force-pushed the implement-upfront-shutdown-script branch from 416de58 to d315f1b Compare June 24, 2021 09:42

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

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

sstone force-pushed the implement-upfront-shutdown-script branch 2 times, most recently from c7a82ff to a526940 Compare July 8, 2021 15:49

sstone changed the title ~~Implement option-upfront-shutdown-script [WIP]~~ Implement option-upfront-shutdown-script Jul 8, 2021

sstone force-pushed the implement-upfront-shutdown-script branch 4 times, most recently from 710ac96 to 807d00f Compare July 15, 2021 14:13

sstone force-pushed the implement-upfront-shutdown-script branch from 807d00f to 0ece233 Compare July 15, 2021 14:48

t-bast reviewed Jul 16, 2021

View reviewed changes

sstone added 4 commits July 16, 2021 15:17

Implement option-upfront-shutdown-script

3efbe51

We now advertise the optional feature bit for this option.

Do not activate option_upfront_shutdown_Script by defaut

d172de1

Users will need to explicitly activate it.

Address review comments

4c7ff83

Add comments about option_upfront_shutdown_script in our default conf…

ae1df2f

…ig file

sstone force-pushed the implement-upfront-shutdown-script branch from ca2232f to ae1df2f Compare July 16, 2021 13:30

sstone marked this pull request as ready for review July 16, 2021 13:35

t-bast reviewed Jul 16, 2021

View reviewed changes

Address review comments

f324eae

sstone force-pushed the implement-upfront-shutdown-script branch from c85035e to f324eae Compare July 16, 2021 14:31

t-bast reviewed Jul 16, 2021

View reviewed changes

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

Send back a warning when we receive an invalid shutdown script

e317405

t-bast approved these changes Jul 16, 2021

View reviewed changes

sstone merged commit 01b4073 into master Jul 16, 2021

sstone deleted the implement-upfront-shutdown-script branch July 16, 2021 15:27

Conversation

sstone commented Jun 23, 2021

Uh oh!

codecov-commenter commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sstone commented Jun 29, 2021

Uh oh!

sstone commented Jun 29, 2021

Uh oh!

t-bast commented Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sstone commented Jul 8, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t-bast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

t-bast left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Jun 24, 2021 •

edited

Loading

t-bast commented Jun 29, 2021 •

edited

Loading