In this commit, we fix a lingering bug related to the way that we
deliver block epoch notifications to end users. Before this commit, we
would launch a new goroutine for *each block*. This was done in order
to ensure that the notification dispatch wouldn’t block the main
goroutine that was dispatching the notifications. This method archived
the goal, but had a nasty side effect that the goroutines could be
re-ordered during scheduling, meaning that in the case of fast
successive blocks, then notifications would be delivered out of order.
Receiving out of order notifications is either disallowed, or can cause
sub-systems that rely on these notifications to get into weird states.
In order to fix this issue, we’ll no longer launch a new goroutine to
deliver each notification to an awaiting client. Instead, each client
will now gain a concurrent in-order queue for notification delivery.
Due to the internal design of chainntnfs.ConcurrentQueue, the caller
should never block, yet the receivers will receive notifications in
order. This change solves the re-ordering issue and also minimizes the
number of goroutines that we’ll create in order to deliver block epoch
notifications.
In this commit, we fix a race condition related to the way we attempt
to query to see if an outpoint has already been spent by the time it’s
registered within the ChainNotifier. If the transaction creating the
outpoint hasn’t made it into the mempool by the time we execute the
GetTxOut call, then we’ll attempt to query for the transaction itself.
In this case, if we query for the transaction, then the block hash
field will be empty as it hasn’t yet made it into a block. Under the
previous logic, we’d then attempt to force a rescan. This is an issue
as the forced rescan will fail since it’ll try to fetch the block hash
of all zeroes.
In this commit, we fix this issue by only entering this “fallback to
rescan” logic iff, the transaction has actually been mined.
In the historical dispatch of btcdnotify, the dispatcher checks if a
transaction has been included in a block. If this check happens before
the notifier has processed the update, it's possible that the
currentHeight of the notifier and the currentHeight of the chain might
be out of sync which causes an off by one error when calculating a
target height for the transaction confirmation. This change uses the
height of the block the transaction was found in, rather than the
currentHeight that's known by the notifier to eliminate this.
This race condition can occur if a transaction is included in a block
right when a notification is being added to the notifier for it AND when
the confirmation requires > 1 confirmations. In this case, the
confirmation gets added to the confirmation heap twice.
This commit fixes a prior bug in the logic for registering a new spend
notification. Previously, if the transaction wasn’t found in the
mempool or already confirmed within the chain, then
GetRawTransactionVerbose would return an error which would cause the
function itself to exit with an error.
This issue would then cause the server to be unable to start up as the
breach arbiter would be unable to register for spend notifications for
all the channels that it needed to be watching.
We fix this error simply by recognizing the particular JSON-RPC error
that will be returned in this scenario and treating it as a benign
error.
This commit fixes a prior mishandled error when attempting historical
confirmation dispatches. In the prior version of this code fragment, if
the transaction under the spotlight wasn’t found within the mempool, or
already in the chain, then an error would be returned by
b.chainConn.GetRawTransactionVerbose, which would case the function to
exit with an error. This behavior was incorrect, as during transaction
re-broadcasts, it was possible for transaction not yet to be a member
of either set.
We fix this issue by ensuring that we treat the JSON error code as a
benign error and continue with the notification registration.
This commit fixes to distinct bugs in the way we previously dipatched
notifications for transactions which needed a historical dispatch.
Previously we would compare transactions when scanning the block using
the `tx.Hash` field. This was incorrect has the `Hash` field is
actually the wtxid, not the txid which should be the item being
compared. We fix this within the second bug fix by actually using the
txid to find the proper transaction.
The second fix has to due with a slight race condition which led to an
off-by-one error when dispatching the historical confirmation. If while
we were dispatching the confirmation, a new block was found, then we
could calculate the wrong block height (off by one) as we were using
the ‘currentHeight’ instead the height of the block which included the
transaction.
This commit modifies two of the main methods in the ChainNotifier
interface to be more light client friendly. In order to do so, we now
tack on an extra parameter to the methods: heightHint. This value
represents the earliest known height that the chain should be scanned
when attempting to do a dispatch from historical data.
All tests have also been updated to use these new parameters properly
when excising the expected behavior of each interface implementation.
This commit modifies the btcdnotify implementation of the ChainNotifier
interface to properly include the height in which the watched output
was spent in the SpendDetail sent as a notification.
The set of tests have also been updated to assert that the proper
spending height is included in received notification.
This commit fixes a race condition that was uncovered by the race
condition detector surrounding cancelling active block epoch
notifications. Previously we would close the main notification channel
for each client, at tine this would cause a read/write race condition
if an active grouting was attempting to dispatch a notification. We now
fix this use by using a distinct channel for signaling cancellation to
the active grouting, and another to signal cancellation to any
notification observers.
This commit is meant to fix an occasional flake in the interrogation
tests cause by the async nature of the cancellation of block epoch
notifications. This commit modifies the cancellation to now be fully
synchronous which should eliminate this flake.
This commit fixes a race condition that was introduced while fixing a
lingering bug in the logic to notify block epoch clients. The race
condition would happen as by removing the default case in the select
statement, it was now possible for the client’s block epoch client to
be closed while the routine was attempting a send on it.
We now eliminate this race condition possibility by adding a wait group
to all goroutines launched to dispatch a block epoch notification. With
this modification, the Stop() goroutine will now wait for all other
goroutine to exit before closing the block epoch channels of all
currently registered clients.
This commit a bug introduced in the chain notifier while we were
limiting the usage of mutexes within the package. In a prior commit a
default case was introduced in the select statement in order to avoid
the possibility of the main goroutine blocking when dispatching block
epoch notification.
In order to avoid this potentially disastrous bug, we now instead
launch a new goroutine for each client to ensure that all notifications
are reliably dispatched.
This commit fixes a race condition in the notifyBlockEpochs detected by
the race condition detector. Previously the notifyBlockEpochs function
could cause a race condition when a new caller was either cancelling an
existing notification intent or creating a new one.
We fix this issue by making the call to notifyBlockEpochs synchronous
rather than asynchronous. An alternative would be to add a mutex
guarding the map state. The channel itself is buffered with a good
margin, so there shouldn’t be a huge impact.
This commit minifies the BtcdNotifier concrete implementation of the
ChainNotifier interface to allow callers to optionally cancel an
outstanding block epoch or spend notificaiton intent.
To do this efficiently, we now give each notification intent a unique
ID based on if it’s an epoch intent or a spend intent. We then use this
ID to reference back to the original un-dispatched notification intent
when the caller wishes to cancel the intent.
On restarts, notifyBlockEpochs would intermittently attempt to send new
block epoch notifications to clients that had already been shut down,
causing a “send on closed channel” error. This change exits
notifyBlockEpochs upon shutdown so as to prevent this.
This commit makes a large number of minor changes concerning API usage
within the deamon to match the latest version on the upstream btcsuite
libraries.
The major changes are the switch from wire.ShaHash to chainhash.Hash,
and that wire.NewMsgTx() now takes a paramter indicating the version of
the transaction to be created.
Moved transaction states from in-memory maps to persistent BoltDB
buckets. This allows channel force closes to operate reliably if the
daemon is shut down and restarted at any point during the forced
channel closure process.
If the lnd daemon is shut down while multiple subsystems are attempting
to register for notifications, the blocking of those chain notifier
registrations may cause the daemon shutdown to deadlock. The additions
in this commit allow the registration functions to return errors rather
than potentially deadlock when the chain notifier is shut down.
This commit modifies the historical dispatch workflow slightly to also
obtain the full block in which the transaction was confirmed we we can
fully populate the full TxConfirmations struct which was recently added
as part of the confirmation subscription API.
With this change, confirmation triggers that a reached while the demon
is down, will now be deliver exactly as if the trigger was reached
while the daemon was up.
This commit updates the BtcdNotifier implementation of the
ChainNotifier by including the details of a transaction’s confirmation
within the ConfirmationEvent struct sent once a registered txid has
reached a sufficient number of confirmation.
This commit modifies the Stop method of the default ChainNotifier
client, the BtcdNotifier. We now close the notificaiton channel for all
the currently active block epoch clients in order to give clients a
signal that the entire daemon and possibly the ChainNotifier is
shutting down. This gives clients an extra signal to more thoroughly
implement a graceful shutdown across the daemon.
This commit modifies the recently added logic to the ChainNotifier to:
1. Fix the off-by-one confirmation error that was missed due a flaky
test
2. Ensure that partial historical confirmations are properly handled.
The partial hostile confirmation case arises when a transaction already
a non-zero number of confirmations when the notification is registered,
but less than what would trigger the confirmation notification. To fix
this, transaction which have a partial number of confirmation are now
properly inserted into the confHeap, skipping first first phase for
notifications.
Without these checks, “zombie” notification requests that would never
be dispatched could be registered. This would occur if notification
requests were made for events (transaction confirmation and output
spent) that had already been recorded on the blockchain.
This commit adds support for dispatching the same spend notification to
multiple clients. This is now required by the ChainNotiifer interface
documentation and will be needed within the daemon in order to support
some upcoming refactors.
Olaoluwa Osuntokun
Jim Posen
Sam Lewis
Philip Hayes
Conner Fromknecht
Andrey Samokhvalov
Christopher Jämthagen
bryanvu