In this commit, we fix a recently introduced bug which can result in a
panic when bitcoind nodes without a txindex active are started. The
issue was that we would still defence the transaction's blockhash, which
would be nil if we detected that the backend didn't have the txindex
active.
Before this commit, we relied on the need of full nodes to enable the
transaction index. This allowed us to fetch historical details about
transactions in order to register and dispatch confirmation and spend
notifications.
This commit allows us to drop that requirement by providing a fallback
method to use when the transaction index is not enabled. This fallback
method relies on manually scanning blocks for the transactions
requested, starting from the earliest height the transactions could have
been included in, to the current height in the chain.
This commit extends the test to exercise a scanario that wasn't properly
covered, by registering for a confirmed spend notification for a
historical spend. We also extend the test to make sure it handles buried
spends properly.
This commit fixes a recently introduced bug in the btcdnotifier, where
we would skip all spend clients waiting for a confirmed spend in
txUpdates. The regular case where a spend is included in a new block was
correctly handled in onBlockConnected, but the txUpdates queue is also
used for confirmed spends during rescans, which we would miss. This
commit fixes that by checking if the tx update is confirmed or
unconfirmed, and acts accordingly.
In this commit, we introduce the ability for the different ChainNotifier
implements to send incremental updates to the subscribers of transaction
confirmations. These incremental updates represent how many
confirmations are left for the transaction to be confirmed. They are
sent to the subscriber at every new height of the chain.
In this commit, we avoid storing extra copies of a transaction when
multiple clients register to be notified for the same transaction. We do
this by using a set, which only stores unique elements.
In this commit, we add a new Updates channel to our ConfirmationEvent
struct. This channel will be used to deliver updates to a subscriber of
a confirmation notification. Updates will be delivered at every
incremental height of the chain with the number of confirmations
remaining for the transaction to be considered confirmed by the
subscriber.
This commit changes the chainntnfs tests to adhere to the new
RegisterSpendNtfn signature. It also makes sure that for the test
testSpendNotification, we are only getting notified when a spend is
mined, as previously btcd would notify on mempool inclusion, while
neutrino and bitcoind would notify only on confirmation, and the test
wouldn't catch this.
This commit adds a boolean to RegisterSpendNtfn, giving the caller the
option to only register for notifications on confirmed spends. This is
implemented for the btcd backend using logic similar to what is in used
for Neutrino, paving the way for later unifying them.
This commit moves the call to the bitcoind backend to start watching an
outpoint for spentness to after we have recorded the outpoint in our
list of clients. This is done to avoid a race that we saw using the btcd
backend, and it is probable that it can also happen using bitcoind.
This commit moves the call to the btcd backend to start watching an
outpoint for spentness to after we have recorded the outpoint in our
list of clients. This is done to avoid a race that could occur if btcd
quicly sent a spend notification before we had been able to record it in
our map, essentially losing it.
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 an issue that was recently introduced to the way
we handle historical dispatches for the neutrino notifier. In a recent
change, we no return an error if we’re unable to actually find the
transaction that spends an outpoint. If this is the case, then the
outpoint is actually unspent, and we should proceed as normal.
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.
All implementations of the ChainNotifier interface support registering
notifications on transaction confirmations. This struct is intended to
be used internally by ChainNotifier implementations to handle much of
this logic.
In this commit, we fix an existing bug within the logic of the neutrino
notifier. Rather than properly dispatching only once a transaction had
reached the expected number of confirmations, the historical dispatch
logic would trigger as soon as the transaction reached a single
confirmation.
This was due to the fact that we were using the scanHeight variable
which would be set to zero to calculate the number of confirmations.
The value would end up being the current height, which is generally
always greater than the number of expected confirmations. To remedy
this, we’ll now properly use the block height the transaction was
originally confirmed in to know when to dispatch.
This also applies a fix that was discovered in
93981a85c0b47622a3a5e7089b8bca9b80b834c5.
In this commit, we extend the existing historical dispatch test case to
detect any instances of early dispatches. This catches a class of bug
within a ChainNotifier when the notifier will *always* dispatch early
no matter the number of confirmations. Currently, this test fails for
the neutrino notifier.
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 test adds a test for a consumer that registers for a transaction
confirmation but takes some time to check if that confirmation has
occured.
The test reveals a race condition that can cause btcdnotify to add a
confirmation entry to its internal heap twice. If the notification
consumer is not prompt in reading from the confirmation channel, this
can cause the notifier to block indefinitely.
This commit reduces the neutrino.WaitForMoreCFHeaders parameter when
instantiating a neutrino instance as a lower value will allow the tests
to complete more quickly.
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.
The btclog package has been changed to defining its own logging
interface (rather than seelog's) and provides a default implementation
for callers to use.
There are two primary advantages to the new logger implementation.
First, all log messages are created before the call returns. Compared
to seelog, this prevents data races when mutable variables are logged.
Second, the new logger does not implement any kind of artifical rate
limiting (what seelog refers to as "adaptive logging"). Log messages
are outputted as soon as possible and the application will appear to
perform much better when watching standard output.
Because log rotation is not a feature of the btclog logging
implementation, it is handled by the main package by importing a file
rotation package that provides an io.Reader interface for creating
output to a rotating file output. The rotator has been configured
with the same defaults that btcd previously used in the seelog config
(10MB file limits with maximum of 3 rolls) but now compresses newly
created roll files. Due to the high compressibility of log text, the
compressed files typically reduce to around 15-30% of the original
10MB file.
This commit adds a new case and proper initialization for the
NeutrinoNotifier implementation, such that it can be tested in-line
with the other implementations for proper behavior conformity.
Due to a delay when btcd sends invs for new blocks, the timeouts for
several of the tests has been extended in order to give enough time for
propagation of the new block.
This commit adds an initial rough implementation father ChainNotifier
interface for neutrino, our new light client implementation. This
implementation largely borrows from the existing BtcdNotifier
implementation. As a result, a follow up commit will perform two
refactoring in order to further consolidate code.
This commit updates two interface-level tests for confirmation
notifications to check the txIndex and blockHeight advertised to serve
as regression tests for the recent bug fix related to properly setting
these two fields.
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.
When iterating with the ChainNotifier, it currently isn’t possible to
cancel a non-dispatched yet active notificaiton intent. As a result,
this can be rather wasteful in many parts of lnd which my repeatedly
create a new spend notification depending on if/when a peer is
connected or not.
In order to fix this, we add a new `Cancel func()` field to both the
`BlockEpochEvent` and `SpendEvent` structs. This new closure attribute
allows the caller to cancel the yet-to-be-dispathed event, allowing the
ChainNotifier to free up resources.
