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.
