Fix several more monitor-update-failed cases #288
Conversation
TheBlueMatt
force-pushed
the
2019-01-test-split-raa-flag-fix
branch
from
5e2f689 to
e9434de
Compare
|
Rebased after #284 merge. |
src/ln/chanmon_update_fail_tests.rs
Outdated
|
|
||
| // Queue up two payments - one will be delivered right away, one immediately goes into the | ||
| // holding cell as nodes[0] is AwaitingRAA. Ultimately this allows us to deliver an RAA | ||
| // immediately after a CS, meaning we deliver an RAA while the channel is in the |
There was a problem hiding this comment.
Hmm not super clear, "deliver an RAA from nodes[0] while the channel is in the monitor-update-failed on nodes[1] side which requires a CS response from this one to commit 2nd payment"
There was a problem hiding this comment.
Rewrote it, is it more clear now?
There was a problem hiding this comment.
Honestly, no. I mean I got it by reading the code, not with comment alone. Generally I find comments helpful when they precise at each message sender/receiver, so you get the whole interaction before to read code and then check if it match description. But don't care if you don't fix it go ahead to land monitor stuff
There was a problem hiding this comment.
Cool I rewrote it and added comments for each step. Hopefully that helps.
There was a problem hiding this comment.
Note that I'm moving the log entry additions to a later PR, but I did fix them so the changes will be reflected in that future PR. Also rebased on other PRs that will want to land first (as this is a file rename I'll wait a bit until we land some of the in-flight test PRs first, hopefully) and added another fix commit (probably one or two more coming).
src/ln/chanmon_update_fail_tests.rs
Outdated
|
|
||
| // Queue up two payments - one will be delivered right away, one immediately goes into the | ||
| // holding cell as nodes[0] is AwaitingRAA. Ultimately this allows us to deliver an RAA | ||
| // immediately after a CS, meaning we deliver an RAA while the channel is in the |
There was a problem hiding this comment.
Rewrote it, is it more clear now?
TheBlueMatt
force-pushed
the
2019-01-test-split-raa-flag-fix
branch
from
e9434de to
f99dc05
Compare
TheBlueMatt
changed the title
TheBlueMatt
force-pushed
the
2019-01-test-split-raa-flag-fix
branch
2 times, most recently
from
8e065a1 to
aec76b1
Compare
TheBlueMatt
force-pushed
the
2019-01-test-split-raa-flag-fix
branch
3 times, most recently
from
ac1508f to
1348fa2
Compare
This fixes a rather subtle case handling RAAs when we don't generate a response due to a previous monitor update failure, but would otherwise send a CS response. We need to still set AwaitingRemoteRevoke on the channl in question, but previously did not. Found by chanmon_fail_consistency fuzz test with the failing test converted and added manually.
TheBlueMatt
force-pushed
the
2019-01-test-split-raa-flag-fix
branch
from
1348fa2 to
7dcd15e
Compare
TheBlueMatt
changed the title
TheBlueMatt
force-pushed
the
2019-01-test-split-raa-flag-fix
branch
from
7dcd15e to
658e558
Compare
|
Gonna go ahead and take this since the first round of review was only nits. |
Based on #284 and #286, this splits up functional_tests into functional_test_utils, functional_tests, and chanmon_update_fail_tests. It then fixes one more (really subtle) case found by the new fuzzer I've been working on and adds some big log_trace output to buid_commitment_transaction.