This stores transaction templates temporarily until their locktime
is reached, avoiding broadcasting (or RBF bumping) transactions
prior to their locktime. For those broadcasting transactions
(potentially indirectly) via Bitcoin Core RPC, this ensures no
automated rebroadcast of transactions on the client side is
required to get transactions confirmed.
Currently, we only send an update_channel message after
disconnecting a peer and waiting some time. We do not send a
followup when the peer has been reconnected for some time.
This changes that behavior to make the disconnect and reconnect
channel updates symmetric, and also simplifies the state machine
somewhat to make it more clear.
Finally, it serializes the current announcement state so that we
usually know when we need to send a new update_channel.
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.
Current Bitcoin Core's policy will reject a p2wsh as a dust if it's
under 330 satoshis. A typical p2wsh output is 43 bytes big to which
Core's `GetDustThreshold()` sums up a minimal spend of 67 bytes (even
if a p2wsh witnessScript might be smaller). `dustRelayFee` is set
to 3000 sat/kb, thus 110 * 3000 / 1000 = 330. As all time-sensitive
outputs are p2wsh, a value of 330 sat is the lower bound desired
to ensure good propagation of transactions. We give a bit margin to
our counterparty and pick up 660 satoshis as an accepted
`dust_limit_satoshis` upper bound.
As this reasoning is tricky and error-prone we hardcode it instead of
letting the user picking up a non-sense value.
Further, this lower bound of 330 sats is also hardcoded as another constant
(MIN_DUST_LIMIT_SATOSHIS) instead of being dynamically computed on
feerate (derive_holder_dust_limit_satoshis`). Reducing risks of
non-propagating transactions in casee of failing fee festimation.
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.
Our current PaymentReceived API is incredibly easy to mis-use -
the "obvious" way to implement a client is to always call
`ChannelManager::claim_funds` in response to a `PaymentReceived`
event. However, users are *required* to check the payment secret
and value against the expected values before claiming in order to
avoid a number of potentially funds-losing attacks.
Instead, if we rely on payment secrets being pre-registered with
the ChannelManager before we receive HTLCs for a payment we can
simply check the payment secrets and never generate
`PaymentReceived` events if they do not match. Further, when the
user knows the value to expect in advance, we can have them
register it as well, allowing us to check it for them.
Other implementations already require payment secrets for inbound
payments, so this shouldn't materially lose compatibility.
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.
During the block API refactor, we started calling
Channel::force_shutdown when a channel is closed due to a bogus
funding tx. However, we still set the channel's state to Shutdown
prior to doing so, leading to an assertion in force_shutdown (that
the channel is not already closed).
This removes the state-set call and adds a (long-overdue) test for
this case.
Fixes: 60b962a18e
This test failed when ConnectionStyle was set to a SkippingBlocks
variant because of a bug in ChannelMonitor::update_best_block.
Parameterize the test with these styles to catch any regressions.
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.
This expands the assertions on block ordering to apply to
`#[cfg(test)]` builds in addition to normal builds, requiring that
unit and functional tests have syntactically-valid (ie the previous
block hash pointer and the heights match the blocks) blockchains.
This requires a reasonably nontrivial diff in the functional tests
however it is mostly straightforward changes.
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>
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.
functional_tests.rs is huge, so anything we can do to split it up
some is helpful. This also exposes a somewhat glaring lack of
reorgs in our existing tests.
