Tweak and Expand the chanmon_consistency fuzz target #753
Conversation
Codecov Report
@@ Coverage Diff @@
## main #753 +/- ##
==========================================
- Coverage 91.45% 91.38% -0.07%
==========================================
Files 37 37
Lines 22249 22249
==========================================
- Hits 20347 20333 -14
- Misses 1902 1916 +14
Continue to review full report at Codecov.
|
TheBlueMatt
force-pushed
the
2020-11-chanmon_consistency-bits
branch
2 times, most recently
from
95671e2 to
ecb3fb5
Compare
TheBlueMatt
force-pushed
the
2020-11-chanmon_consistency-bits
branch
from
ecb3fb5 to
6626d08
Compare
fuzz/src/chanmon_consistency.rs
Outdated
| match err.as_str() { | ||
| "Peer for first hop currently disconnected/pending monitor update!" => test_return!(), | ||
| _ if err.starts_with("Cannot push more than their max accepted HTLCs ") => test_return!(), | ||
| _ if err.starts_with("Cannot send value that would put us over the max HTLC value in flight our peer will accept ") => test_return!(), | ||
| _ if err.starts_with("Cannot send value that would put our balance under counterparty-announced channel reserve value") => test_return!(), | ||
| _ if err.starts_with("Cannot send value that would overdraw remaining funds.") => test_return!(), | ||
| _ if err.starts_with("Cannot send value that would not leave enough to pay for fees.") => test_return!(), | ||
| _ => panic!(err), | ||
| } |
There was a problem hiding this comment.
Critically, we keep going after hitting an error, as there's no
reason channels should get out of sync even if a send fails.
Doesn't this still result in the test returning if a send fails?
There was a problem hiding this comment.
No - we only return from do_test if either we read a byte we don't know how to process or we run out of bytes. Prior to this we would test_return!() if we max out sending over a channel as well.
There was a problem hiding this comment.
Aye, think it was changed as the quoted code is using test_return! but now is {}. So should be good.
| 0x03 => *monitor_a.update_ret.lock().unwrap() = Ok(()), | ||
| 0x04 => *monitor_b.update_ret.lock().unwrap() = Ok(()), | ||
| 0x05 => *monitor_c.update_ret.lock().unwrap() = Ok(()), | ||
| 0x06 => { | ||
| 0x04 => *monitor_a.update_ret.lock().unwrap() = Ok(()), | ||
| 0x05 => *monitor_b.update_ret.lock().unwrap() = Ok(()), | ||
| 0x06 => *monitor_c.update_ret.lock().unwrap() = Ok(()), | ||
|
|
||
| 0x08 => { |
There was a problem hiding this comment.
Could you help me understand why values like 0x03 and 0x07 are skipped?
There was a problem hiding this comment.
To keep related message groups having the same binary prefix. Honestly it probably doesn't matter, but fuzzers do do some bit-twiddling-based tweaking and I figured no reason to not have bit-twiddling often have similar effects. I noted it in a new comment.
Instead of simply always considering a payment-send failure as acceptable (and aborting fuzzing), we check that a payment send failure is from a list of errors that we know we can hit, mostly around maxing out our channel balance. Critically, we keep going after hitting an error, as there's no reason channels should get out of sync even if a send fails.
This should make it a bit easier for the fuzzer to hit any given balance breakdown during run as well as tweaks the command strings to be more bit-pattern friendly.
In previous versions of related commits, the macros in chanmon_consistency ended up blowing up rustc a bit resulting in 20+GB memory usage and long compile times. Shorter function bodies by avoiding macros where possible fix this.
We should never generate Ignore-action HandleError events anymore
This adds a new command string in the chanmon_consistency fuzzer which tests that, once all pending HTLCs are settled, at least one side of a channel can still send funds. While this should have caught the recent(ish) spec bug where channels could get stuck, I did not attempt to reproduce said bug with this patch.
TheBlueMatt
force-pushed
the
2020-11-chanmon_consistency-bits
branch
from
6626d08 to
6563f7a
Compare
|
Gonna take so that we can talk about/analyze things like #757 against git latest instead of against some random branch. |
I spent some time with chanmon_consistency to expand the errors it could catch. Luckily no bugs turned up (yet) from this work.