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

Do not include optional timestamp/checksum TLV if they are empty #1280

Closed
wants to merge 1 commit into from

Conversation

sstone
Copy link
Member

@sstone sstone commented Jan 16, 2020

This change fixes an interop issue with c-lighting, which does not handle reply_channel_range messages that include an option timestamps TLV that contains an empty array of timestamps with a ZLIB encodingand fails with aconnection-level error, failing all channels! reason=reply_channel_range 0 timestamps when 0 scids?` error

It is related to lightning/bolts#729

@sstone
Copy link
Member Author

sstone commented Jan 16, 2020

I'll make it ready for review as soon as I've added proper tests

@sstone sstone requested review from pm47 and t-bast January 16, 2020 11:11
@codecov-io
Copy link

codecov-io commented Jan 16, 2020

Codecov Report

Merging #1280 into master will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1280      +/-   ##
==========================================
+ Coverage   77.11%   77.17%   +0.06%     
==========================================
  Files         142      142              
  Lines        9934     9934              
  Branches      417      417              
==========================================
+ Hits         7661     7667       +6     
+ Misses       2273     2267       -6
Impacted Files Coverage Δ
...src/main/scala/fr/acinq/eclair/router/Router.scala 92.29% <ø> (-0.69%) ⬇️
...q/eclair/blockchain/electrum/ElectrumWatcher.scala 50.42% <0%> (-1.71%) ⬇️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 83.65% <0%> (+0.55%) ⬆️
...clair/blockchain/electrum/ElectrumClientPool.scala 78.49% <0%> (+4.3%) ⬆️
...cinq/eclair/blockchain/bitcoind/zmq/ZMQActor.scala 94.87% <0%> (+5.12%) ⬆️

@sstone
Copy link
Member Author

sstone commented Jan 16, 2020

@sstone sstone marked this pull request as ready for review January 16, 2020 15:12
@sstone sstone changed the title Do not include optional tmestamp/checksum TLV if they are empty Do not include optional timestamp/checksum TLV if they are empty Jan 16, 2020
t-bast
t-bast previously approved these changes Jan 16, 2020
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

Don't include optional timestmap or checksum TLVs if they are empty (i.e. there are no scids in the current block range)
@pm47
Copy link
Member

pm47 commented Jan 20, 2020

I think previous implementation made more sense. How can I tell now that my peer understood that I requested additional data, but just doesn't have any?

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.

4 participants