Fix premature claims broadcast

[morehouse]

A claim transaction with locktime T can only be mined at block heights of T+1 or above. Due to an off-by-one bug, we were broadcasting some claim transactions one block before they could actually be mined.

AFAICT, nothing bad resulted from this bug -- later rebroadcasts of the transaction would eventually succeed once the correct height was reached.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.69%. Comparing base (ddeaab6) to head (463ba15).
Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #3453    +/-   ##
========================================
  Coverage   89.69%   89.69%            
========================================
  Files         130      130            
  Lines      107472   107615   +143     
  Branches   107472   107615   +143     
========================================
+ Hits        96396    96526   +130     
- Misses       8674     8684    +10     
- Partials     2402     2405     +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[arik-so]

I think some unit tests would be really good here. Additionally, a transaction with locktime x can be broadcast as soon as block x is mined. The first block it can be mined in may be x+1, but that is impertinent to the broadcast height.

[morehouse]

I think some unit tests would be really good here.\

Added a test.

Additionally, a transaction with locktime x can be broadcast as soon as block x is mined. The first block it can be mined in may be x+1, but that is impertinent to the broadcast height.

Yep, the existing code broadcasts at x-1. After this commit, it correctly broadcasts after x is mined.

[arik-so]

I misinterpreted your comment, because your code actually looked right :)

[morehouse]

Rebased and updated commit message to be clearer.

[arik-so]

Incredible, thanks so much for the test! If I may ask for one more addition within the test – can you make it fail for if package_locktime > cur_height - 1? We can obviously broadcast after the locktime, but the test specifically should ensure that as soon as a broadcast is possible, it happens.

[morehouse]

Not sure I understand you... I think the test already does that.

It has 3 HTLC-Timeouts with locktimes of 0, 1, and 2. The test submits the claim requests at height 1 and then verifies that the HTLC-Timeouts with locktimes of 0 and 1 are broadcast immediately while the last HTLC is not. It then mines 1 block and verifies that the last HTLC-Timeout with locktime 2 is broadcast immediately at height 2.

[arik-so]

I actually just meant that when modifying the line to package_locktime > cur_height - 1, the unit test still passes, but that's because the method has a self-correcting mechanism below:

// Claim everything up to and including `cur_height`.
		let remaining_locked_packages = self.locktimed_packages.split_off(&(cur_height + 1));

[morehouse]

I actually just meant that when modifying the line to package_locktime > cur_height - 1, the unit test still passes, but that's because the method has a self-correcting mechanism below:

// Claim everything up to and including `cur_height`.
		let remaining_locked_packages = self.locktimed_packages.split_off(&(cur_height + 1));

Right. If you modify the line as you suggested and also modify this line to

let remaining_locked_packages = self.locktimed_packages.split_off(&(cur_height));

then the test will fail because the HTLC-Timeout with locktime of 1 will not be broadcast until height 2.

So the test seems to catch this mutation already...

[arik-so]

Exactly. So everything looks good to me, but I'd ask to wait for another reviewer to approve this, too, before merging.

[arik-so]

Though it does make me wonder why we have these two code paths for the same thing at all.

[morehouse]

Though it does make me wonder why we have these two code paths for the same thing at all.

The paths can likely be merged. From my observation there are many such opportunities to simplify and improve readability within the chain module. IMO a cleanup would be worthwhile for this security-critical code.

[TheBlueMatt]

Thanks! Indeed this code could be cleaned up a good bit...we did a tiny bit of it with the changes to improve package merging but a lot of this stuff originated when Antoine refactored some straightline code to make it more OOP. Its really old code and could use more love than it gets...always tricky to touch it much, though, of course.