Skip to content

Upfront shutdown script#1286

Closed
t-bast wants to merge 3 commits into
masterfrom
upfront-shutdown-script
Closed

Upfront shutdown script#1286
t-bast wants to merge 3 commits into
masterfrom
upfront-shutdown-script

Conversation

@t-bast
Copy link
Copy Markdown
Member

@t-bast t-bast commented Jan 21, 2020

Extending all messages with a TLV stream (spec PR here) requires making currently optional data to become mandatory.

We don't support upfront_shutdown_script yet which is going to be a problem if we want to extend open_channel and accept_channel with additional TLVs (which we already do for Phoenix).

This PR implements partial support for upfront_shutdown_script: we allow our peers to use that feature, but there's no hooks to use it ourselves. If our users want us to add support for specifying an upfront_shutdown_script, we can add it later.

Add to features list.
Activate optional bit by default.
Encode/decode upfront_shutdown_script in open_channel and accept_channel
messages.

Decode both old and new format, always encode with the new one.
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 21, 2020

Codecov Report

Merging #1286 into master will increase coverage by 0.28%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1286      +/-   ##
==========================================
+ Coverage   77.16%   77.45%   +0.28%     
==========================================
  Files         143      144       +1     
  Lines        9942    10059     +117     
  Branches      394      420      +26     
==========================================
+ Hits         7672     7791     +119     
+ Misses       2270     2268       -2
Impacted Files Coverage Δ
...a/fr/acinq/eclair/wire/LightningMessageTypes.scala 86.36% <ø> (ø) ⬆️
.../fr/acinq/eclair/wire/LightningMessageCodecs.scala 100% <100%> (ø) ⬆️
.../src/main/scala/fr/acinq/eclair/wire/OpenTlv.scala 100% <100%> (ø)
...core/src/main/scala/fr/acinq/eclair/Features.scala 100% <100%> (ø) ⬆️
...cinq/eclair/blockchain/bitcoind/zmq/ZMQActor.scala 89.74% <0%> (-5.13%) ⬇️
...la/fr/acinq/eclair/payment/relay/NodeRelayer.scala 97.05% <0%> (-0.54%) ⬇️
...nq/eclair/blockchain/electrum/ElectrumWallet.scala 80.75% <0%> (-0.25%) ⬇️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 83.47% <0%> (-0.1%) ⬇️
...r/acinq/eclair/payment/send/PaymentInitiator.scala 97.1% <0%> (-0.05%) ⬇️
...nq/eclair/blockchain/electrum/ElectrumClient.scala 72.92% <0%> (ø) ⬆️
... and 6 more

("yourLastPerCommitmentSecret" | optional(bitsRemaining, privateKey)) ::
("myCurrentPerCommitmentPoint" | optional(bitsRemaining, publicKey))).as[ChannelReestablish]

// Legacy nodes are supposed to encode an upfront_shutdown_script only if both sides advertised support for option_upfront_shutdown_script.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that a node may include an optional shutdown script even if its peer does not support it:

  • if both nodes advertised the option_upfront_shutdown_script feature:
  • MUST include either a valid shutdown_scriptpubkey as required by shutdown >scriptpubkey, or a zero-length shutdown_scriptpubkey.
  • otherwise:
  • MAY include ashutdown_scriptpubkey.

@sstone
Copy link
Copy Markdown
Member

sstone commented Jan 23, 2020

I think there is something missing: if both peers support option_upfront_shutdown_script then we must include one (at least an empty one) ?

@t-bast t-bast closed this Jan 23, 2020
@t-bast t-bast deleted the upfront-shutdown-script branch January 23, 2020 15:43
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