Move fail-backwards up for no to-remote output claims #280
Conversation
TheBlueMatt
force-pushed
the
2018-12-no-to-remote-revoked-htlcs
branch
from
a68005e to
44ca7be
Compare
This fixes HTLC fail-backwards in case we haven't yet sent enough to have a to_remote output to claim, plus some edge cases where it could be removed due to a fee update, though hopefully that goes away with simplified_commitment.
TheBlueMatt
force-pushed
the
2018-12-no-to-remote-revoked-htlcs
branch
from
44ca7be to
5f937a9
Compare
|
Rebased after #278 merge. |
There was a problem hiding this comment.
Good, think I'm the one who forgot to take this case in consideration initially.
You mention edges cases due to fee update, do you test them somewhere ?
And no diff, but there https://github.com/rust-bitcoin/rust-lightning/blob/5f937a986d896ceb1abae4f62686226fae72748d/src/ln/functional_tests.rs#L3052 you speak about 2 monitors but only check_added_monitors! one ?
This tests that we fail back HTLCs that don't make it into commitment transactions the same as we test ones that do.
This tests a case we previously didn't handle correctly where we returned early if there was no to_remote output to claim and thus failed to fail-backwards HTLCs which were present.
TheBlueMatt
force-pushed
the
2018-12-no-to-remote-revoked-htlcs
branch
from
5f937a9 to
913d56e
Compare
|
Fixed the "2 monitors" comment, it was just stale after the async fail-backwards PR. |
|
As for fee update potentially triggering this case, no, they're not tested, I'm not even 100% sure they can, but I also don't really want to add new tests that rely on fee update stuff cause we barely handle that stuff now and hopefully we will remove it wholesale prior to any mainnet uses because of option_simplified_commitment. |
This fixes HTLC fail-backwards in case we haven't yet sent enough
to have a to_remote output to claim, plus some edge cases where it
could be removed due to a fee update, though hopefully that goes
away with simplified_commitment.
Based on #278.