Crash when a payment first succeeds and then fails

[cdecker]

Some days ago we had a situation with eclair, which does not short-circuit multiple payments with the same payment-hash. The first payment succeeded, providing us with the payment-key, and then eclair sent another payment with the same payment-hash. The second one failed, which left us in a state in which we both failed an HTLC and had the secret matching that HTLC.

That then eventually caused us to crash due to htlc_end.c:82. This is an easy way to crash us remotely, so we should probably react in some other way (handing through failure or stealing funds).

[rustyrussell]

Hmm... We need a test case for this: we'll have to stop the node, edit db to remove record of sent payment, then retry.

But they should be separate htlcs, so can't see quite how this happens. Note that spec says you should accept two payments with same hash. Maybe with retransmit of same htlc it can happen somehow?

[ZmnSCPxj]

Currently not on my hacking computer, but would a test much like the below tickle this bug?

def test_pay_sameinvoice(self):
    """Pay the same invoice multiple times, leading to the same rhash being paid twice
    """
    # l2 -> l1 <- l3
    l1 = self.node_factory.get_node()
    l2 = self.node_factory.get_node()
    l3 = self.node_factory.get_node()
    l1.rpc.connect(l2.info['id'], 'localhost', l2.info['port'])
    l1.rpc.connect(l3.info['id'], 'localhost', l3.info['port'])
    self.fund_channel(l2, l1, 10**6)
    self.fund_channel(l3, l1, 10**6)

    inv = l1.rpc.invoice(10**6 / 2, 'label', 'desc')['bolt11']

    l2.rpc.pay(inv)
    self.assertRaises(ValueError, l3.rpc.pay, inv)
    # or just l3.rpc.pay(inv) if we *should* accept two payments with same hash

[ZmnSCPxj]

No, the test above does not tickle the bug. Each struct htlc_in is identified by peer+id, so we need to somehow cause the same peer to pay the same invoice twice.

[cdecker]

Yes, the original instance of this was some sort of broken connection during fulfillment and then a re-attempt which failed. Rusty proposed manually doctoring with the database to make the sender forget the payment_key and then restarting.