Allow multiple calls to monitor_update_failed

[valentinewallace]

There's a few open TODOs/questions.

Found by the fuzzer. Repro script:

export TARGET="chanmon_consistency" # adjust for your output
export HEX="04000000ffffffffffffffffffff00000006ffff6200000000000011000c 0021181800000011000c002118181c181818211720000000343201000000 00001000011b181821172000001f5a32010021172000001f5a3201000000 00000000011b6b00450000200a2000001d0000ff00002fca000000000100 0000000000200000" # adjust for your output

mkdir -p ./test_cases/$TARGET
echo $HEX | xxd -r -p > ./test_cases/$TARGET/any_filename_works

export RUST_BACKTRACE=full
export RUSTFLAGS="--cfg=fuzzing"
cargo test -- --nocapture &> output.txt

[codecov]

Codecov Report

Merging #1066 (3d77cc7) into main (24065c8) will increase coverage by 0.01%.
The diff coverage is 94.93%.

@@            Coverage Diff             @@
##             main    #1066      +/-   ##
==========================================
+ Coverage   90.98%   90.99%   +0.01%     
==========================================
  Files          65       65              
  Lines       33695    33767      +72     
==========================================
+ Hits        30658    30727      +69     
- Misses       3037     3040       +3     

Impacted Files	Coverage Δ
lightning/src/ln/chanmon_update_fail_tests.rs	97.75% <94.66%> (-0.14%)	⬇️
lightning/src/ln/channel.rs	88.65% <100.00%> (-0.02%)	⬇️
lightning/src/ln/functional_tests.rs	97.42% <0.00%> (-0.02%)	⬇️
lightning/src/ln/channelmanager.rs	85.96% <0.00%> (+0.06%)	⬆️

---

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 24065c8...3d77cc7. Read the comment docs.

[TheBlueMatt]

Nice and simple! We also need to be careful at all the sites we update resend_order to make sure we don't change the order we need to resend in.

[TheBlueMatt]

Minor test comments, but otherwise ACK. I gave honggfuzz 25 million iterations with the previously-failing seed and it hasn't found any new crashes yet so we'll call it a separate issue if it finds any :).

[TheBlueMatt]

Diff (after rebase - dunno why this was rebased?) from last review:

$ git diff-tree -U1 HEAD upstream/pull/1066
diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs
index 9b2fad3e2..9a5d2eaa9 100644
--- a/lightning/src/ln/chanmon_update_fail_tests.rs
+++ b/lightning/src/ln/chanmon_update_fail_tests.rs
@@ -2053,3 +2053,3 @@ fn test_path_paused_mpp() {
 
-	claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, false, payment_preimage);
+	claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, payment_preimage);
 }
diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs
index 6ebd7fdcf..6824fb043 100644
--- a/lightning/src/ln/functional_test_utils.rs
+++ b/lightning/src/ln/functional_test_utils.rs
@@ -1159,3 +1159,3 @@ pub fn send_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, route: Route
 
-pub fn claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, skip_initial_claim: bool, our_payment_preimage: PaymentPreimage) {
+pub fn claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths: &[&[&Node<'a, 'b, 'c>]], skip_last: bool, our_payment_preimage: PaymentPreimage) {
 	for path in expected_paths.iter() {
@@ -1163,6 +1163,4 @@ pub fn claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, exp
 	}
-	if !skip_initial_claim {
-		assert!(expected_paths[0].last().unwrap().node.claim_funds(our_payment_preimage));
-		check_added_monitors!(expected_paths[0].last().unwrap(), expected_paths.len());
-	}
+	assert!(expected_paths[0].last().unwrap().node.claim_funds(our_payment_preimage));
+	check_added_monitors!(expected_paths[0].last().unwrap(), expected_paths.len());
 
@@ -1252,3 +1250,3 @@ pub fn claim_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, exp
 pub fn claim_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], our_payment_preimage: PaymentPreimage) {
-	claim_payment_along_route(origin_node, &[expected_route], false, false, our_payment_preimage);
+	claim_payment_along_route(origin_node, &[expected_route], false, our_payment_preimage);
 }
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index fc52c5363..e1a57cd7c 100644
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@ -3309,3 +3309,3 @@ fn test_simple_peer_disconnect() {
 
-	claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[2]]], true, false, payment_preimage_3);
+	claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[2]]], true, payment_preimage_3);
 	fail_payment_along_route(&nodes[0], &[&nodes[1], &nodes[2]], true, payment_hash_5);
@@ -8051,3 +8051,3 @@ fn test_simple_mpp() {
 	send_along_route_with_secret(&nodes[0], route, &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], 200_000, payment_hash, payment_secret);
-	claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, false, payment_preimage);
+	claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, payment_preimage);
 }
$

[TheBlueMatt]

Will take after CI.