breacharbiter: reuse spend events instead of re-registering

This commit fixes a broadcast loop within the breach arbiter, that would
occur when HTLC outputs had been taken to the second level. The breach
arbiter would register for a spend event, but would immediately go on to
create and publish the justice, without waiting for the response to be
received on the spend channel. This lead to a race, where the outpoint
could actually already have been spent, but the notification would
arrive after the breach arbiter checked the channel, and publishing the
justice TX would fail because it was a double spend. This would create a
"broadcast loop", as seen in the logs from the integration test
revoked_uncooperative_close_retribution_remote_hodl.

This is fixed by reusing an existing spend event for the outputs,
meaning we will actually receive on the first channel we initiated,
making the broadcast loop more likely to terminate.
This commit is contained in:
Johan T. Halseth 2018-03-23 13:36:57 +01:00
parent daf542cf89
commit 2fa2b545fc
No known key found for this signature in database
GPG Key ID: 15BAADA29DA20D26

@ -490,6 +490,8 @@ func (b *breachArbiter) exactRetribution(confChan *chainntnfs.ConfirmationEvent,
// construct a sweep transaction and write it to disk. This will allow
// the breach arbiter to re-register for notifications for the justice
// txid.
spendNtfns := make(map[wire.OutPoint]*chainntnfs.SpendEvent)
secondLevelCheck:
if finalTx == nil {
// Before we create the justice tx, we need to check to see if
@ -510,24 +512,32 @@ secondLevelCheck:
breachedOutput.outpoint, breachInfo.chanPoint)
// Now that we have an HTLC output, we'll quickly check
// to see if it has been spent or not.
spendNtfn, err := b.cfg.Notifier.RegisterSpendNtfn(
&breachedOutput.outpoint, breachInfo.breachHeight,
)
if err != nil {
brarLog.Errorf("unable to check for spentness "+
"of out_point=%v: %v",
breachedOutput.outpoint, err)
// to see if it has been spent or not. If we have
// already registered for a notification for this
// output, we'll reuse it.
spendNtfn, ok := spendNtfns[breachedOutput.outpoint]
if !ok {
spendNtfn, err = b.cfg.Notifier.RegisterSpendNtfn(
&breachedOutput.outpoint,
breachInfo.breachHeight,
)
if err != nil {
brarLog.Errorf("unable to check for "+
"spentness of out_point=%v: %v",
breachedOutput.outpoint, err)
// Registration may have failed if we've been
// instructed to shutdown. If so, return here to
// avoid entering an infinite loop.
select {
case <-b.quit:
return
default:
continue
// Registration may have failed if
// we've been instructed to shutdown.
// If so, return here to avoid entering
// an infinite loop.
select {
case <-b.quit:
return
default:
continue
}
}
spendNtfns[breachedOutput.outpoint] = spendNtfn
}
select {
@ -536,6 +546,7 @@ secondLevelCheck:
if !ok {
return
}
delete(spendNtfns, breachedOutput.outpoint)
// In this case we'll morph our initial revoke
// spend to instead point to the second level