Synchronized Spend and Epoch Notification Cancellation

[cfromknecht]

Makes cancellation of neutrino and btcd notifications block until notification dispatcher has processed the cancellation request. This commit also unifies the logic across all cancellation handling in regards to formatting and documentation.

The issue is primarily created because of a quirk in go, allowing buffered channels to be read after they have been closed. The solution is to ensure the notification channels are properly drained and closed before yielding to the caller of Cancel. By doing so, we can ensure that the channel is both empty and closed before allowing the caller to attempt a read on the buffered channel. Under this proposal, we can safely remove the use of done channels used to synchronize epoch cancellation since draining the channel is sufficient to ensure that the cancellation has been processed by the notification dispatcher and that the client is guaranteed to see a closed channel, as intended.

Example Errors

    --- FAIL: TestInterfaces/btcd:_cancel_spend_ntfn: spend ntfn should've been cancelled
    --- FAIL: TestInterfaces/btcd:_cancel_epoch_ntfn: epoch ntfn should've been cancelled
    --- FAIL: TestInterfaces/neutrino:_cancel_spend_ntfn: spend ntfn should've been cancelled
    --- FAIL: TestInterfaces/neutrino:_cancel_epoch_ntfn: epoch ntfn should've been cancelled

Blocking Drain of Epoch Event Channel

&chainntnfs.BlockEpochEvent{
    Epochs: registration.epochChan,
    Cancel: func() {
        cancel := &epochCancel{
            epochID: registration.epochID,
        }

        // Submit epoch cancellation to notification dispatcher.
        select {
        case b.notificationCancels <- cancel:
            // Cancellation is being handled, drain the epoch channel until it is
            // closed before yielding to caller.
            for {
                select {
                case _, ok := <-registration.epochChan:
                    if !ok {
                        return
                    }
                case <-b.quit:
                }
            }
            case <-b.quit:
        },
    }
}

[mention-bot]

@cfromknecht, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Roasbeef, @AndrewSamokhvalov and @bryanvu to be potential reviewers.

[coveralls]

Coverage decreased (-0.06%) to 73.815% when pulling ffb9d3b on cfromknecht:sync-ntfn-cancel into 78f6caf on lightningnetwork:master.

[coveralls]

Coverage remained the same at 73.87% when pulling c886b2d on cfromknecht:sync-ntfn-cancel into 78f6caf on lightningnetwork:master.

[coveralls]

Coverage decreased (-0.05%) to 73.816% when pulling c4cf854 on cfromknecht:sync-ntfn-cancel into 78f6caf on lightningnetwork:master.

[coveralls]

Coverage decreased (-0.008%) to 73.862% when pulling e1fc54e on cfromknecht:sync-ntfn-cancel into 78f6caf on lightningnetwork:master.

[Roasbeef]

Nice approach in ensuring that the clients will never read from a closed channel! The new cancellation scheme results in very tight synchronization between the caller and the ChainNotifier instance. I've verified locally that this addresses the flakes referenced in the PR's body.

LGTM 👾