renepay bug fix: list previous sendpays

- previous pending sendpays must add up so that the plugin tries to pay
the rest of the amount,
- avoid groupid, partid collisions,
- add shadow fees if the option is set and the payment amount - total
  delivering = 0
- add a test,
- also fix a buggy shadow routing test

plugins/renepay/pay.c

@@ -424,7 +424,7 @@ const char *try_paying(const tal_t *ctx,
 			       payment,
 			       remaining, feebudget,
 			       /* is entire payment? */
-			       amount_msat_eq(payment->total_delivering, AMOUNT_MSAT(0)),
+			       amount_msat_eq(remaining, AMOUNT_MSAT(0)),
 			       ecode);
 	gossmap_remove_localmods(pay_plugin->gossmap, payment->local_gossmods);
 
@@ -708,7 +708,7 @@ payment_listsendpays_previous(
 
 		/* If we decide to create a new group, we base it on max_group_id */
 		if (groupid > max_group_id)
-			max_group_id = 1;
+			max_group_id = groupid;
 
 		/* status could be completed, pending or failed */
 		if (streq(status, "complete")) {
@@ -739,6 +739,15 @@ payment_listsendpays_previous(
 			pending_group_id = groupid;
 			if (partid > max_pending_partid)
 				max_pending_partid = partid;
+
+			if (!amount_msat_add(&pending_msat, pending_msat,
+					     this_msat) ||
+			    !amount_msat_add(&pending_sent, pending_sent,
+					     this_sent))
+				plugin_err(pay_plugin->plugin,
+					   "%s (line %d) msat overflow.",
+					   __PRETTY_FUNCTION__, __LINE__);
+
 		} else
 			assert(streq(status, "failed"));
 	}

plugins/renepay/pay_flow.c

@@ -581,6 +581,8 @@ const char *add_payflows(const tal_t *ctx, struct payment *p,
 
 		/* This can adjust amounts and final cltv for each flow,
 		 * to make it look like it's going elsewhere */
+		// FIXME adding shadow fees after flows_fit_amount could mean
+		// that we end up again with over-commitments
 		const u32 *final_cltvs = shadow_additions(
 		    tmpctx, pay_plugin->gossmap, p, flows, is_entire_payment);
 

tests/test_renepay.py

@@ -7,6 +7,8 @@
 import json
 import subprocess
 
+random.seed(334)
+
 
 def test_simple(node_factory):
     '''Testing simply paying a peer.'''
@@ -32,26 +34,26 @@ def test_direction_matters(node_factory):
     assert details['destination'] == l3.info['id']
 
 
-def test_shadow(node_factory):
-    '''Make sure we shadow correctly.'''
-    l1, l2, l3 = node_factory.line_graph(3,
-                                         wait_for_announce=True,
-                                         opts=[{},
-                                               {'fee-base': 20, 'fee-per-satoshi': 20, 'cltv-delta': 20},
-                                               {'fee-base': 30, 'fee-per-satoshi': 30, 'cltv-delta': 30}])
+def test_shadow_routing(node_factory):
+    """
+    Test the value randomization through shadow routing
 
-    # Shadow doesn't always happen (50% chance)!
-    for i in range(20):
-        inv = l2.rpc.invoice(123000, f'test_renepay{i}', 'description')['bolt11']
-        details = l1.rpc.call('renepay', {'invstring': inv})
-        assert details['status'] == 'complete'
-        assert details['amount_msat'] == Millisatoshi(123000)
-        assert details['destination'] == l2.info['id']
+    Note there is a very low (0.5**10) probability that it fails.
+    """
+    # We need l3 for random walk
+    l1, l2, l3 = node_factory.line_graph(3, wait_for_announce=True)
 
-        line = l1.daemon.wait_for_log("No MPP, so added .*msat shadow fee")
-        if 'added 0msat' not in line:
-            break
-        assert i != 19
+    amount = 10000
+    total_amount = 0
+    n_payments = 10
+    for i in range(n_payments):
+        inv = l3.rpc.invoice(amount, "{}".format(i), "test")["bolt11"]
+        total_amount += l1.rpc.call('renepay',
+            {'invstring': inv, 'dev_use_shadow': True})["amount_sent_msat"]
+
+    assert total_amount > n_payments * amount
+    # Test that the added amount isn't absurd
+    assert total_amount < int((n_payments * amount) * (1 + 0.01))
 
 
 def test_mpp(node_factory):
@@ -430,3 +432,83 @@ def test_htlc_max(node_factory):
     l1.wait_for_htlcs()
     invoice = only_one(l6.rpc.listinvoices('inv')['invoices'])
     assert invoice['amount_received_msat'] >= Millisatoshi('1800000sat')
+
+
+def test_previous_sendpays(node_factory, bitcoind):
+    '''
+    Check that renepay can complete a payment that already started
+    '''
+    opts = [
+        {'disable-mpp': None, 'fee-base': 1000, 'fee-per-satoshi': 1000},
+    ]
+    l1, l2, l3, l4 = node_factory.line_graph(4,
+                                         wait_for_announce = True,
+                                         opts=opts*4)
+
+    ### First case, do not overpay a pending MPP payment ###
+    invstr = l3.rpc.invoice("100000sat", 'inv1', 'description')['bolt11']
+    inv = l1.rpc.decode(invstr)
+    route = l1.rpc.call('getroute',
+        {'id': inv['payee'],
+        'amount_msat':'50000sat',
+        'riskfactor': 10})
+    # we start a MPP payment
+    sendpay = l1.rpc.call('sendpay',
+        {'route': route['route'],
+        'payment_hash': inv['payment_hash'],
+        'payment_secret': inv['payment_secret'],
+        'amount_msat': '100000sat',
+        'groupid': 1,
+        'partid': 1})
+    # while it is pending, we try to complete it with renepay
+    details = l1.rpc.call('renepay', {'invstring': invstr})
+    invoice = only_one(l3.rpc.listinvoices('inv1')['invoices'])
+    # the receive amount should be exact
+    assert invoice['amount_received_msat'] == Millisatoshi('100000sat')
+
+    ### Second case, do not collide with failed sendpays ###
+    invstr = l3.rpc.invoice("100000sat", 'inv2', 'description')['bolt11']
+    inv = l1.rpc.decode(invstr)
+    route = l1.rpc.call('getroute',
+        {'id': inv['payee'],
+        'amount_msat':'50000sat',
+        'riskfactor': 10})
+
+    # load a plugin that fails all HTLCs
+    l2.rpc.call("plugin",
+        {"subcommand": "start",
+        "plugin": os.path.join(os.getcwd(), "tests/plugins/fail_htlcs.py")})
+    l2.daemon.wait_for_log(r'^(?=.*plugin-manager.*fail_htlcs.py).*')
+
+    # start a MPP payment that will fail at l2
+    sendpay = l1.rpc.call('sendpay',
+        {'route': route['route'],
+        'payment_hash': inv['payment_hash'],
+        'payment_secret': inv['payment_secret'],
+        'amount_msat': '100000sat',
+        'groupid': 1,
+        'partid': 1})
+    l2.daemon.wait_for_log(r'Failing htlc on purpose')
+    l1.wait_for_htlcs()
+
+    # another payment that fails
+    sendpay = l1.rpc.call('sendpay',
+        {'route': route['route'],
+        'payment_hash': inv['payment_hash'],
+        'payment_secret': inv['payment_secret'],
+        'amount_msat': '100000sat',
+        'groupid': 2,
+        'partid': 1})
+    l2.daemon.wait_for_log(r'Failing htlc on purpose')
+    l1.wait_for_htlcs()
+
+    # unload the fail_htlcs plugin
+    l2.rpc.call("plugin", {"subcommand": "stop", "plugin": "fail_htlcs.py"})
+    l2.daemon.wait_for_log(r'plugin-fail_htlcs.py: Killing plugin: stopped by lightningd via RPC')
+
+    # now renepay should be able to construct new sendpays that do not collide
+    # with the previously failed sendpays
+    details = l1.rpc.call('renepay',
+        {'invstring': invstr, 'dev_use_shadow': False})
+    invoice = only_one(l3.rpc.listinvoices('inv2')['invoices'])
+    assert invoice['amount_received_msat'] == Millisatoshi('100000sat')