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.

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

Make UpfrontShutdownScript a TLV record #1333

Merged
merged 4 commits into from
Feb 28, 2020
Merged

Make UpfrontShutdownScript a TLV record #1333

merged 4 commits into from
Feb 28, 2020

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Feb 26, 2020

We make some previously optional fields mandatory in ChannelReestablish, OpenChannel and AcceptChannel. This allows us to extend all messages with an optional TLV stream.

See lightning/bolts#714 for more details.

We're moving upfront_shutdown_script to a TLV field: this is compatible at the byte level and provides more flexibility in the future (to either include or exlude it). For now we always include an empty one, for backwards-compatibility.

This allowed removing the ugly hack in the codecs that was made specifically for Phoenix. In the branch we deploy to the node connected to Phoenix wallets, we should simply remove the UpfrontShutdownScript(ByteVector.empy) from the TLV stream and we will be backwards-compatible with older Phoenix.

E2E tests:

  • eclair-v0.3.3
  • lnd v0.9.1-beta.rc1
  • c-lightning v0.8.1

ChannelReestablish contains optional fields that are currently
gated on the DLP/SRK feature bits.

We make them mandatory to allow extending the message with TLVs.
OpenChannel and AcceptChannel contained an optional field for
upfront_shutdown_script.

We make this field mandatory to allow extending the message with TLVs.
We move this field inside a TLV record for future flexibility.
@t-bast t-bast requested review from sstone and pm47 February 26, 2020 13:07
@codecov-io
Copy link

codecov-io commented Feb 28, 2020

Codecov Report

Merging #1333 into master will increase coverage by 0.13%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1333      +/-   ##
==========================================
+ Coverage   77.62%   77.75%   +0.13%     
==========================================
  Files         144      144              
  Lines       10141    10206      +65     
  Branches      406      401       -5     
==========================================
+ Hits         7872     7936      +64     
- Misses       2269     2270       +1
Impacted Files Coverage Δ
...a/fr/acinq/eclair/wire/LightningMessageTypes.scala 86.36% <ø> (ø) ⬆️
.../fr/acinq/eclair/wire/LightningMessageCodecs.scala 100% <100%> (ø) ⬆️
...c/main/scala/fr/acinq/eclair/wire/ChannelTlv.scala 100% <100%> (ø)
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 84.17% <100%> (+0.1%) ⬆️
...q/eclair/blockchain/electrum/ElectrumWatcher.scala 53.6% <0%> (-2.4%) ⬇️
...cala/fr/acinq/eclair/crypto/TransportHandler.scala 90.55% <0%> (-1.12%) ⬇️
...q/eclair/payment/receive/MultiPartPaymentFSM.scala 93.84% <0%> (+1.34%) ⬆️
...cinq/eclair/payment/receive/MultiPartHandler.scala 96.37% <0%> (+1.81%) ⬆️
... and 1 more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants