This also pays a fee on the transactions we generate in response to
SpendableOutputDescriptors in tests.
This fixes the known issues in #630, though we should test for
standardness in other ways as well.
This resolves a number of bugs around how we calculate feerates on
closing transactions:
* We previously calculated the weight wrong both by always
counting two outputs instead of counting the number of outputs
that actually exist in the closing transaction and by not
counting the witness redeemscript.
* We use assertions to check the calculated weight matches what we
actually build (with debug_assertions for variable-length sigs).
* As an additional sanity check, we really should check that the
transaction had at least min-relay-fee when we were the channel
initator.
It was noticed (via clippy) by @casey that we were taking and then
immediately dropping the total_consistency_lock because `let _ =`
doesn't actually bind the response to anything. This appears to be
a consequence of wanting `if let Some(_) =` to not hold a ref to
the contained value at all, but is relatively surprising to me.
Previously, we had a concept of "rescaning" blocks when we detected
a need to monitor for a new set of outputs in future blocks while
connecting a block. In such cases, we'd need to possibly learn about
these new spends later in the *same block*, requiring clients who
filter blocks to get a newly-filtered copy of the same block. While
redoing the chain access API, it became increasingly clear this was
an overly complicated API feature, and it seems likely most clients
will not use it anyway.
Further, any client who *does* filter blocks can simply update their
filtering algorithm to include any descendants of matched
transactions in the filter results, avoiding the need for rescan
support entirely.
Thus, it was decided that we'd move forward without rescan support
in #649, however to avoid significant further changes in the
already-large 649, we decided to fully remove support in a
follow-up.
Here, we remove the API features that existed for rescan and fix
the few tests to not rely on it.
After this commit, we now only ever have one possible version of
block connection transactions, making it possible to be
significantly more confident in our test coverage actually
capturing all realistic scenarios.
Given the chain::Watch interface is defined in terms of ChannelMonitor
and ChannelMonitorUpdateErr, move channelmonitor.rs from the ln module
to the chain module.
Transaction data from a block may be filtered before it is passed to
block_connected functions, which may need the index of each transaction
within the block. Rather than define each function in terms of a slice
of tuples, define a type alias for the slice where it can be documented.
Outputs to watch are tracked by ChannelMonitor as of
73dce207dd. Instead of determining new
outputs to watch independently using ChainWatchedUtil, do so by
comparing against outputs already tracked. Thus, ChainWatchedUtil and
WatchEvent are no longer needed.
WatchEventProvider served as a means for replacing ChainWatchInterface.
However, it requires users to explicitly fetch WatchEvents, even if not
interested in them. Replace WatchEventProvider by chain::Filter, which
is an optional member of ChainMonitor. If set, interesting transactions
and output spends are registered such that blocks containing them can be
retrieved from a chain source in an efficient manner.
This is useful when the chain source is not a full node. For Electrum,
it allows for pre-filtered blocks. For BIP157/158, it serves as a means
to match against compact filters.
BlockNotifier was removed in the previous commit, thus ChainListener is
no longer needed. Instead, anything needing chain events should be
notified directly.
BlockNotifier is a convenience for handing blocks to listeners. However,
it requires that each listener conforms to the ChainListener interface.
Additionally, there are only two listeners, ChannelManager and
ChainMonitor, the latter of which may not be used when monitoring
channels remotely. Remove BlockNotifier since it doesn't provide much
value and constrains each listener to a specific interface.
ChainMonitor's template Key parameter was meant to allow supporting
both local monitoring, where Key=OutPoint, and watchtowers, where Key=
(PublicKey, u32). Use OutPoint directly since the watchtower case will
not be supported this way.
ManyChannelMonitor was renamed chain::Watch in the previous commit. Use
a more concise name for an implementation that monitors the chain for
channel activity. Future work will parameterize the struct to allow for
different varieties of persistence. Thus, users usually will be able to
use ChainMonitor directly rather than implementing a chain::Watch that
wraps it.
Rename ManyChannelMonitor to chain::Watch and move to chain/mod.rs,
where chain-related interfaces live. Update the documentation for
clarity and to conform to rustdoc formatting.
ChainWatchInterface was intended as an interface for watching rather
than accessing the chain. Remove get_chain_utxo and add chain::Access
trait for this behavior. Wrap it with an Option in NetGraphMsgHandler in
order to simplify the error interface.
Use of ChainWatchInterface was replaced with WatchEvent in the previous
commit. Remove it from the parameterization of SimpleManyChannelMonitor
since it is no longer needed.
SimpleManyChannelMonitor is parameterized by ChainWatchInterface to
signal what transactions and outputs to watch for on chain. The
interface has grown to cover chain access (via get_chain_utxo) and block
block filtering (via filter_block and reentered), which has added
complexity for implementations and user (see ChainWatchInterfaceUtil).
Pull the watch functionality out as a first step to eliminating
ChainWatchInterface entirely.
Replace direct uses of BlockNotifier in functional tests with utility
functions. This is in preparation for signaling watch events back via a
refactoring of ManyChannelMonitor and ChainWatchInterface. Those events
will be processed by connect_block.
Change confirm_transaction and connect_blocks to take a Node instead of
a BlockNotifier. This is in preparation for signaling watch events back
via a refactoring of ManyChannelMonitor and ChainWatchInterface.
Use a simple ChainListner implementation rather than large test objects
for testing BlockNotifier.
Remove unregister_single_listener_ref_test since it is redundant with
unregister_single_listener_test.
Remove unnecessary clone() calls.
ChannelMonitor has block_connected and block_disconnected methods called
by <SimpleManyChannelMonitor as ChainListener>. Use similar parameters
in ChannelMonitor such that transformations are not needed and the
interface is more closely aligned with ChainListener.
ChainListeners should be independent of each other, but in practice this
is not the case because ChainWatchInterface introduces a dependency
between them. Push ChainWatchInterface down into the ChainListener
implementations where needed. Update ChainListener's block_connected
method to take a slice of the form &[(usize, &Transaction)] where each
transaction is paired with its position within the block.
In anticipation for removing support for users calling
block_connected multiple times for the same block to include all
relevant transactions in the next PR, this commit stops testing
such cases. Specifically, users who filter blocks for relevant
transactions before calling block_connected will need to filter by
including any transactions which spend a previously-matched
transaction in the same block (and we now do so in our own
filtering logic, which is also used in our testing).
Add a few comments to make it clear whats going on a bit more and
assert the basic structure of more transactions.
Further, point out where transactions are double-spends and don't
confirm double-spends.
We'd previously largely not turned on derive(Debug) on any of our
structs, but not for good reason. Especially for Events objects,
Debug can be a very useful for users to quickly print what they
received from us without having to write out a large match.
* Channel::get_counterparty_htlc_minimum_msat() returned
holder_htlc_minimum_msat, which was obviously incorrect.
* ChannelManager::get_channel_update set htlc_minimum_msat to
Channel::get_holder_htlc_minimum_msat(), but the spec explicitly
states we "MUST set htlc_minimum_msat to the minimum HTLC value
(in millisatoshi) that the channel peer will accept." This makes
sense because the reason we're rejecting the HTLC is because our
counterparty's HTLC minimum value is too small for us to send to
them, our own HTLC minimum value plays no role. Further, our
router already expects this - looking at the same directional
channel info as it does fees.
Finally, we add a test in the existing onion router test cases
which fails if either of the above is incorrect (the second issue
discovered in the process of writing the test).
