We had a user who pointed out that we weren't creating
`SpendableOutputs` events when we should have been after they
called `ChannelMonitor::best_block_updated` with a block well
after a CSV locktime and then called
`ChannelMonitor::transactions_confirmed` with the transaction which
we should have been spending (with a block height/hash a ways in
the past).
This was due to `ChannelMonitor::transactions_confirmed` only
calling `ChannelMonitor::block_confirmed` with the height at which
the transactions were confirmed, resulting in all checks being done
against that, not the current height.
Further, in the same scenario, we also would not fail-back and HTLC
where the HTLC-Timeout transaction was confirmed more than
ANTI_REORG_DELAY blocks ago.
To address this, we use the best block height for confirmation
threshold checks in `ChannelMonitor::block_confirmed` and pass both
the confirmation and current heights through to
`OnchainTx::update_claims_view`, using each as appropriate.
Fixes#962.
This increases the CLTV_CLAIM_BUFFER constant to 18, much better
capturing how long it takes to go on chain to claim payments.
This is also more in line with other clients, and the spec, which
sets the default CLTV delay in invoices to 18.
As a side effect, we have to increase MIN_CLTV_EXPIRY_DELTA as
otherwise as are subject to an attack where someone can hold an
HTLC being forwarded long enough that we *also* close the channel
on which we received the HTLC.
For users who get PaymentPreimages via
`get_payment_secret_preimage`, they need to provide the
PaymentPreimage back in `claim_funds` but they aren't actually
given the preimage anywhere.
This commit gives users the PaymentPreimage in the
`PaymentReceived` event.
Like the payment_secret parameter, this paramter has been the source
of much confusion, so we just drop it.
Users should prefer to do this check when registering the payment
secret instead of at claim-time.
This allows users to store metadata about an invoice at
invoice-generation time and then index into that storage with a
general-purpose id when they call `get_payment_secret`. They will
then be provided the same index when the payment has been received.
This prepares us for requiring payment_secrets for all received
payments, by demonstrating test changes work even prior to the new
requirement.
In order to avoid needing to pipe payment secrets through to
additional places in the claim logic and then removing that
infrastructure once payment secrets are required, we use the new
payment secret storage in ChannelManager to look up the payment
secret for any given pament hash in claim and fail-back functions.
This part of the diff is reverted in the next commit.
In order to reduce code movement in the next commit, this commit
simply tweaks get_payment_preimage_hash!() and related functions in
functional tests to return a payment secret. Further, we ensure
that we always call get_payment_preimage_hash!() with the node
which will ultimately receive the payment.
There is a possible race condition when both the latest block hash and
height are needed. Combine these in one struct and place them behind a
single lock.
Instead of relying on the user to ensure the funding transaction is
correct (and panicing when it is confirmed), we should check it is
correct when it is generated. By taking the full funding transaciton
from the user on generation, we can also handle broadcasting for
them instead of doing so via an event.
When we force-close a channel, for whatever reason, it is nice to
send an error message to our peer. This allows them to closes the
channel on their end instead of trying to send through it and
failing. Further, it may induce them to broadcast their commitment
transaction, possibly getting that confirmed and saving us on fees.
This commit adds a few more cases where we should have been sending
error messages but weren't. It also includes an almost-global
replace in tests of the second argument in
`check_closed_broadcast!()` from false to true (indicating an error
message is expected). There are only a few exceptions, notably
those where the closure is the result of our counterparty having
sent *us* an error message.
chain::Filter::register_output may return an in-block dependent
transaction that spends the output. Test the scenario where the txdata
given to ChainMonitor::block_connected includes a commitment transaction
whose HTLC output is spent in the same block but not included in txdata.
Instead, it is returned by chain::Filter::register_output when given the
commitment transaction's HTLC output. This is a common scenario for
Electrum clients, which provided filtered txdata.
Many functional tests rely on being able to call block_connected
arbitrarily, jumping back in time to confirm a transaction at a
specific height. Instead, this takes us one step towards having a
well-formed blockchain in the functional tests.
We also take this opportunity to reduce the number of blocks
connected during tests, requiring a number of constant tweaks in
various functional tests.
Co-authored-by: Valentine Wallace <vwallace@protonmail.com>
Co-authored-by: Matt Corallo <git@bluematt.me>
Sadly the connected-in-order tests have to be skipped in our normal
test suite as many tests violate it. Luckily we can still enforce
it in the tests which run in other crates.
Co-authored-by: Matt Corallo <git@bluematt.me>
Co-authored-by: Jeffrey Czyz <jkczyz@gmail.com>
We currently only use it to override the graph-specific features
returned in the route, though we should also use it to enable or
disable MPP.
Note that tests which relied on MPP behavior have had all of their
get_route calls upgraded to provide the MPP flag.
When ChannelMonitors are persisted, they need to store the most recent
block hash seen. However, for newly created channels the default block
hash is used. If persisted before a block is connected, the funding
output may be missed when syncing after a restart. Instead, initialize
ChannelManager with a "birthday" hash so it can be used later when
creating channels.
