send warning when we receive a old commitment transaction

[vincenzopalazzo]

During a channel_reestablish now we send a warning message when we receive an old commitment transaction.

Fixes #1207 with the following consideration lightning/bolts#934 (comment)

[codecov-commenter]

Codecov Report

Merging #1430 (e6300da) into main (171dfee) will increase coverage by 0.46%.
The diff coverage is 88.57%.

@@            Coverage Diff             @@
##             main    #1430      +/-   ##
==========================================
+ Coverage   90.89%   91.36%   +0.46%     
==========================================
  Files          75       75              
  Lines       41657    43716    +2059     
  Branches    41657    43716    +2059     
==========================================
+ Hits        37866    39941    +2075     
+ Misses       3791     3775      -16     

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	92.57% <75.00%> (+4.18%)	⬆️
lightning/src/ln/functional_tests.rs	97.40% <92.59%> (+0.22%)	⬆️
lightning/src/ln/reorg_tests.rs	100.00% <0.00%> (ø)
lightning/src/util/config.rs	48.35% <0.00%> (+3.45%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 171dfee...e6300da. Read the comment docs.

[vincenzopalazzo]

OK! I tried to make a little bit of refactoring and applied the changes that you suggested.

The only think that is unclear to me right now, is this case

https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/ln/channel.rs#L3708-L3712

Why in this case there is a CloseDelayBroadcast why does mean? i was not able to find some comments about the meaning, and I didn't check if the spec has some info about this case!

[TheBlueMatt]

Sorry for the delay in reviewing this!

[vincenzopalazzo]

Rebased on master, I reorganized the test in a more clean way where the different cases are executed at the end of the function. In this way, it is cleaner to the reader what we are trying to do!

[TheBlueMatt]

Awesome! One note - we don't like having intermediate commits that fail tests. We don't test for it in CI (only that it compiles), but if at all possible try to make sure each individual commit passes tests as well, ensuring we can git bisect easily.

[vincenzopalazzo]

we don't like having intermediate commits that fail tests. We don't test for it in CI (only that it compiles), but if at all possible try to make sure each individual commit passes tests as well, ensuring we can git bisect easily.

Sorry! I rebased and squash the commit in a single one!

[TheBlueMatt]

Basically looks good, one comment on the second branch of the test.

[TheBlueMatt]

LGTM!

[TheBlueMatt]

Hmm, was there a conflict? In general please avoid rebase'ing unless there's a conflict with the current upstream git.

[TheBlueMatt]

Basically LGTM, lets get another reviewer.

[vincenzopalazzo]

I clean up the code and remove the enum. thanks, @valentinewallace to point me on this, and also for the grammar mistake!

I also rebased on top of the last master!

[TheBlueMatt]

I also rebased on top of the last master!

Generally please avoid this unless there is a conflict. Rebases make it harder to diff-tree and see changes to a PR, range-diff can be a bit more finicky and hard to read.

[vincenzopalazzo]

Ops! sorry @TheBlueMatt