Adds Transaction to lighting-block-sync::convert #1061
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #1061 +/- ##
==========================================
- Coverage 90.76% 90.75% -0.01%
==========================================
Files 65 65
Lines 33894 33968 +74
==========================================
+ Hits 30764 30829 +65
- Misses 3130 3139 +9
Continue to review full report at Codecov.
|
|
Hmm, I do wonder how far down this path we want to go - we're starting to get to the point of duplicating a good chunk of https://github.com/rust-bitcoin/rust-bitcoincore-rpc, whereas we could go the other way and merge our HTTP client work there so that we get the RPC conversion logic from that crate and can still use our almost-dependency-free async-optional HTTP client. |
I was unaware of this 😅 Is it worth to push this PR forward or may it be better to put the effort into merging our HTTP client into Not sure how much of a heavy lift would be to do the latter, but I can certainly give it a look. |
Hmm, I think its kinda up to you. I don't want to end up maintaining a full RPC client here, that would be a huge waste of effort, but one or two converters isn't so bad. Another PR or two and I'd want to call time out, though :p.
Yea, I haven't looked at all :/ |
|
@TheBlueMatt I went straightaway with this one, but for any other rpc functionality I'll give a shot to merging the logic into I've simplified the logic in the last commit given errors are not passed as I initially assumed. This has been tested against Both commits can be squashed, left them as they are for the review diffs. |
|
Hmm, so this is a bit awkward - this will break when reading a transaction from Bitcoin Core with zero inputs, I think. The rust-bitcoin deserialization logic doesn't tolerate the zero-input serialization ambiguity of segwit, and bitcoin core serializes such transactions that way for backwards compatibility. Trying to fix it is probably nontrivially complex/requires deserializing somewhat manually based on context. |
|
Umm that's unfortunate. Not sure how to proceed here since, given this seems a limitation of rust-bitcoin wrt core (or a design choice, as you'd rather), having a dedicated parser for this alone may be cumbersome. On the other hand, it may not be safe to assume the library should not accept this kind of transactions. |
|
Hmm, looks like rust-bitcoincore-rpc doesn't handle this right either (rust-bitcoin/rust-bitcoincore-rpc#197). Maybe for now we merge this and add a note on |
sr-gi
added a commit
to sr-gi/rust-lightning
that referenced
this pull request
Sep 6, 2021
|
I've added a note to |
jkczyz
reviewed
Sep 17, 2021
lightning-block-sync/src/convert.rs
Outdated
| @@ -156,6 +156,7 @@ impl TryInto<(BlockHash, Option<u32>)> for JsonResponse { | |||
| } | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
nit: remove extra blank line
lightning-block-sync/src/convert.rs
Outdated
| @@ -181,13 +182,79 @@ impl TryInto<Txid> for JsonResponse { | |||
| } | |||
| } | |||
|
|
|||
| /// Converts a JSON value into a transaction. WATCH OUT! this cannot be used form zero-input transactions | |||
lightning-block-sync/src/convert.rs
Outdated
| std::io::ErrorKind::InvalidData, | ||
| "invalid hex data", | ||
| )), | ||
|
|
lightning-block-sync/src/convert.rs
Outdated
Comment on lines
612
to
613
| /// TryInto<Transaction> can be used in two ways, first with plain hex response where data is the hex encoded transaction (e.g. as a result of getrawtransaction) | ||
| /// or as a JSON object where the hex encoded transaction can be found in the hex field of the object (if present) (e.g. as a result of signrawtransactionwithwallet). |
There was a problem hiding this comment.
Use // for non-doc comments.
lightning-block-sync/src/convert.rs
Outdated
Comment on lines
612
to
613
| /// TryInto<Transaction> can be used in two ways, first with plain hex response where data is the hex encoded transaction (e.g. as a result of getrawtransaction) | ||
| /// or as a JSON object where the hex encoded transaction can be found in the hex field of the object (if present) (e.g. as a result of signrawtransactionwithwallet). |
There was a problem hiding this comment.
Could you wrap at 100 characters?
lightning-block-sync/src/convert.rs
Outdated
| let response = JsonResponse(serde_json::json!(encode::serialize_hex(&target_tx))); | ||
| match TryInto::<Transaction>::try_into(response) { | ||
| Err(e) => panic!("Unexpected error: {:?}", e), | ||
| Ok(txid) => assert_eq!(txid, target_tx), |
lightning-block-sync/src/convert.rs
Outdated
Comment on lines
655
to
656
| let tx_bytes = Vec::from_hex("0100000001a15d57094aa7a21a28cb20b59aab8fc7d1149a3bdbcddba9c622e4f5f6a99ece010000006c493046022100f93bb0e7d8db7bd46e40132d1f8242026e045f03a0efe71bbb8e3f475e970d790221009337cd7f1f929f00cc6ff01f03729b069a7c21b59b1736ddfee5db5946c5da8c0121033b9b137ee87d5a812d6f506efdd37f0affa7ffc310711c06c7f3e097c9447c52ffffffff0100e1f505000000001976a9140389035a9225b3839e2bbf32d826a1e222031fd888ac00000000").unwrap(); | ||
| let target_tx = deserialize(&tx_bytes).unwrap(); | ||
| let response = JsonResponse(serde_json::json!(encode::serialize_hex(&target_tx))); |
There was a problem hiding this comment.
Not quite sure I follow the conversions. Is the string literal on line 655 the same as the hex produced by encode::serialize_hex on line 657? Would it be simpler to take say the coinbase Transaction from the genesis block as expected and converted to hex for the JSON response.
sr-gi
added a commit
to sr-gi/rust-lightning
that referenced
this pull request
Sep 20, 2021
sr-gi
force-pushed
the
add-transaction-convert
branch
from
f8a01a0
to
1a64496
Compare
|
Addressed the minor comments pointed out by @jkczyz and rebased from master |
jkczyz
approved these changes
Sep 20, 2021
There was a problem hiding this comment.
ACK 1a64496. Tested with sample node and was able to open a channel.
Once @TheBlueMatt approves, I think you can squash the commits.
TheBlueMatt
reviewed
Sep 20, 2021
| } | ||
| } | ||
|
|
||
| // transaction in hex field of JSON object |
There was a problem hiding this comment.
Could use a success test with a {"hex"} json object.
|
Moving to 102 so we can get 101 out today. |
Includes disclaimer in docs, see lightningdevkit#1061 (comment)
sr-gi
force-pushed
the
add-transaction-convert
branch
from
1a64496
to
5718e43
Compare
Sorry, I didn't get you were planning on releasing today. I just pushed the requested changes. |
TheBlueMatt
approved these changes
Sep 21, 2021
There was a problem hiding this comment.
Thanks. Diff since Jeff's ACK is test-only:
$ git diff-tree -U1 1a64496 5718e43b658d07e4c028b91990c75c089f801cfd
diff --git a/lightning-block-sync/src/convert.rs b/lightning-block-sync/src/convert.rs
index d7131f25..358076f4 100644
--- a/lightning-block-sync/src/convert.rs
+++ b/lightning-block-sync/src/convert.rs
@@ -652,3 +652,3 @@ pub(crate) mod tests {
#[test]
- fn into_tx_from_json_response_with_valid_tx_data() {
+ fn into_tx_from_json_response_with_valid_tx_data_plain() {
let genesis_block = genesis_block(Network::Bitcoin);
@@ -662,2 +662,13 @@ pub(crate) mod tests {
+ #[test]
+ fn into_tx_from_json_response_with_valid_tx_data_hex_field() {
+ let genesis_block = genesis_block(Network::Bitcoin);
+ let target_tx = genesis_block.txdata.get(0).unwrap();
+ let response = JsonResponse(serde_json::json!({"hex": encode::serialize_hex(&target_tx)}));
+ match TryInto::<Transaction>::try_into(response) {
+ Err(e) => panic!("Unexpected error: {:?}", e),
+ Ok(tx) => assert_eq!(&tx, target_tx),
+ }
+ }
+
// transaction in hex field of JSON object
$
vss96
added a commit
to vss96/rust-lightning
that referenced
this pull request
Jan 19, 2022
Add methods to count total fees and total amount in a Route #999
* Added `get_total_fees` method to route,
to calculate all the fees paid accross each path.
* Added `get_total_amount` method to route,
to calculate the total of actual amounts paid in each path.
Update docs to specify where process events is called
Add Event::ChannelClosed generation at channel shutdown
Rename MonitorEvent::CommitmentTxBroadcasted to CommitmentTxConfirmed
Extend MsgHandleErrInternal with a new chan_id field Option<[u8; 32]>
This field is used in next commit to generate appropriate
ChannelClosed event at `handle_error()` processing.
Add ChannelClosed generation at cooperative/force-close/error processing
When we detect a channel `is_shutdown()` or call on it
`force_shutdown()`, we notify the user with a Event::ChannelClosed
informing about the id and closure reason.
Add `pending_events` deadlock detection in `handle_error`
Bump HTTP read timeout to match reality of Bitcoin Core blocking
Fix future unknown `Event` variant backwards compatibility
In 8ffc2d1742ff1171a87b0410b21cbbd557ff8247, in 0.0.100, we added
a backwards compatibility feature to the reading of `Event`s - if
the type was unknown and odd, we'd simply ignore the event and
treat it as no event. However, we failed to read the
length-prefixed TLV stream when doing so, resulting in us reading
some of the skipped-event data as the next event or other data in
the ChannelManager.
We fix this by reading the varint length prefix written, then
skipping that many bytes when we come across an unknown odd event
type.
Fix a panic in Route's new fee-calculation methods and clean up
This addresses Val's feedback on the new Route fee- and
amount-calculation methods, including fixing the panic she
identified and cleaning up various docs and comments.
Adds Transaction to lighting-block-sync::convert
Includes disclaimer in docs, see https://github.com/rust-bitcoin/rust-lightning/pull/1061#issuecomment-911960862
Rename PaymentFailed -> PaymentPathFailed
Since we don't want to imply to users that a payment has
completely failed when it really has just partially
failed
Add path field to PaymentPathFailed event
Fix windows-only test failure added in #997
This is a trivial bugfix to add a missing test updated required in
PR 997.
Move trait bounds on `wire::Type` from use to the trait itself
`wire::Type` is only (publicly) used as the `CustomMessage`
associated type in `CustomMessageReader`, where it has additional
trait bounds on `Debug` and `Writeable`. The documentation for
`Type` even mentions that you need to implement `Writeable` because
this is the one place it is used.
To make this more clear, we move the type bounds onto the trait
itself and not on the associated type.
This is also the only practical way to build C bindings for `Type`
as we cannot have a concrete, single, `Type` struct in C which only
optionally implements various subtraits, at least not without
runtime checking of the type bounds.
Make `ChainMonitor::get_claimable_balances` take a slice of refs
For the same reason as `get_route`, a slice of objects isn't
practical to map to bindings - the objects in the bindings space
are structs with a pointer and some additional metadata. Thus, to
create a slice of them, we'd need to take ownership of the objects
behind the pointer, place them into a slace, and then restore them
to the pointer.
This would be a lot of memory copying and marshalling, not to
mention wouldn't be thread-safe, which the same function otherwise
would be if we used a slice of references instead of a slice of
objects.
Use Infallible for the unconstructable default custom message type
When we landed custom messages, we used the empty tuple for the
custom message type for `IgnoringMessageHandler`. This was fine,
except that we also implemented `Writeable` to panic when writing
a `()`. Later, we added support for anchor output construction in
CommitmentTransaction, signified by setting a field to `Some(())`,
which is serialized as-is.
This causes us to panic when writing a `CommitmentTransaction`
with `opt_anchors` set. Note that we never set it inside of LDK,
but downstream users may.
Instead, we implement `Writeable` to write nothing for `()` and use
`core::convert::Infallible` for the default custom message type as
it is, appropriately, unconstructable.
This also makes it easier to implement various things in bindings,
as we can always assume `Infallible`-conversion logic is
unreachable.
Drop redundant generic bounds when the trait requires the bounds
Make method time on trait impl explitit to help bindings generator
Associated types in C bindings is somewhat of a misnomer - we
concretize each trait to a single struct. Thus, different trait
implementations must still have the same type, which defeats the
point of associated types.
In this particular case, however, we can reasonably special-case
the `Infallible` type, as an instance of it existing implies
something has gone horribly wrong.
In order to help our bindings code figure out how to do so when
referencing a parent trait's associated type, we specify the
explicit type in the implementation method signature.
Update CHANGELOG for 0.0.101
Bump Crate versions to 0.0.101 (and invoice to 0.9)
Make `NetworkGraph` Clone-able again
There isn't a lot of user-utility for cloning `NetworkGraph`
directly (its a rather large struct, and there probably isn't a lot
of reason to have *multiple* `NetworkGraph`s). Thus, when locks
were pushed down into it, the `Clone`-ability of it was dropped as
well.
Sadly, mapping the Java memory model onto:
* `Read`-ing a `NetworkGraph`, creating a Java-owned
`NetworkGraph` object that the JVM will destruct for us,
* Passing it to a `NetGraphMsgHandler`, which now expects to own
the `NetworkGraph`, including destructing it,
isn't really practical without adding a clone in between.
Given this, and the fact that there's nothing inherently wrong with
clone-ing a `NetworkGraph`, we simply re-add `Clone` here.
Drop broken test that is unfixable due to being undocumented
This should be reverted at some point, but the test is deficient
and breaks on later changes that are important to land ASAP.
Increase our default/minimum dust limit to 354 sat/vbytes
330 sat/vbyte, the current value, is not sufficient to ensure a
future segwit script longer than 32 bytes meets the dust limit if
used for a shutdown script. Thus, we can either check the value
on shutdown or we can simply require segwit outputs and require a
dust value of no less than 354 sat/vbyte.
We swap the minimum dust value to 354 sat/vbyte here, requiring
segwit scripts in a future commit.
See https://github.com/lightningnetwork/lightning-rfc/issues/905
Reduce the maximum allowed counterparty dust limit to 546 sat/vbyte
546 sat/vbyte is the current default dust limit on most
implementations, matching the network dust limit for P2SH outputs.
Implementations don't currently appear to send any larger dust
limits, and allowing a larger dust limit implies higher payment
failure risk, so we'd like to be as tight as we can here.
Require user cooperative close payout scripts to be Segwit
There is little reason for users to be paying out to non-Segwit
scripts when closing channels at this point. Given we will soon, in
rare cases, force-close during shutdown when a counterparty closes
to a non-Segwit script, we should also require it of our own users.
Force-close channels if closing transactions may be non-standard
If a counterparty (or an old channel of ours) uses a non-segwit
script for their cooperative close payout, they may include an
output which is unbroadcastable due to not meeting the network dust
limit.
Here we check for this condition, force-closing the channel instead
if we find an output in the closing transaction which does not meet
the limit.
Rename MIN_DUST_LIMIT_SATOSHIS constant to disambiguate chan vs P2P
While channel and P2P network dust limits are related, they're
ultimately two different things, and thus their constant names
should reference that.
Regenerate PendingHTLCsForwardable on reload instead of serializing
When we are prepared to forward HTLCs, we generate a
PendingHTLCsForwardable event with a time in the future when the
user should tell us to forward. This provides some basic batching
of forward events, improving privacy slightly.
After we generate the event, we expect users to spawn a timer in
the background and let us know when it finishes. However, if the
user shuts down before the timer fires, the user will restart and
have no idea that HTLCs are waiting to be forwarded/received.
To fix this, instead of serializing PendingHTLCsForwardable events
to disk while they're pending (before the user starts the timer),
we simply regenerate them when a ChannelManager is deserialized
with HTLCs pending.
Fixes #1042
Don't apply monitor updates after watch_channel PermFail
The full stack fuzzer found an unreachable panic where we receive a
FundingSigned with a duplicate channel outpoint.
Update Watch docs to disallow dup channel outpoints
on watch_channel
Rename MppId to PaymentId
Leftover from previous PR Jeff feedback.
Useful in upcoming commits as we'll expose this to users for payment retries
Return PaymentId from send_*payment functions
Used in upcoming commits for retries
Refactor send_payment internals for retries
We want to reuse send_payment internal functions for retries,
so some need to now be parameterized by PaymentId to avoid
generating a new PaymentId on retry
Refactor send_payment internals for retries 2
Retrying a partial payment means send_payment_internal needs to be parameterized
by a total payment amount, else 'HTLC values do not match' errors
channelmanager: Add retry data to pending_outbound_payments
Add method to retry payments
Don't remove failed payments when all paths fail
This is because we want the ability to retry completely failed
payments.
Upcoming commits will remove these payments on timeout to prevent
DoS issues
Also test that this removal allows retrying single-path payments
Expire outbound payments after 3 blocks if no parts are pending
Correct step number in `get_route`
Consider many first-hop paths to the same counterparty in routing
Previously we'd simply overwritten "the" first hop path to each
counterparty when routing, however this results in us ignoring all
channels except the last one in the `ChannelDetails` list per
counterparty.
f readability improvements from val
Update Event::PaymentReceived docs since we require payment secret
Users no longer need to verify the amounts of received payments as
the payment secret will protect us against the probing attacks such
verification was intended to fix.
Move tests of payment retries into a new module
Move pending payment tracking to after the new HTLC flies
If we attempt to send a payment, but the HTLC cannot be send due to
local channel limits, we'll provide the user an error but end up
with an entry in our pending payment map. This will result in a
memory leak as we'll never reclaim the pending payment map entry.
Correct error returned when `retry_payment` doesn't have a payment
Add payment_hash to PaymentSent #999
Replace PublicKey with [u8; 33] in NetworkGraph
Adds DiscardFunding event
During the event of a channel close, if the funding transaction
is yet to be broadcasted then a DiscardFunding event is issued
along with the ChannelClose event.
Use local channel state when constructing routes in test macro
This is a bit more realistic and needed to route over non-public
channels.
Fix loop label shadowing warning
Remove special case for onion error expiry_too_far
With channel scoring and payment retries, it is no longer necessary to
have expiry_too_far imply a payment failure.
Pass hop index in construct_onion_keys_callback
This simplifies failing route hop calculation, which will be useful for
later identifying the failing hop for PaymentFailed events.
Clean up fee_insufficient computation
Add failing short channel id to PaymentPathFailed
This will be useful for scoring channels when a payment fails.
Expose ReadOnlyNetworkGraph::get_addresses to C by cloning result
We cannot expose ReadOnlyNetworkGraph::get_addresses as is in C as
it returns a list of references to an enum, which the bindings
dont support. Instead, we simply clone the result so that it
doesn't contain references.
Speed up test_timer_tick_called
Fix unused variable warnings in fuzzer
Replace get_route with get_route_and_payment_hash
The interface for get_route will change to take a scorer. Using
get_route_and_payment_hash whenever possible allows for keeping the
scorer inside get_route_and_payment_hash rather than at every call site.
Replace get_route with get_route_and_payment_hash wherever possible.
Additionally, update get_route_and_payment_hash to use the known invoice
features and the sending node's logger.
Move mpp_failure test to payment_tests.rs
Move `Persist` trait to chainmonitor as that's the only reference
Move ChannelMonitorUpdateErr to chain as it is a chain::Watch val
Make `ChainMonitor::monitors` private and expose monitor via getter
Exposing a `RwLock<HashMap<>>` directly was always a bit strange,
and in upcoming changes we'd like to change the internal
datastructure in `ChainMonitor`.
Further, the use of `RwLock` and `HashMap` meant we weren't able
to expose the ChannelMonitors themselves to users in bindings,
leaving a bindings/rust API gap.
Thus, we take this opportunity go expose ChannelMonitors directly
via a wrapper, hiding the internals of `ChainMonitor` behind
getters. We also update tests to use the new API.
Simplify channelmonitor tests which use chain::Watch and Persister
test_simple_monitor_permanent_update_fail and
test_simple_monitor_temporary_update_fail both have a mode where
they use either chain::Watch or persister to return errors.
As we won't be doing any returns directly from the chain::Watch
wrapper in a coming commit, the chain::Watch-return form of the
test will no longer make sense.
Handle Persister returning TemporaryFailure for new channels
Previously, if a Persister returned a TemporaryFailure error when
we tried to persist a new channel, the ChainMonitor wouldn't track
the new ChannelMonitor at all, generating a PermanentFailure later
when the updating is restored.
This fixes that by correctly storing the ChannelMonitor on
TemporaryFailures, allowing later update restoration to happen
normally.
This is (indirectly) tested in the next commit where we use
Persister to return all monitor-update errors.
Use Persister to return errors in tests not chain::Watch
As ChainMonitor will need to see those errors in a coming PR,
we need to return errors via Persister so that our ChainMonitor
chain::Watch implementation sees them.
Use Persister to return errors in fuzzers not chain::Watch
Fixed 'Advancing Bitcoin' video URL.
Add channel scoring to get_route
Failed payments may be retried, but calling get_route may return a Route
with the same failing path. Add a routing::Score trait used to
parameterize get_route, which it calls to determine how much a channel
should be penalized in terms of msats willing to pay to avoid the
channel.
Also, add a Scorer struct that implements routing::Score with a constant
constant penalty. Subsequent changes will allow for more robust scoring
by feeding back payment path success and failure to the scorer via event
handling.
Return the temporary channel id in success from `create_channel`
This makes it more practical for users to track channels prior to
funding, especially if the channel fails because the peer rejects
it for a parameter mismatch.
Include the user channel id counter in Event::ChannelClosed
This makes it more practical for users to track channels using
their own IDs, especially across funding.
Rename create_channel param to user_channel_id to standardize it
Add CHANGELOG entries for 0.0.102
Bump crate versions to 0.0.102 and lightning-invoice 0.10
Simplify prefers_shorter_route_with_higher_fees
In order to make the scoring tests easier to read, only check the
relevant RouteHop fields. The remaining fields are tested elsewhere.
Expand the test to show the path used without scoring.
Add source and target nodes to routing::Score
Expand routing::Score::channel_penalty_msat to include the source and
target node ids of the channel. This allows scorers to avoid certain
nodes altogether if desired.
Move MonitorEvent serialization to TLV-enum-upgradable from custom
Move the two-AtomicUsize counter in peer_handler to a util struct
We also take this opportunity to drop byte_utils::le64_to_array, as
our MSRV now supports the native to_le_bytes() call.
Move ChannelManager::monitor_updated to a MonitorEvent
In the next commit we'll need ChainMonitor to "see" when a monitor
persistence completes, which means `monitor_updated` needs to move
to `ChainMonitor`. The simplest way to then communicate that
information to `ChannelManager` is via `MonitorEvet`s, which seems
to line up ok, even if they're now constructed by multiple
different places.
Use an opaque type to describe monitor updates in Persist
In the next commit, we'll be originating monitor updates both from
the ChainMonitor and from the ChannelManager, making simple
sequential update IDs impossible.
Further, the existing async monitor update API was somewhat hard to
work with - instead of being able to generate monitor_updated
callbacks whenever a persistence process finishes, you had to
ensure you only did so at least once all previous updates had also
been persisted.
Here we eat the complexity for the user by moving to an opaque
type for monitor updates, tracking which updates are in-flight for
the user and only generating monitor-persisted events once all
pending updates have been committed.
Persist `ChannelMonitor`s after new blocks are connected
This resolves several user complaints (and issues in the sample
node) where startup is substantially delayed as we're always
waiting for the chain data to sync.
Further, in an upcoming PR, we'll be reloading pending payments
from ChannelMonitors on restart, at which point we'll need the
change here which avoids handling events until after the user
has confirmed the `ChannelMonitor` has been persisted to disk.
It will avoid a race where we
* send a payment/HTLC (persisting the monitor to disk with the
HTLC pending),
* force-close the channel, removing the channel entry from the
ChannelManager entirely,
* persist the ChannelManager,
* connect a block which contains a fulfill of the HTLC, generating
a claim event,
* handle the claim event while the `ChannelMonitor` is being
persisted,
* persist the ChannelManager (before the CHannelMonitor is
persisted fully),
* restart, reloading the HTLC as a pending payment in the
ChannelManager, which now has no references to it except from
the ChannelMonitor which still has the pending HTLC,
* replay the block connection, generating a duplicate PaymentSent
event.
Update test_dup_htlc_onchain_fails_on_reload for new persist API
ChannelMonitors now require that they be re-persisted before
MonitorEvents be provided to the ChannelManager, the exact thing
that test_dup_htlc_onchain_fails_on_reload was testing for when it
*didn't* happen. As such, test_dup_htlc_onchain_fails_on_reload is
now testing that we bahve correctly when the API guarantees are not
met, something we don't need to do.
Here, we adapt it to test the new API requirements through
ChainMonitor's calls to the Persist trait instead.
Always release `MonitorEvent`s to `ChannelManager` after 3 blocks
If we have a `ChannelMonitor` update from an on-chain event which
returns a `TemporaryFailure`, we block `MonitorEvent`s from that
`ChannelMonitor` until the update is persisted. This prevents
duplicate payment send events to the user after payments get
reloaded from monitors on restart.
However, if the event being avoided isn't going to generate a
PaymentSent, but instead result in us claiming an HTLC from an
upstream channel (ie the HTLC was forwarded), then the result of a
user delaying the event is that we delay getting our money, not a
duplicate event.
Because user persistence may take an arbitrary amount of time, we
need to bound the amount of time we can possibly wait to return
events, which we do here by bounding it to 3 blocks.
Thanks to Val for catching this in review.
Clarify the contexts in which persist_new_channel may be called
Its somewhat confusing that `persist_new_channel` is called on
startup for an existing channel in common deployments, so we call
it out explicitly.
Make `Channel::revoke_and_ack`'s return tuple a struct
This substantially improves readability at the callsite and in the
function.
Make `Channel::monitor_updating_restored`'s return tuple a struct
This improves readability at the callsite and in the function.
Implement `HashMap` read for `MaybeReadable` values
This allows us to read a `HashMap` that has values which may be
skipped if they are some backwards-compatibility type.
We also take this opportunity to fail deserialization if keys are
duplicated.
Inform ChannelManager when fulfilled HTLCs are finalized
When an HTLC has been failed, we track it up until the point there
exists no broadcastable commitment transaction which has the HTLC
present, at which point Channel returns the HTLCSource back to the
ChannelManager, which fails the HTLC backwards appropriately.
When an HTLC is fulfilled, however, we fulfill on the backwards path
immediately. This is great for claiming upstream HTLCs, but when we
want to track pending payments, we need to ensure we can check with
ChannelMonitor data to rebuild pending payments. In order to do so,
we need an event similar to the HTLC failure event, but for
fulfills instead.
Specifically, if we force-close a channel, we remove its off-chain
`Channel` object entirely, at which point, on reload, we may notice
HTLC(s) which are not present in our pending payments map (as they
may have received a payment preimage, but not fully committed to
it). Thus, we'd conclude we still have a retryable payment, which
is untrue.
This commit does so, informing the ChannelManager via a new return
element where appropriate of the HTLCSource corresponding to the
failed HTLC.
Track payments after they resolve until all HTLCs are finalized
In the next commit, we will reload lost pending payments from
ChannelMonitors during restart. However, in order to avoid
re-adding pending payments which have already been fulfilled, we
must ensure that we do not fully remove pending payments until all
HTLCs for the payment have been fully removed from their
ChannelMonitors.
We do so here, introducing a new PendingOutboundPayment variant
called `Completed` which only tracks the set of pending HTLCs.
Rename payment object vars to refer to payments and not session IDs
Add PaymentSecrets to HTLCSource::OutboundRoute objects
Reload pending payments from ChannelMonitor HTLC data on reload
If we go to send a payment, add the HTLC(s) to the channel(s),
commit the ChannelMonitor updates to disk, and then crash, we'll
come back up with no pending payments but HTLC(s) ready to be
claim/failed.
This makes it rather impractical to write a payment sender/retryer,
as you cannot guarantee atomicity - you cannot guarantee you'll
have retry data persisted even if the HTLC(s) are actually pending.
Because ChannelMonitors are *the* atomically-persisted data in LDK,
we lean on their current HTLC data to figure out what HTLC(s) are a
part of an outbound payment, rebuilding the pending payments list
on reload.
Add some basic test coverage of monitor payment data reloading
Move test_dup_htlc_onchain_fails_on_reload to payment_tests
test_dup_htlc_onchain_fails_on_reload is now more of a
payment_test than a functional_test, testing for handling of
pending payments.
Add a test of an HTLC being fulfilled and then later failed
Peers probably shouldn't do this, but if they want to give us free
money, we should take it and not generate any spurious events.
Define Payee abstraction for use in get_route
A payee can be identified by a pubkey and optionally have an associated
set of invoice features and route hints. Use this in get_route instead
of three separate parameters. This may be included in PaymentPathFailed
later to use when finding a new route.
Remove outdated line from get_route docs
Include PaymentPathRetry data in PaymentPathFailed
When a payment path fails, it may be retried. Typically, this means
re-computing the route after updating the NetworkGraph and channel
scores in order to avoid the failing hop. The last hop in
PaymentPathFailed's path field contains the pubkey, amount, and CLTV
values needed to pass to get_route. However, it does not contain the
payee's features and route hints from the invoice.
Include the entire set of parameters in PaymentPathRetry and add it to
the PaymentPathFailed event. Add a get_retry_route wrapper around
get_route that takes PaymentPathRetry. This allows an EventHandler to
retry failed payment paths using the payee's route hints and features.
Use option TLV decoding for short_channel_id
Using ignorable TLV decoding is only applicable for an Option containing
an enum, but short_channel_id is an Option<u64>. Use option TLV encoding
instead.
Make `Payee::pubkey` pub.
`Payee` is expected to be used by users to get routes for payment
retries, potentially with their own router. Thus, its helpful if it
is pub, even if it is redundant with the last hop in the `path`
field in `Events::PaymentPathFailed`.
Copy `Payee` into `Route`s to provide them to `ChannelManager`
Store `Payee` information in `HTLCSource::OutboundRoute`.
This stores and tracks HTLC payee information with HTLCSource info,
allowing us to provide it back to the user if the HTLC fails and
ensuring persistence by keeping it with the HTLC itself as it
passes between Channel and ChannelMonitor.
Expose log_bytes! macro for use in other crates
Needed to log PaymentHash in the lightning-invoice crate when retrying
payments.
Add PaymentId to PaymentSent event
The payment_hash may not uniquely identify the payment if it has been
reused. Include the payment_id in PaymentSent events so it can
correlated with the send_payment call.
Add PaymentId to PaymentPathFailed event
The PaymentId is needed when retrying payments. Include it in the
PaymentPathFailed event so it can be used in that manner.
Rewrite Invoice's interface in terms of msats
InvoiceBuilder's interface was changed recently to work in terms of
msats. Update Invoice's interface to return the amount in msats, too,
and make amount_pico_btc private.
Unify route finding methods
An upcoming Router interface will be used for finding a Route both when
initially sending a payment and also when retrying failed payment paths.
Unify the three varieties of get_route so the interface can consist of a
single method implemented by the new `find_route` method. Give get_route
pub(crate) visibility so it can still be used in tests.
Add InvoicePayer for retrying failed payments
When a payment fails, it's useful to retry the payment once the network
graph and channel scores are updated. InvoicePayer is a utility for
making payments which will retry any failed payment paths for a payment
up to a configured number of total attempts. It is parameterized by a
Payer and Router for ease of customization and testing.
Implement EventHandler for InvoicePayer as a decorator that intercepts
PaymentPathFailed events and retries that payment using the parameters
from the event. It delegates to the decorated EventHandler after retries
have been exhausted and for other events.
Support paying zero-value invoices
Fail payment retry if Invoice is expired
According to BOLT 11:
- after the `timestamp` plus `expiry` has passed
- SHOULD NOT attempt a payment
Add a convenience method for checking if an Invoice has expired, and use
it to short-circuit payment retries.
Implement Payer and Router for lightning crate
Implements Payer for ChannelManager and Rotuer for find_route, which can
be used to parameterize InvoicePayer when needing payment retries.
Add a utility trait in `router` to get the fees along a given path
Pass the failing/succeeding `Path` to PendingOutboundPayment meths
This will make the next commit much simpler
Track the amount spent on fees as payments are retried
Especially once we merge the `InvoicePayer` logic soon, we'll want
to expose the total fee paid in the `PaymentSent` event.
Correct send-bounding logic in `TestRoutingMessageHandler`
The `cmp::min` appeared to confused `end` for a count.
Add `PeerManager::disconnect_all_peers` to avoid complexity in BP
In the coming commits simply calling `timer_tick_occurred` will no
longer disconnect all peers, so its helpful to have a utility
method.
Constify the ratio in buf limits between forward and init sync msgs
Util-ify enqueueing an encoded message in peer_handler
This marginally simplifies coming commits.
Give peers which are sending us messages longer to respond to ping
See comment for rationale.
Give peers one timer tick to finish handshake before disconnecting
This ensures we don't let a hung connection stick around forever if
the peer never completes the initial handshake.
This also resolves a race where, on receiving a second connection
from a peer, we may reset their_node_id to None to prevent sending
messages even though the `channel_encryptor`
`is_ready_for_encryption()`. Sending pings only checks the
`channel_encryptor` status, not `their_node_id` resulting in an
`unwrap` on `None` in `enqueue_message`.
Log peer public key more thoroughly when logging in peer_handler
Notify scorer of failing payment path and channel
Upon receiving a PaymentPathFailed event, the failing payment may be
retried on a different path. To avoid using the channel responsible for
the failure, a scorer should be notified of the failure before being
used to find a new route.
Add a payment_path_failed method to routing::Score and call it in
InvoicePayer's event handler. Introduce a LockableScore parameterization
to InvoicePayer so the scorer is locked only once before calling
find_route.
Penalize failed channels in Scorer
As payments fail, the channel responsible for the failure may be
penalized. Implement Scorer::payment_path_failed to penalize the failed
channel using a configured penalty. As time passes, the penalty is
reduced using exponential decay, though penalties will accumulate if the
channel continues to fail. The decay interval is also configurable.
Test InvoicePayer in BackgroundProcessor
Proof of concept showing InvoicePayer can be used with an
Arc<ChannelManager> passed to BackgroundProcessor. Likely do not need to
merge this commit.
Move PaymentId to a [u8; 32] in bindings as for other hash objects
This should allow us to fix
https://github.com/lightningdevkit/ldk-garbagecollected/issues/52
Provide payment retry data when an MPP payment failed partially
This will allow `InvoicePayer` to properly retry payments that only
partially failed to send.
Expand `InvoicePayer` documentation somewhat to clarify edge-cases
Dont unwrap `RouteParameter::expiry_time` as users can set it
Users can provide anything they want as `RouteParameters` so we
shouldn't assume any fields are set any particular way, including
`expiry_time` set at all.
Rewrite InvoicePayer retry to correctly handle MPP partial failures
This rewrites a good chunk of the retry logic in `InvoicePayer` to
address two issues:
* it was not considering the return value of `send_payment` (and
`retry_payment`) may indicate a failure on some paths but not
others,
* it was not considering that more failures may still come later
when removing elements from the retry count map. This could
result in us seeing an MPP-partial-failure, failing to retry,
removing the retries count entry, and then retrying other parts,
potentially forever.
Add an integration test for InvoicePayer paying when one part fails
This tests the multi-part-single-failure-immediately fixes in the
previous commit.
Add integration test for InvoicePayerretry on an immediate failure
Check for invoice expiry in InvoicePayer before we send any HTLCs
Parameterize NetGraphMsgHandler with NetworkGraph
NetworkGraph is owned by NetGraphMsgHandler, but DefaultRouter requires
a reference to it. Introduce shared ownership to NetGraphMsgHandler so
that both can use the same NetworkGraph.
Make NetGraphMsgHandler::network_graph private
Since NetworkGraph has shared ownership, NetGraphMsgHandler does not
need to expose its field.
Clarify Scorer docs around penalizing channels
Refactor channel failure penalty logic
Move channel failure penalty logic into a ChannelFailure abstraction.
This encapsulates the logic for accumulating penalties and decaying them
over time. It also is responsible for the no-std behavior. This cleans
up Scorer and will make it easier to serialize it.
Parameterize Scorer by a Time trait
Scorer uses time to determine how much to penalize a channel after a
failure occurs. Parameterizing it by time cleans up the code such that
no-std support is in a single AlwaysPresent struct, which implements the
Time trait. Time is implemented for std::time::Instant when std is
available.
This parameterization also allows for deterministic testing since a
clock could be devised to advance forward as needed.
Implement (de)serialization for Scorer
Scorer should be serialized to retain penalty data between restarts.
Implement (de)serialization for Scorer by serializing last failure times
as duration since the UNIX epoch. For no-std, the zero-Duration is used.
Remove trailing ;s from macro calls to silence new rustc warnings
Rename Payee::new to Payee::from_node_id to clarify it somewhat
This also differentiates it from the bindings default-constructed
`new` method which is constructed when all fields are exposed and
of mappable types.
Make payment_path_failed path type bindings-mappable
The bindings don't currently support passing `Vec`s of objects
which it mappes as "opaque types". This is because it will require
clones to convert its own list of references to Rust's list of
objects.
In the near future we should resolve this limitation, allowing us
to revert this (and make `find_route`'s method signature similarly
cleaner), but for now we must avoid `Vec<OpaqueType>`.
Remove now-unused import in routing/mod.rs
Add `(C-not exported)` tag to a `Payee` modifier with move semantics
This matches the other `Payee` move-modifier functions.
Add `(C-not exported)` tags as required in tuple types
This prepares us for C bindings auto-exporting tuple type fields.
Tweak serialization of ScorerUsingTime for better forward compat
Update CHANGELOG for 0.0.103
Bump crate versions to 0.0.103/invoice 0.11
Fix `cargo doc` on older rustc
Apparently at least rustc 1.48 doesn't support `Self` in doc links,
so we make it explicit.
Add a ChannelTypeFeatures features object for the new channel_type
Its semantics are somewhat different from existing features,
however not enough to merit a different struct entirely.
Specifically, it only supports required features (if you send a
channel_type, the counterparty has to accept it wholesale or try
again, it cannot select only a subset of the flags) and it is
serialized differently (only appearing in TLVs).
Support de/ser of the new channel_type field in open_channel
Support send/recv'ing the new channel_type field in open_channel
This implements the channel type negotiation, though as we currently
only support channels with only static_remotekey set, it doesn't
implement the negotiation explicitly.
0.0.103 CHANGELOG tweaks from Jeff
Add note about PaymentId fields to 0.0.103 changelog
Add SinceEpoch time to test Scorer hermetically
In order to test Scorer hermetically, sleeps must be avoided. Add a
SinceEpoch abstraction for manually advancing time. Implement the Time
trait for SinceEpoch so that it can be used with ScorerUsingTime in
tests.
Add unit tests for Scorer
Test basic and channel failure penalties, including after a
(de-)serialization round trip.
Log before+after ChannelMonitor/Manager updates for visibility
I realized on my own node that I don't have any visibility into how
long a monitor or manager persistence call takes, potentially
blocking other operations. This makes it much more clear by adding
a relevant log_trace!() print immediately before and immediately
after persistence.
Fix to_remote output redeemscript when anchors enabled
Renamed script_for_p2wpkh to get_p2wpkh_redeemscript to match convention
Fix a minor memory leak on PermanentFailure mon errs when sending
If we send a payment and fail to update the first-hop channel state
with a `PermanentFailure` ChannelMonitorUpdateErr, we would have an
entry in our pending payments map, but possibly not return the
PaymentId back to the user to retry the payment, leading to a (rare
and relatively minor) memory leak.
Use upstream rust-bitcoin's dust calculation instead of our own
Not only does this move to common code, but it fixes handling of
all output types except for a few trivial cases.
Be less aggressive in outbound HTLC CLTV timeout checks
We currently assume our counterparty is naive and misconfigured and
may force-close a channel to get an HTLC we just forwarded them.
There shouldn't be any reason to do this - we don't have any such
bug, and we shouldn't start by assuming our counterparties are
buggy. Worse, this results in refusing to forward payments today,
failing HTLCs for largely no reason.
Instead, we keep a fairly conservative check, but not one which
will fail HTLC forwarding spuriously - testing only that the HTLC
doesn't expire for a few blocks from now.
Fixes #1114.
Correct Channel type serialization logic
Currently, we write out the Channel's `ChannelTypeFeatures` as an
odd type, implying clients which don't understand the
`ChannelTypeFeatures` field can simply ignore it. This is obviously
nonsense if the channel type is some future version - the client
needs to fail to deserialize as it doesn't understand the channel's
type.
We adapt the serialization logic here to only write out the
`ChannelTypeFeatures` field if it is something other than
only-static-remote-key, and simply consider that "default" (as it
is the only supported type today). Then, we write out the channel
type as an even TLV, implying clients which do not understand it
must fail to read the `Channel`.
Note that we do not need to bother reserving the TLV type no longer
written as it never appeared in a release (merged post-0.0.103).
Refactor InvoicePayer for spontaneous payments
To support spontaneous payments, InvoicePayer's sending logic must be
invoice-agnostic. Refactor InvoicePayer::pay_invoice_internal such that
invoice-specific code is in pay_invoice_using_amount and the remaining
logic is in pay_internal.
Further refactor the code's payment_cache locking such that it is
accessed consistently when needed, and tidy up the code a bit.
Support spontaneous payments in InvoicePayer
InvoicePayer handles retries not only when handling PaymentPathFailed
events but also for some types of PaymentSendFailure on the initial
send. Expand InvoicePayer's interface with a pay_pubkey function for
spontaneous (keysend) payments. Add a send_spontaneous_payment function
to the Payer trait to support this and implement it for ChannelManager.
Replace expect_value_msat with expect_send
Modify all InvoicePayer unit tests to use expect_send instead of
expect_value_msat, since the former can discern whether the send was for
an invoice, spontaneous payment, or a retry. Updates tests to set payer
expectations if they weren't already and assert these before returning a
failure.
Test retrying payment on partial send failure
Add some test coverage for when InvoicePayer retries within pay_invoice
because the Payer returned a partial failure.
Add PaymentHash parameter to Router::find_route
Implementations of Router may need the payment hash in order to look up
pre-computed routes from a probe for a given payment. Add a PaymentHash
parameter to Router::find_route to allow for this.
Provide `Score` the HTLC amount and channel capacity
This should allow `Score` implementations to make substantially
better decisions, including of the form "willing to pay X to avoid
routing over this channel which may have a high failure rate".
Penalize large HTLCs relative to channels in default `Scorer`
Sending HTLCs which are any greater than a very small fraction of the
channel size tend to fail at a much higher rate. Thus, by default
we start applying a penalty at only 1/8th the channel size and
increase it linearly as the amount reaches the channel's capacity,
20 msat per 1024th of the channel capacity.
Move `Score` into a `scoring` module instead of a top-level module
Traits in top-level modules is somewhat confusing - generally
top-level modules are just organizational modules and don't contain
things themselves, instead placing traits and structs in
sub-modules. Further, its incredibly awkward to have a `scorer`
sub-module, but only have a single struct in it, with the relevant
trait it is the only implementation of somewhere else. Not having
`Score` in the `scorer` sub-module is further confusing because
it's the only module anywhere that references scoring at all.
Return `ClosureReason` from `Channel` chain update methods
This fixes a few `ClosureReason`s and allows us to have
finer-grained user-visible errors when a channel closes due to an
on-chain event.
Automatically close channels that go unfunded for 2016 blocks
As recommended by BOLT 2 added in
https://github.com/lightningnetwork/lightning-rfc/pull/839
Add 'accept_inbound_channels' config option.
Limit minimum output size to the dust limit when RBF-bumping
Check all outputs meet the dust threshold in check_spends!()
Correct txid logging to reverse bytes.
We also take this opportunity to log the channel being closed when
one is closed by an on-chain spend of the funding output.
Ensure current channel state is logged for all channels on startup
Remove user_payment_id
In upcoming commits, we'll be making the payment secret and payment hash/preimage
derivable from info about the payment + a node secret. This means we don't
need to store any info about incoming payments and can eventually get rid of the
channelmanager::pending_inbound_payments map.
Add a new log-level for gossip messages.
Fix MPP routefinding when we first collect 95% of payment value
See comment in new test for more details.
Introduce new helper commit_tx_fee_sat
Cancel the outbound feerate update if above what we can afford
Check we won't overflow `max_dust_htlc_exposure_msat` at outbound feerate update
Re-add `test_max_dust_htlc_exposure`
Introduce CommitmentStats
Check outbound update_fee affordance incremented with holding cell HTLCs
Add anchor support to commitment HTLC outputs
Increase visibility of anchor related methods
Add anchor support to build_htlc_transaction
Adjust HTLC_{SUCCESS,TIMEOUT}_TX_WEIGHT when anchors used
Add test vectors for get_htlc_redeemscript wrt anchors
Generate PaymentPathSuccessful event for each path
A single PaymentSent event is generated when a payment is fulfilled.
This is occurs when the preimage is revealed on the first claimed HTLC.
For subsequent HTLCs, the event is not generated.
In order to score channels involved with a successful payments, the
scorer must be notified of each successful path involved in the payment.
Add a PaymentPathSuccessful event for this purpose. Generate it whenever
a part is removed from a pending outbound payment. This avoids duplicate
events when reconnecting to a peer.
Make Channel::commit_tx_fee_msat static and take fee explicitly
This may avoid risk of bugs in the future as it requires the caller
to think about the fee being used, not just blindly use the current
(committed) channel feerate.
Rewrite test_update_fee_that_funder_cannot_afford to avoid magic
Instead of magic hard-coded constants, its better for tests to
derive the values used so that they change if constants are changed
and so that it is easier to re-derive constants in the future as
needed.
Correct initial commitment tx fee affordability checks on open
Previously, we would reject inbound channels if the funder wasn't
able to meet our channel reserve on their first commitment
transaction only if they also failed to push enough to us for us
to not meet their initial channel reserve as well.
There's not a lot of reason to care about us meeting their reserve,
however - its largely expected that they may not push enough to us
in the initial open to meet it, and its not actually our problem if
they don't.
Further, we used our own fee, instead of the channel's actual fee,
to calculate fee affordability of the initial commitment
transaction.
We resolve both issues here, rewriting the combined affordability
check conditionals in inbound channel open handling and adding a
fee affordability check for outbound channels as well.
The prior code may have allowed a counterparty to start the channel
with "no punishment" states - violating the reason for the reserve
threshold.
Test fixed channel reserve checks on channel open
Fix compilation with the max_level_trace feature
Test all log-limiting features in CI
Support `logger::Record` in C by String-ing the fmt::Arguments
This adds a new (non-feature) cfg argument `c_bindings` which will
be set when building C bindings. With this, we can (slightly) tweak
behavior and API based on whether we are being built for Rust or C
users.
Ideally we'd never need this, but as long as we can keep the API
consistent-enough to avoid material code drift, this gives us a
cheap way of doing the "right" thing for both C and Rust when the
two are in tension.
We also move lightning-background-processor to support the same
MSRV as the main lightning crate, instead of only
lightning-net-tokio's MSRV.
Make `Score : Writeable` in c_bindings and impl on `LockedScore`
Ultimately we likely need to wrap the locked `Score` in a struct
that exposes writeable somehow, but because all traits have to be
fully concretized for C bindings we'll still need `Writeable` on
all `Score` in order to expose `Writeable` on the locked score.
Otherwise, we'll only have a `LockedScore` with a `Score` visible
that only has the `Score` methods, never the original type.
Seal `scoring::Time` and only use `Instant` or `Eternity` publicly
`scoring::Time` exists in part to make testing the passage of time
in `Scorer` practical. To allow no-std users to provide a time
source it was exposed as a trait as well. However, it seems
somewhat unlikely that a no-std user is going to have a use for
providing their own time source (otherwise they wouldn't be a
no-std user), and likely they won't have a graph in memory either.
`scoring::Time` as currently written is also exceptionally hard to
write C bindings for - the C bindings trait mappings relies on the
ability to construct trait implementations at runtime with function
pointers (i.e. `dyn Trait`s). `scoring::Time`, on the other hand,
is a supertrait of `core::ops::Sub` which requires a `sub` method
which takes a type parameter and returns a type parameter. Both of
which aren't practical in bindings, especially given the
`Sub::Output` associated type is not bound by any trait bounds at
all (implying we cannot simply map the `sub` function to return an
opaque trait object).
Thus, for simplicity, we here simply seal `scoring::Time` and make
it effectively-private, ensuring the bindings don't need to bother
with it.
Implement Clone, Hash, PartialEq for ClosingTransaction
This is a public struct intended to be used as an object by users,
so it should likely have common implementations, given they're
trivial.
Implement Clone for InvalidShutdownScript
Users hopefully shouldn't have much of a reason to use this, but
the bindings may need it to ensure no leaking pointers over an ffi.
Store holder channel reserve and max-htlc-in-flight explicitly
Previously, `holder_selected_channel_reserve_satoshis` and
`holder_max_htlc_value_in_flight_msat` were constant functions
of the channel value satoshis. However, in the future we may allow
allow users to specify it. In order to do so, we'll need to track
them explicitly, including serializing them as appropriate.
We go ahead and do so here, in part as it will make testing
different counterparty-selected channel reserve values easier.
Explicitly support counterparty setting 0 channel reserve
A peer providing a channel_reserve_satoshis of 0 (or less than our
dust limit) is insecure, but only for them. Because some LSPs do it
with some level of trust of the clients (for a substantial UX
improvement), we explicitly allow it. Because its unlikely to
happen often in normal testing, we test it explicitly here.
Fix regression when reading `Event::PaymentReceived` in some cases
For some reason rustc was deciding on a type for the `Option` being
deserialized for us as `_user_payment_id`. This really, really,
absolutely should have been a compile failure - the type (with
methods called on it!) was ambiguous! Instead, rustc seems to have
been defaulting to `Option<()>`, causing us to read zero of the
eight bytes in the `user_payment_id` field, which returns an
`Err(InvalidValue)` error as TLVs must always be read fully.
This should likely be reported to rustc as its definitely a bug,
but I cannot seem to cause the same error on any kinda of
vaguely-minimized version of the same code.
Found by `chanmon_consistency` fuzz target.
Fix compilation in `payment` rustdoc examples
The samples were not valid rust, but previous versions of rustc had
a bug where they were accepted anyway. Latest rustc beta no longer
accepts these.
Score successful payment paths
Expand the Score trait with a payment_path_successful function for
scoring successful payment paths. Called by InvoicePayer's EventHandler
implementation when processing PaymentPathSuccessful events. May be used
by Score implementations to revert any channel penalties that were
applied by calls to payment_path_failed.
Decay channel failure penalty upon success
If a payment failed to route through a channel, a penalty is applied to
the channel in the future when finding a route. This penalty decays over
time. Immediately decay the penalty by one half life when a payment is
successfully routed through the channel.
Fix shift overflow in Scorer::channel_penalty_msat
An unchecked shift of more than 64 bits on u64 values causes a shift
overflow panic. This may happen if a channel is penalized only once and
(1) is not successfully routed through and (2) after 64 or more half
life decays. Use a checked shift to prevent this from happening.
Allow missing-docs on test-only macros
Prefer fully-specified paths in test macros
This avoids macros being context-specific use-dependent.
Continue after a single failure in `ChannelMonitor::update_monitor`
`ChannelMonitorUpdate`s may contain multiple updates, including, eg
a payment preimage after a commitment transaction update. While
such updates are generally not generated today, we shouldn't return
early out of the update loop, causing us to miss any updates after
an earlier update fails.
Always return failure in `update_monitor` after funding spend
Previously, monitor updates were allowed freely even after a
funding-spend transaction confirmed. This would allow a race
condition where we could receive a payment (including the
counterparty revoking their broadcasted state!) and accept it
without recourse as long as the ChannelMonitor receives the block
first, the full commitment update dance occurs after the block is
connected, and before the ChannelManager receives the block.
Obviously this is an incredibly contrived race given the
counterparty would be risking their full channel balance for it,
but its worth fixing nonetheless as it makes the potential
ChannelMonitor states simpler to reason about.
The test in this commit also tests the behavior changed in the
previous commit.
Drop `MonitorUpdateErr` in favor of opaque errors.
We don't expect users to ever change behavior based on the string
contained in a `MonitorUpdateErr`, except log it, so there's little
reason to not just log it ourselves and return a `()` for errors.
We do so here, simplifying the callsite in `ChainMonitor` as well.
Make APIError debug output more clear by including the variant
Add a simple test for ChainMonitor MonitorUpdate-holding behavior
Add a test for MonitorEvent holding when they complete out-of-order
Add trivial test of monitor update failure during block connection
Remove OnionV2 parsing support
OnionV2s don't (really) work on Tor anymore anyway, and the field
is set for removal in the BOLTs [1]. Sadly because of the way
addresses are parsed we have to continue to understand that type 3
addresses are 12 bytes long. Thus, for simplicity we keep the
`OnionV2` enum variant around and just make it an opaque 12 bytes,
with the documentation updated to note the deprecation.
[1] https://github.com/lightning/bolts/pull/940
Ensure ChannelManager methods are idempotent
During event handling, ChannelManager methods may need to be called as
indicated in the Event documentation. Ensure that these calls are
idempotent for the same event rather than panicking. This allows users
to persist events for later handling without needing to worry about
processing the same event twice (e.g., if ChannelManager is not
persisted but the events were, the restarted ChannelManager would return
some of the same events).
Define public getters for all feature flags
There's not a ton of reason not to do this, and moving it to the
macro removes some code.
Support the `channel_type` feature bit.
Note that this feature bit does absolutely nothing. We signal it
(as we already support channel type negotiation), but do not bother
to look to see if peers support it, as we don't care - we simply
look for the TLV entry and deduce if a peer supports channel type
negotiation from that.
The only behavioral change at all here is that we don't barf if a
peer sets channel type negotiation to required via the feature bit
(instead of failing the channel at open-time), but of course no
implementations do this, and likely won't for some time (if ever -
you can simply fail channels with unknown types later, and there's
no reason to refuse connections, really).
As defined in https://github.com/lightning/bolts/pull/906
Reduce force-closures with user fee estimators which round poorly
See comment for more
Upgrade to codecov uploader v2
Some time ago codecov stopped supporting their old v1 uploader, and
it seems they've now finally turned it off, so we aren't getting
any coverage reports anymore. Hopefully upgrading is pretty trivial.
Getter for the total channel balance
The existing balance getters subtract reserve, this one does not.
Separate ChannelAnnouncement and ChannelUpdate broadcast conditions
When a `ChannelUpdate` message is generated for broadcast as a part
of a `BroadcastChannelAnnouncement` event, it may be newer than our
previous `ChannelUpdate` and need to be broadcast. However, if the
`ChannelAnnouncement` had already been seen we wouldn't
re-broadcast either message as the `handle_channel_announcement`
call would fail, short-circuiting the condition to broadcast both.
Instead, we split the broadcast of each message as well as the
conditional so that we always attempt to handle each message and
update our local graph state, then broadcast the message if its
update was processed successfully.
Update `ChannelUpdate::timestamp` when channels are dis-/en-abled
We update the `Channel::update_time_counter` field (which is copied
into `ChannelUpdate::timestamp`) only when the channel is
initialized or closes, and when a new block is connected. However,
if a peer disconnects or reconnects, we may wish to generate
`ChannelUpdate` updates in between new blocks. In such a case, we
need to make sure the `timestamp` field is newer than any previous
updates' `timestamp` fields, which we do here by simply
incrementing it when the channel status is changed.
As a side effect of this we have to update
`test_background_processor` to ensure it eventually succeeds even
if the serialization of the `ChannelManager` changes after the test
begins.
Re-broadcast our own gossip even if its same as the last broadcast
Even if our gossip hasn't changed, we should be willing to
re-broadcast it to our peers. All our peers may have been
disconnected the last time we broadcasted it.
Add a comment describing `update_time_counter` and when its updated
Add a comment describing the timestamp field's use
DRY up payment failure macros in functional_test_utils
... with a more extensible expectation-checking framework for them.
Add a variant to `PendingOutboundPayment` for retries-exceeded
When a payer gives up trying to retry a payment, they don't know
for sure what the current state of the event queue is.
Specifically, they cannot be sure that there are not multiple
additional `PaymentPathFailed` or even `PaymentSuccess` events
pending which they will see later. Thus, they have a very hard
time identifying whether a payment has truly failed (and informing
the UI of that fact) or if it is still pending. See [1] for more
information.
In order to avoid this mess, we will resolve it here by having the
payer give `ChannelManager` a bit more information - when they
have given up on a payment - and using that to generate a
`PaymentFailed` event when all paths have failed.
This commit adds the neccessary storage and changes for the new
state inside `ChannelManager` and a public method to mark a payment
as failed, the next few commits will add the new `Event` and use
the new features in our `PaymentRetrier`.
[1] https://github.com/lightningdevkit/rust-lightning/issues/1164
Expose an event when a payment has failed and retries complete
When a payment fails, a payer needs to know when they can consider
a payment as fully-failed, and when only some of the HTLCs in the
payment have failed. This isn't possible with the current event
scheme, as discovered recently and as described in the previous
commit.
This adds a new event which describes when a payment is fully and
irrevocably failed, generating it only after the payment has
expired or been marked as expired with
`ChannelManager::mark_retries_exceeded` *and* all HTLCs for it
have failed. With this, a payer can more simply deduce when a
payment has failed and use that to remove payment state or
finalize a payment failure.
Use `Event::PaymentFailed` in `InvoicePayer` to remove retry count
This finally fixes the bug described in the previous commits where
we retry a payment after its retry count has expired due to early
removal of the payment from the retry count tracking map. A test is
also added which demonstrates the bug in previous versions and
which passes now.
Fixes #1164.
Make attempting to retry a succeeded payment an APIError, not Route
This is symmetric with the new failure once a payment is abandoned.
Updates Fee estimator docs to include a max return value for get_est_sat_per_1000_weight
Updates docs to include a max return value for get_est_sat_per_1000_weight
Adds max to both conversions
Additional detail
Add mpp_timeout and invalid_onion_payload descriptions & handling
Pin tokio to 1.14.0 in CI for older rustc's
Build no-std on 1.47 now that core2 supports it
DRY up network_graph tests substantially with message creation fns
Drop `allow_wallclock_use` feature in favor of simply using `std`
Fixes #1147.
Add a method to prune stale channels from `NetworkGraph`
We define "stale" as "haven't heard an updated channel_update in
two weeks", as described in BOLT 7.
We also filter out stale channels at write-time for `std` users, as
we can look up the current time.
Reject channel_update messages with timestamps too old or new
Because we time out channel info that is older than two weeks now,
we should also reject new channel info that is older than two
weeks, in addition to rejecting future channel info.
Automatically prune NetworkGraph of stale channels hourly in BP
Add get_inbound_payment_key_material to KeysInterface
This will allow us to retrieve key material for encrypting/decrypting inbound
payment info, in upcoming commits
Macro-ize checking that the total value of an MPP's parts is sane
This DRY-ed code will be used in upcoming commits when we stop storing inbound
payment data
Add get_single_block to chacha20 module
In the next commit, we'll want to get a single block from a chacha stream that
takes a 16-byte nonce.
Drop need to store pending inbound payments
and replace payment_secret with encrypted metadata
See docs on `inbound_payment::verify` for details
Also add min_value checks to all create_inbound_payment* methods
Add new invoice CreationError::InvalidAmount for use in checking `create_inbound_payment`
in an invoice creation utility. Note that if the error type of `create_inbound_payment` ever
changed, we'd be forced to update the invoice utility's callsite to handle the new error
Salt inbound payment ExpandedKey
Leftover feedback from #1177
create_inbound_payment: warn about dup hashes
Leftover feedback from #1177
inbound_payment: DRY verify method for use in getting preimage
in the next commit(s)
inbound_payment: Add utility to get payment preimage given hash/secret
User-requested feature
Remove trailing whitespace in the FeeEstimator interface
Update CHANGELOG for 0.0.104
Bump versions to 0.0.104/invoice 0.12
Swap around generic argument ordering in DefaultRouter for bindings
The bindings generation really should support default generic types
in where clauses, but currently does not. To avoid needing to add
support during the current release process, we simply swap around
the arguments to move them to the first <> instead of the where.
Add a constructor to MultiThreadedLockableScore
...as otherwise the struct is rather useless.
Add a C-not exported tag to `NetGraphMsgHandler.network_graph`
Swap around generic argument ordering in InvoicePayer for bindings
The bindings generation really should support generic bounds other
than Deref::Target in where clauses, but currently does not. To
avoid needing to add support during the current release process,
we simply swap around the arguments to move them to the first <>
instead of the where.
update repo name to use lightningdevkit
Log gossip rejections due to stale channel_updates at GOSSIP level
This further reduces noise at the TRACE level during initial gossip
sync.
Support building `cfg=c_bindings` with `no-std`
This will be needed for JavaScript bindings eventually.
Adapt lightning-invoice to no_std
Do not turn on bech32/std by default for lightning-invoice
Add a new `WarningMessage` message to send and receive warnings
Handle sending and receiving warning messages
Send warning messages when appropriate in gossip handling pipeline
Convert `shutdown` invalid script checks to warning messages
As required by the warning messages PR, we should simply warn our
counterparty in this case and let them try again, continuing to try
to use the channel until they tell us otherwise.
Send `warning` instead of `error` when we incounter bogus gossip
Rely on Error/Warning message data lengths being correct
In https://github.com/lightning/bolts/pull/950, the (somewhat
strange) requirement that error messages be handled even if the
length field is set larger than the size of the package was
removed. Here we change the code to drop the special handling for
this, opting to just fail to read the message if the length is
incorrect.
Set the SigHashType of remote htlc signatures w/ anchors to SinglePlusAnyoneCanPay
Isolated channelmonitor weight unit tests and added anchor loops
make WEIGHT{_REVOKED,}_{OFFERED,RECEIVED}_HTLC functions with opt_anchors parameter
Add unit test coverage for package::weight_{offered,received}_htlc
Convert COMMITMENT_TX_BASE_WEIGHT to anchor-aware function
Convert HTLC_{SUCCESS,TIMEOUT}_TX_WEIGHT to anchor-aware functions
Debit funder's output to cover anchors
Add anchor tests to outbound_commitment_test
Set opt_anchors for calls to CommitmentTransaction::new_with_auxiliary_htlc_data
Fixed comment on weight_received_htlc
Make lockorder consistent in channelmanager
This resolves a lockorder inversion in
`ChannelManager::finalize_claims` where `pending_outbound_payments`
is locked after `pending_events`, opposite of, for example, the
lockorder in `ChannelManager::fail_htlc_backwards_internal` where
`pending_outbound_payments` is locked at the top of the
`HTLCSource::OutboundRoute` handling and then `pending_events` is
locked at the end.
Fix some (non-bug) lock-order-inversions in tests
Check lockorders in tests with a trivial lockorder tracker
Fix build errors
Create script using p2wsh for comparison
Using p2wpkh for generating the payment script
spendable_outputs sanity check
vss96
added a commit
to vss96/rust-lightning
that referenced
this pull request
Jan 19, 2022
Add methods to count total fees and total amount in a Route #999
* Added `get_total_fees` method to route,
to calculate all the fees paid accross each path.
* Added `get_total_amount` method to route,
to calculate the total of actual amounts paid in each path.
Update docs to specify where process events is called
Add Event::ChannelClosed generation at channel shutdown
Rename MonitorEvent::CommitmentTxBroadcasted to CommitmentTxConfirmed
Extend MsgHandleErrInternal with a new chan_id field Option<[u8; 32]>
This field is used in next commit to generate appropriate
ChannelClosed event at `handle_error()` processing.
Add ChannelClosed generation at cooperative/force-close/error processing
When we detect a channel `is_shutdown()` or call on it
`force_shutdown()`, we notify the user with a Event::ChannelClosed
informing about the id and closure reason.
Add `pending_events` deadlock detection in `handle_error`
Bump HTTP read timeout to match reality of Bitcoin Core blocking
Fix future unknown `Event` variant backwards compatibility
In 8ffc2d1742ff1171a87b0410b21cbbd557ff8247, in 0.0.100, we added
a backwards compatibility feature to the reading of `Event`s - if
the type was unknown and odd, we'd simply ignore the event and
treat it as no event. However, we failed to read the
length-prefixed TLV stream when doing so, resulting in us reading
some of the skipped-event data as the next event or other data in
the ChannelManager.
We fix this by reading the varint length prefix written, then
skipping that many bytes when we come across an unknown odd event
type.
Fix a panic in Route's new fee-calculation methods and clean up
This addresses Val's feedback on the new Route fee- and
amount-calculation methods, including fixing the panic she
identified and cleaning up various docs and comments.
Adds Transaction to lighting-block-sync::convert
Includes disclaimer in docs, see https://github.com/rust-bitcoin/rust-lightning/pull/1061#issuecomment-911960862
Rename PaymentFailed -> PaymentPathFailed
Since we don't want to imply to users that a payment has
completely failed when it really has just partially
failed
Add path field to PaymentPathFailed event
Fix windows-only test failure added in #997
This is a trivial bugfix to add a missing test updated required in
PR 997.
Move trait bounds on `wire::Type` from use to the trait itself
`wire::Type` is only (publicly) used as the `CustomMessage`
associated type in `CustomMessageReader`, where it has additional
trait bounds on `Debug` and `Writeable`. The documentation for
`Type` even mentions that you need to implement `Writeable` because
this is the one place it is used.
To make this more clear, we move the type bounds onto the trait
itself and not on the associated type.
This is also the only practical way to build C bindings for `Type`
as we cannot have a concrete, single, `Type` struct in C which only
optionally implements various subtraits, at least not without
runtime checking of the type bounds.
Make `ChainMonitor::get_claimable_balances` take a slice of refs
For the same reason as `get_route`, a slice of objects isn't
practical to map to bindings - the objects in the bindings space
are structs with a pointer and some additional metadata. Thus, to
create a slice of them, we'd need to take ownership of the objects
behind the pointer, place them into a slace, and then restore them
to the pointer.
This would be a lot of memory copying and marshalling, not to
mention wouldn't be thread-safe, which the same function otherwise
would be if we used a slice of references instead of a slice of
objects.
Use Infallible for the unconstructable default custom message type
When we landed custom messages, we used the empty tuple for the
custom message type for `IgnoringMessageHandler`. This was fine,
except that we also implemented `Writeable` to panic when writing
a `()`. Later, we added support for anchor output construction in
CommitmentTransaction, signified by setting a field to `Some(())`,
which is serialized as-is.
This causes us to panic when writing a `CommitmentTransaction`
with `opt_anchors` set. Note that we never set it inside of LDK,
but downstream users may.
Instead, we implement `Writeable` to write nothing for `()` and use
`core::convert::Infallible` for the default custom message type as
it is, appropriately, unconstructable.
This also makes it easier to implement various things in bindings,
as we can always assume `Infallible`-conversion logic is
unreachable.
Drop redundant generic bounds when the trait requires the bounds
Make method time on trait impl explitit to help bindings generator
Associated types in C bindings is somewhat of a misnomer - we
concretize each trait to a single struct. Thus, different trait
implementations must still have the same type, which defeats the
point of associated types.
In this particular case, however, we can reasonably special-case
the `Infallible` type, as an instance of it existing implies
something has gone horribly wrong.
In order to help our bindings code figure out how to do so when
referencing a parent trait's associated type, we specify the
explicit type in the implementation method signature.
Update CHANGELOG for 0.0.101
Bump Crate versions to 0.0.101 (and invoice to 0.9)
Make `NetworkGraph` Clone-able again
There isn't a lot of user-utility for cloning `NetworkGraph`
directly (its a rather large struct, and there probably isn't a lot
of reason to have *multiple* `NetworkGraph`s). Thus, when locks
were pushed down into it, the `Clone`-ability of it was dropped as
well.
Sadly, mapping the Java memory model onto:
* `Read`-ing a `NetworkGraph`, creating a Java-owned
`NetworkGraph` object that the JVM will destruct for us,
* Passing it to a `NetGraphMsgHandler`, which now expects to own
the `NetworkGraph`, including destructing it,
isn't really practical without adding a clone in between.
Given this, and the fact that there's nothing inherently wrong with
clone-ing a `NetworkGraph`, we simply re-add `Clone` here.
Drop broken test that is unfixable due to being undocumented
This should be reverted at some point, but the test is deficient
and breaks on later changes that are important to land ASAP.
Increase our default/minimum dust limit to 354 sat/vbytes
330 sat/vbyte, the current value, is not sufficient to ensure a
future segwit script longer than 32 bytes meets the dust limit if
used for a shutdown script. Thus, we can either check the value
on shutdown or we can simply require segwit outputs and require a
dust value of no less than 354 sat/vbyte.
We swap the minimum dust value to 354 sat/vbyte here, requiring
segwit scripts in a future commit.
See https://github.com/lightningnetwork/lightning-rfc/issues/905
Reduce the maximum allowed counterparty dust limit to 546 sat/vbyte
546 sat/vbyte is the current default dust limit on most
implementations, matching the network dust limit for P2SH outputs.
Implementations don't currently appear to send any larger dust
limits, and allowing a larger dust limit implies higher payment
failure risk, so we'd like to be as tight as we can here.
Require user cooperative close payout scripts to be Segwit
There is little reason for users to be paying out to non-Segwit
scripts when closing channels at this point. Given we will soon, in
rare cases, force-close during shutdown when a counterparty closes
to a non-Segwit script, we should also require it of our own users.
Force-close channels if closing transactions may be non-standard
If a counterparty (or an old channel of ours) uses a non-segwit
script for their cooperative close payout, they may include an
output which is unbroadcastable due to not meeting the network dust
limit.
Here we check for this condition, force-closing the channel instead
if we find an output in the closing transaction which does not meet
the limit.
Rename MIN_DUST_LIMIT_SATOSHIS constant to disambiguate chan vs P2P
While channel and P2P network dust limits are related, they're
ultimately two different things, and thus their constant names
should reference that.
Regenerate PendingHTLCsForwardable on reload instead of serializing
When we are prepared to forward HTLCs, we generate a
PendingHTLCsForwardable event with a time in the future when the
user should tell us to forward. This provides some basic batching
of forward events, improving privacy slightly.
After we generate the event, we expect users to spawn a timer in
the background and let us know when it finishes. However, if the
user shuts down before the timer fires, the user will restart and
have no idea that HTLCs are waiting to be forwarded/received.
To fix this, instead of serializing PendingHTLCsForwardable events
to disk while they're pending (before the user starts the timer),
we simply regenerate them when a ChannelManager is deserialized
with HTLCs pending.
Fixes #1042
Don't apply monitor updates after watch_channel PermFail
The full stack fuzzer found an unreachable panic where we receive a
FundingSigned with a duplicate channel outpoint.
Update Watch docs to disallow dup channel outpoints
on watch_channel
Rename MppId to PaymentId
Leftover from previous PR Jeff feedback.
Useful in upcoming commits as we'll expose this to users for payment retries
Return PaymentId from send_*payment functions
Used in upcoming commits for retries
Refactor send_payment internals for retries
We want to reuse send_payment internal functions for retries,
so some need to now be parameterized by PaymentId to avoid
generating a new PaymentId on retry
Refactor send_payment internals for retries 2
Retrying a partial payment means send_payment_internal needs to be parameterized
by a total payment amount, else 'HTLC values do not match' errors
channelmanager: Add retry data to pending_outbound_payments
Add method to retry payments
Don't remove failed payments when all paths fail
This is because we want the ability to retry completely failed
payments.
Upcoming commits will remove these payments on timeout to prevent
DoS issues
Also test that this removal allows retrying single-path payments
Expire outbound payments after 3 blocks if no parts are pending
Correct step number in `get_route`
Consider many first-hop paths to the same counterparty in routing
Previously we'd simply overwritten "the" first hop path to each
counterparty when routing, however this results in us ignoring all
channels except the last one in the `ChannelDetails` list per
counterparty.
f readability improvements from val
Update Event::PaymentReceived docs since we require payment secret
Users no longer need to verify the amounts of received payments as
the payment secret will protect us against the probing attacks such
verification was intended to fix.
Move tests of payment retries into a new module
Move pending payment tracking to after the new HTLC flies
If we attempt to send a payment, but the HTLC cannot be send due to
local channel limits, we'll provide the user an error but end up
with an entry in our pending payment map. This will result in a
memory leak as we'll never reclaim the pending payment map entry.
Correct error returned when `retry_payment` doesn't have a payment
Add payment_hash to PaymentSent #999
Replace PublicKey with [u8; 33] in NetworkGraph
Adds DiscardFunding event
During the event of a channel close, if the funding transaction
is yet to be broadcasted then a DiscardFunding event is issued
along with the ChannelClose event.
Use local channel state when constructing routes in test macro
This is a bit more realistic and needed to route over non-public
channels.
Fix loop label shadowing warning
Remove special case for onion error expiry_too_far
With channel scoring and payment retries, it is no longer necessary to
have expiry_too_far imply a payment failure.
Pass hop index in construct_onion_keys_callback
This simplifies failing route hop calculation, which will be useful for
later identifying the failing hop for PaymentFailed events.
Clean up fee_insufficient computation
Add failing short channel id to PaymentPathFailed
This will be useful for scoring channels when a payment fails.
Expose ReadOnlyNetworkGraph::get_addresses to C by cloning result
We cannot expose ReadOnlyNetworkGraph::get_addresses as is in C as
it returns a list of references to an enum, which the bindings
dont support. Instead, we simply clone the result so that it
doesn't contain references.
Speed up test_timer_tick_called
Fix unused variable warnings in fuzzer
Replace get_route with get_route_and_payment_hash
The interface for get_route will change to take a scorer. Using
get_route_and_payment_hash whenever possible allows for keeping the
scorer inside get_route_and_payment_hash rather than at every call site.
Replace get_route with get_route_and_payment_hash wherever possible.
Additionally, update get_route_and_payment_hash to use the known invoice
features and the sending node's logger.
Move mpp_failure test to payment_tests.rs
Move `Persist` trait to chainmonitor as that's the only reference
Move ChannelMonitorUpdateErr to chain as it is a chain::Watch val
Make `ChainMonitor::monitors` private and expose monitor via getter
Exposing a `RwLock<HashMap<>>` directly was always a bit strange,
and in upcoming changes we'd like to change the internal
datastructure in `ChainMonitor`.
Further, the use of `RwLock` and `HashMap` meant we weren't able
to expose the ChannelMonitors themselves to users in bindings,
leaving a bindings/rust API gap.
Thus, we take this opportunity go expose ChannelMonitors directly
via a wrapper, hiding the internals of `ChainMonitor` behind
getters. We also update tests to use the new API.
Simplify channelmonitor tests which use chain::Watch and Persister
test_simple_monitor_permanent_update_fail and
test_simple_monitor_temporary_update_fail both have a mode where
they use either chain::Watch or persister to return errors.
As we won't be doing any returns directly from the chain::Watch
wrapper in a coming commit, the chain::Watch-return form of the
test will no longer make sense.
Handle Persister returning TemporaryFailure for new channels
Previously, if a Persister returned a TemporaryFailure error when
we tried to persist a new channel, the ChainMonitor wouldn't track
the new ChannelMonitor at all, generating a PermanentFailure later
when the updating is restored.
This fixes that by correctly storing the ChannelMonitor on
TemporaryFailures, allowing later update restoration to happen
normally.
This is (indirectly) tested in the next commit where we use
Persister to return all monitor-update errors.
Use Persister to return errors in tests not chain::Watch
As ChainMonitor will need to see those errors in a coming PR,
we need to return errors via Persister so that our ChainMonitor
chain::Watch implementation sees them.
Use Persister to return errors in fuzzers not chain::Watch
Fixed 'Advancing Bitcoin' video URL.
Add channel scoring to get_route
Failed payments may be retried, but calling get_route may return a Route
with the same failing path. Add a routing::Score trait used to
parameterize get_route, which it calls to determine how much a channel
should be penalized in terms of msats willing to pay to avoid the
channel.
Also, add a Scorer struct that implements routing::Score with a constant
constant penalty. Subsequent changes will allow for more robust scoring
by feeding back payment path success and failure to the scorer via event
handling.
Return the temporary channel id in success from `create_channel`
This makes it more practical for users to track channels prior to
funding, especially if the channel fails because the peer rejects
it for a parameter mismatch.
Include the user channel id counter in Event::ChannelClosed
This makes it more practical for users to track channels using
their own IDs, especially across funding.
Rename create_channel param to user_channel_id to standardize it
Add CHANGELOG entries for 0.0.102
Bump crate versions to 0.0.102 and lightning-invoice 0.10
Simplify prefers_shorter_route_with_higher_fees
In order to make the scoring tests easier to read, only check the
relevant RouteHop fields. The remaining fields are tested elsewhere.
Expand the test to show the path used without scoring.
Add source and target nodes to routing::Score
Expand routing::Score::channel_penalty_msat to include the source and
target node ids of the channel. This allows scorers to avoid certain
nodes altogether if desired.
Move MonitorEvent serialization to TLV-enum-upgradable from custom
Move the two-AtomicUsize counter in peer_handler to a util struct
We also take this opportunity to drop byte_utils::le64_to_array, as
our MSRV now supports the native to_le_bytes() call.
Move ChannelManager::monitor_updated to a MonitorEvent
In the next commit we'll need ChainMonitor to "see" when a monitor
persistence completes, which means `monitor_updated` needs to move
to `ChainMonitor`. The simplest way to then communicate that
information to `ChannelManager` is via `MonitorEvet`s, which seems
to line up ok, even if they're now constructed by multiple
different places.
Use an opaque type to describe monitor updates in Persist
In the next commit, we'll be originating monitor updates both from
the ChainMonitor and from the ChannelManager, making simple
sequential update IDs impossible.
Further, the existing async monitor update API was somewhat hard to
work with - instead of being able to generate monitor_updated
callbacks whenever a persistence process finishes, you had to
ensure you only did so at least once all previous updates had also
been persisted.
Here we eat the complexity for the user by moving to an opaque
type for monitor updates, tracking which updates are in-flight for
the user and only generating monitor-persisted events once all
pending updates have been committed.
Persist `ChannelMonitor`s after new blocks are connected
This resolves several user complaints (and issues in the sample
node) where startup is substantially delayed as we're always
waiting for the chain data to sync.
Further, in an upcoming PR, we'll be reloading pending payments
from ChannelMonitors on restart, at which point we'll need the
change here which avoids handling events until after the user
has confirmed the `ChannelMonitor` has been persisted to disk.
It will avoid a race where we
* send a payment/HTLC (persisting the monitor to disk with the
HTLC pending),
* force-close the channel, removing the channel entry from the
ChannelManager entirely,
* persist the ChannelManager,
* connect a block which contains a fulfill of the HTLC, generating
a claim event,
* handle the claim event while the `ChannelMonitor` is being
persisted,
* persist the ChannelManager (before the CHannelMonitor is
persisted fully),
* restart, reloading the HTLC as a pending payment in the
ChannelManager, which now has no references to it except from
the ChannelMonitor which still has the pending HTLC,
* replay the block connection, generating a duplicate PaymentSent
event.
Update test_dup_htlc_onchain_fails_on_reload for new persist API
ChannelMonitors now require that they be re-persisted before
MonitorEvents be provided to the ChannelManager, the exact thing
that test_dup_htlc_onchain_fails_on_reload was testing for when it
*didn't* happen. As such, test_dup_htlc_onchain_fails_on_reload is
now testing that we bahve correctly when the API guarantees are not
met, something we don't need to do.
Here, we adapt it to test the new API requirements through
ChainMonitor's calls to the Persist trait instead.
Always release `MonitorEvent`s to `ChannelManager` after 3 blocks
If we have a `ChannelMonitor` update from an on-chain event which
returns a `TemporaryFailure`, we block `MonitorEvent`s from that
`ChannelMonitor` until the update is persisted. This prevents
duplicate payment send events to the user after payments get
reloaded from monitors on restart.
However, if the event being avoided isn't going to generate a
PaymentSent, but instead result in us claiming an HTLC from an
upstream channel (ie the HTLC was forwarded), then the result of a
user delaying the event is that we delay getting our money, not a
duplicate event.
Because user persistence may take an arbitrary amount of time, we
need to bound the amount of time we can possibly wait to return
events, which we do here by bounding it to 3 blocks.
Thanks to Val for catching this in review.
Clarify the contexts in which persist_new_channel may be called
Its somewhat confusing that `persist_new_channel` is called on
startup for an existing channel in common deployments, so we call
it out explicitly.
Make `Channel::revoke_and_ack`'s return tuple a struct
This substantially improves readability at the callsite and in the
function.
Make `Channel::monitor_updating_restored`'s return tuple a struct
This improves readability at the callsite and in the function.
Implement `HashMap` read for `MaybeReadable` values
This allows us to read a `HashMap` that has values which may be
skipped if they are some backwards-compatibility type.
We also take this opportunity to fail deserialization if keys are
duplicated.
Inform ChannelManager when fulfilled HTLCs are finalized
When an HTLC has been failed, we track it up until the point there
exists no broadcastable commitment transaction which has the HTLC
present, at which point Channel returns the HTLCSource back to the
ChannelManager, which fails the HTLC backwards appropriately.
When an HTLC is fulfilled, however, we fulfill on the backwards path
immediately. This is great for claiming upstream HTLCs, but when we
want to track pending payments, we need to ensure we can check with
ChannelMonitor data to rebuild pending payments. In order to do so,
we need an event similar to the HTLC failure event, but for
fulfills instead.
Specifically, if we force-close a channel, we remove its off-chain
`Channel` object entirely, at which point, on reload, we may notice
HTLC(s) which are not present in our pending payments map (as they
may have received a payment preimage, but not fully committed to
it). Thus, we'd conclude we still have a retryable payment, which
is untrue.
This commit does so, informing the ChannelManager via a new return
element where appropriate of the HTLCSource corresponding to the
failed HTLC.
Track payments after they resolve until all HTLCs are finalized
In the next commit, we will reload lost pending payments from
ChannelMonitors during restart. However, in order to avoid
re-adding pending payments which have already been fulfilled, we
must ensure that we do not fully remove pending payments until all
HTLCs for the payment have been fully removed from their
ChannelMonitors.
We do so here, introducing a new PendingOutboundPayment variant
called `Completed` which only tracks the set of pending HTLCs.
Rename payment object vars to refer to payments and not session IDs
Add PaymentSecrets to HTLCSource::OutboundRoute objects
Reload pending payments from ChannelMonitor HTLC data on reload
If we go to send a payment, add the HTLC(s) to the channel(s),
commit the ChannelMonitor updates to disk, and then crash, we'll
come back up with no pending payments but HTLC(s) ready to be
claim/failed.
This makes it rather impractical to write a payment sender/retryer,
as you cannot guarantee atomicity - you cannot guarantee you'll
have retry data persisted even if the HTLC(s) are actually pending.
Because ChannelMonitors are *the* atomically-persisted data in LDK,
we lean on their current HTLC data to figure out what HTLC(s) are a
part of an outbound payment, rebuilding the pending payments list
on reload.
Add some basic test coverage of monitor payment data reloading
Move test_dup_htlc_onchain_fails_on_reload to payment_tests
test_dup_htlc_onchain_fails_on_reload is now more of a
payment_test than a functional_test, testing for handling of
pending payments.
Add a test of an HTLC being fulfilled and then later failed
Peers probably shouldn't do this, but if they want to give us free
money, we should take it and not generate any spurious events.
Define Payee abstraction for use in get_route
A payee can be identified by a pubkey and optionally have an associated
set of invoice features and route hints. Use this in get_route instead
of three separate parameters. This may be included in PaymentPathFailed
later to use when finding a new route.
Remove outdated line from get_route docs
Include PaymentPathRetry data in PaymentPathFailed
When a payment path fails, it may be retried. Typically, this means
re-computing the route after updating the NetworkGraph and channel
scores in order to avoid the failing hop. The last hop in
PaymentPathFailed's path field contains the pubkey, amount, and CLTV
values needed to pass to get_route. However, it does not contain the
payee's features and route hints from the invoice.
Include the entire set of parameters in PaymentPathRetry and add it to
the PaymentPathFailed event. Add a get_retry_route wrapper around
get_route that takes PaymentPathRetry. This allows an EventHandler to
retry failed payment paths using the payee's route hints and features.
Use option TLV decoding for short_channel_id
Using ignorable TLV decoding is only applicable for an Option containing
an enum, but short_channel_id is an Option<u64>. Use option TLV encoding
instead.
Make `Payee::pubkey` pub.
`Payee` is expected to be used by users to get routes for payment
retries, potentially with their own router. Thus, its helpful if it
is pub, even if it is redundant with the last hop in the `path`
field in `Events::PaymentPathFailed`.
Copy `Payee` into `Route`s to provide them to `ChannelManager`
Store `Payee` information in `HTLCSource::OutboundRoute`.
This stores and tracks HTLC payee information with HTLCSource info,
allowing us to provide it back to the user if the HTLC fails and
ensuring persistence by keeping it with the HTLC itself as it
passes between Channel and ChannelMonitor.
Expose log_bytes! macro for use in other crates
Needed to log PaymentHash in the lightning-invoice crate when retrying
payments.
Add PaymentId to PaymentSent event
The payment_hash may not uniquely identify the payment if it has been
reused. Include the payment_id in PaymentSent events so it can
correlated with the send_payment call.
Add PaymentId to PaymentPathFailed event
The PaymentId is needed when retrying payments. Include it in the
PaymentPathFailed event so it can be used in that manner.
Rewrite Invoice's interface in terms of msats
InvoiceBuilder's interface was changed recently to work in terms of
msats. Update Invoice's interface to return the amount in msats, too,
and make amount_pico_btc private.
Unify route finding methods
An upcoming Router interface will be used for finding a Route both when
initially sending a payment and also when retrying failed payment paths.
Unify the three varieties of get_route so the interface can consist of a
single method implemented by the new `find_route` method. Give get_route
pub(crate) visibility so it can still be used in tests.
Add InvoicePayer for retrying failed payments
When a payment fails, it's useful to retry the payment once the network
graph and channel scores are updated. InvoicePayer is a utility for
making payments which will retry any failed payment paths for a payment
up to a configured number of total attempts. It is parameterized by a
Payer and Router for ease of customization and testing.
Implement EventHandler for InvoicePayer as a decorator that intercepts
PaymentPathFailed events and retries that payment using the parameters
from the event. It delegates to the decorated EventHandler after retries
have been exhausted and for other events.
Support paying zero-value invoices
Fail payment retry if Invoice is expired
According to BOLT 11:
- after the `timestamp` plus `expiry` has passed
- SHOULD NOT attempt a payment
Add a convenience method for checking if an Invoice has expired, and use
it to short-circuit payment retries.
Implement Payer and Router for lightning crate
Implements Payer for ChannelManager and Rotuer for find_route, which can
be used to parameterize InvoicePayer when needing payment retries.
Add a utility trait in `router` to get the fees along a given path
Pass the failing/succeeding `Path` to PendingOutboundPayment meths
This will make the next commit much simpler
Track the amount spent on fees as payments are retried
Especially once we merge the `InvoicePayer` logic soon, we'll want
to expose the total fee paid in the `PaymentSent` event.
Correct send-bounding logic in `TestRoutingMessageHandler`
The `cmp::min` appeared to confused `end` for a count.
Add `PeerManager::disconnect_all_peers` to avoid complexity in BP
In the coming commits simply calling `timer_tick_occurred` will no
longer disconnect all peers, so its helpful to have a utility
method.
Constify the ratio in buf limits between forward and init sync msgs
Util-ify enqueueing an encoded message in peer_handler
This marginally simplifies coming commits.
Give peers which are sending us messages longer to respond to ping
See comment for rationale.
Give peers one timer tick to finish handshake before disconnecting
This ensures we don't let a hung connection stick around forever if
the peer never completes the initial handshake.
This also resolves a race where, on receiving a second connection
from a peer, we may reset their_node_id to None to prevent sending
messages even though the `channel_encryptor`
`is_ready_for_encryption()`. Sending pings only checks the
`channel_encryptor` status, not `their_node_id` resulting in an
`unwrap` on `None` in `enqueue_message`.
Log peer public key more thoroughly when logging in peer_handler
Notify scorer of failing payment path and channel
Upon receiving a PaymentPathFailed event, the failing payment may be
retried on a different path. To avoid using the channel responsible for
the failure, a scorer should be notified of the failure before being
used to find a new route.
Add a payment_path_failed method to routing::Score and call it in
InvoicePayer's event handler. Introduce a LockableScore parameterization
to InvoicePayer so the scorer is locked only once before calling
find_route.
Penalize failed channels in Scorer
As payments fail, the channel responsible for the failure may be
penalized. Implement Scorer::payment_path_failed to penalize the failed
channel using a configured penalty. As time passes, the penalty is
reduced using exponential decay, though penalties will accumulate if the
channel continues to fail. The decay interval is also configurable.
Test InvoicePayer in BackgroundProcessor
Proof of concept showing InvoicePayer can be used with an
Arc<ChannelManager> passed to BackgroundProcessor. Likely do not need to
merge this commit.
Move PaymentId to a [u8; 32] in bindings as for other hash objects
This should allow us to fix
https://github.com/lightningdevkit/ldk-garbagecollected/issues/52
Provide payment retry data when an MPP payment failed partially
This will allow `InvoicePayer` to properly retry payments that only
partially failed to send.
Expand `InvoicePayer` documentation somewhat to clarify edge-cases
Dont unwrap `RouteParameter::expiry_time` as users can set it
Users can provide anything they want as `RouteParameters` so we
shouldn't assume any fields are set any particular way, including
`expiry_time` set at all.
Rewrite InvoicePayer retry to correctly handle MPP partial failures
This rewrites a good chunk of the retry logic in `InvoicePayer` to
address two issues:
* it was not considering the return value of `send_payment` (and
`retry_payment`) may indicate a failure on some paths but not
others,
* it was not considering that more failures may still come later
when removing elements from the retry count map. This could
result in us seeing an MPP-partial-failure, failing to retry,
removing the retries count entry, and then retrying other parts,
potentially forever.
Add an integration test for InvoicePayer paying when one part fails
This tests the multi-part-single-failure-immediately fixes in the
previous commit.
Add integration test for InvoicePayerretry on an immediate failure
Check for invoice expiry in InvoicePayer before we send any HTLCs
Parameterize NetGraphMsgHandler with NetworkGraph
NetworkGraph is owned by NetGraphMsgHandler, but DefaultRouter requires
a reference to it. Introduce shared ownership to NetGraphMsgHandler so
that both can use the same NetworkGraph.
Make NetGraphMsgHandler::network_graph private
Since NetworkGraph has shared ownership, NetGraphMsgHandler does not
need to expose its field.
Clarify Scorer docs around penalizing channels
Refactor channel failure penalty logic
Move channel failure penalty logic into a ChannelFailure abstraction.
This encapsulates the logic for accumulating penalties and decaying them
over time. It also is responsible for the no-std behavior. This cleans
up Scorer and will make it easier to serialize it.
Parameterize Scorer by a Time trait
Scorer uses time to determine how much to penalize a channel after a
failure occurs. Parameterizing it by time cleans up the code such that
no-std support is in a single AlwaysPresent struct, which implements the
Time trait. Time is implemented for std::time::Instant when std is
available.
This parameterization also allows for deterministic testing since a
clock could be devised to advance forward as needed.
Implement (de)serialization for Scorer
Scorer should be serialized to retain penalty data between restarts.
Implement (de)serialization for Scorer by serializing last failure times
as duration since the UNIX epoch. For no-std, the zero-Duration is used.
Remove trailing ;s from macro calls to silence new rustc warnings
Rename Payee::new to Payee::from_node_id to clarify it somewhat
This also differentiates it from the bindings default-constructed
`new` method which is constructed when all fields are exposed and
of mappable types.
Make payment_path_failed path type bindings-mappable
The bindings don't currently support passing `Vec`s of objects
which it mappes as "opaque types". This is because it will require
clones to convert its own list of references to Rust's list of
objects.
In the near future we should resolve this limitation, allowing us
to revert this (and make `find_route`'s method signature similarly
cleaner), but for now we must avoid `Vec<OpaqueType>`.
Remove now-unused import in routing/mod.rs
Add `(C-not exported)` tag to a `Payee` modifier with move semantics
This matches the other `Payee` move-modifier functions.
Add `(C-not exported)` tags as required in tuple types
This prepares us for C bindings auto-exporting tuple type fields.
Tweak serialization of ScorerUsingTime for better forward compat
Update CHANGELOG for 0.0.103
Bump crate versions to 0.0.103/invoice 0.11
Fix `cargo doc` on older rustc
Apparently at least rustc 1.48 doesn't support `Self` in doc links,
so we make it explicit.
Add a ChannelTypeFeatures features object for the new channel_type
Its semantics are somewhat different from existing features,
however not enough to merit a different struct entirely.
Specifically, it only supports required features (if you send a
channel_type, the counterparty has to accept it wholesale or try
again, it cannot select only a subset of the flags) and it is
serialized differently (only appearing in TLVs).
Support de/ser of the new channel_type field in open_channel
Support send/recv'ing the new channel_type field in open_channel
This implements the channel type negotiation, though as we currently
only support channels with only static_remotekey set, it doesn't
implement the negotiation explicitly.
0.0.103 CHANGELOG tweaks from Jeff
Add note about PaymentId fields to 0.0.103 changelog
Add SinceEpoch time to test Scorer hermetically
In order to test Scorer hermetically, sleeps must be avoided. Add a
SinceEpoch abstraction for manually advancing time. Implement the Time
trait for SinceEpoch so that it can be used with ScorerUsingTime in
tests.
Add unit tests for Scorer
Test basic and channel failure penalties, including after a
(de-)serialization round trip.
Log before+after ChannelMonitor/Manager updates for visibility
I realized on my own node that I don't have any visibility into how
long a monitor or manager persistence call takes, potentially
blocking other operations. This makes it much more clear by adding
a relevant log_trace!() print immediately before and immediately
after persistence.
Fix to_remote output redeemscript when anchors enabled
Renamed script_for_p2wpkh to get_p2wpkh_redeemscript to match convention
Fix a minor memory leak on PermanentFailure mon errs when sending
If we send a payment and fail to update the first-hop channel state
with a `PermanentFailure` ChannelMonitorUpdateErr, we would have an
entry in our pending payments map, but possibly not return the
PaymentId back to the user to retry the payment, leading to a (rare
and relatively minor) memory leak.
Use upstream rust-bitcoin's dust calculation instead of our own
Not only does this move to common code, but it fixes handling of
all output types except for a few trivial cases.
Be less aggressive in outbound HTLC CLTV timeout checks
We currently assume our counterparty is naive and misconfigured and
may force-close a channel to get an HTLC we just forwarded them.
There shouldn't be any reason to do this - we don't have any such
bug, and we shouldn't start by assuming our counterparties are
buggy. Worse, this results in refusing to forward payments today,
failing HTLCs for largely no reason.
Instead, we keep a fairly conservative check, but not one which
will fail HTLC forwarding spuriously - testing only that the HTLC
doesn't expire for a few blocks from now.
Fixes #1114.
Correct Channel type serialization logic
Currently, we write out the Channel's `ChannelTypeFeatures` as an
odd type, implying clients which don't understand the
`ChannelTypeFeatures` field can simply ignore it. This is obviously
nonsense if the channel type is some future version - the client
needs to fail to deserialize as it doesn't understand the channel's
type.
We adapt the serialization logic here to only write out the
`ChannelTypeFeatures` field if it is something other than
only-static-remote-key, and simply consider that "default" (as it
is the only supported type today). Then, we write out the channel
type as an even TLV, implying clients which do not understand it
must fail to read the `Channel`.
Note that we do not need to bother reserving the TLV type no longer
written as it never appeared in a release (merged post-0.0.103).
Refactor InvoicePayer for spontaneous payments
To support spontaneous payments, InvoicePayer's sending logic must be
invoice-agnostic. Refactor InvoicePayer::pay_invoice_internal such that
invoice-specific code is in pay_invoice_using_amount and the remaining
logic is in pay_internal.
Further refactor the code's payment_cache locking such that it is
accessed consistently when needed, and tidy up the code a bit.
Support spontaneous payments in InvoicePayer
InvoicePayer handles retries not only when handling PaymentPathFailed
events but also for some types of PaymentSendFailure on the initial
send. Expand InvoicePayer's interface with a pay_pubkey function for
spontaneous (keysend) payments. Add a send_spontaneous_payment function
to the Payer trait to support this and implement it for ChannelManager.
Replace expect_value_msat with expect_send
Modify all InvoicePayer unit tests to use expect_send instead of
expect_value_msat, since the former can discern whether the send was for
an invoice, spontaneous payment, or a retry. Updates tests to set payer
expectations if they weren't already and assert these before returning a
failure.
Test retrying payment on partial send failure
Add some test coverage for when InvoicePayer retries within pay_invoice
because the Payer returned a partial failure.
Add PaymentHash parameter to Router::find_route
Implementations of Router may need the payment hash in order to look up
pre-computed routes from a probe for a given payment. Add a PaymentHash
parameter to Router::find_route to allow for this.
Provide `Score` the HTLC amount and channel capacity
This should allow `Score` implementations to make substantially
better decisions, including of the form "willing to pay X to avoid
routing over this channel which may have a high failure rate".
Penalize large HTLCs relative to channels in default `Scorer`
Sending HTLCs which are any greater than a very small fraction of the
channel size tend to fail at a much higher rate. Thus, by default
we start applying a penalty at only 1/8th the channel size and
increase it linearly as the amount reaches the channel's capacity,
20 msat per 1024th of the channel capacity.
Move `Score` into a `scoring` module instead of a top-level module
Traits in top-level modules is somewhat confusing - generally
top-level modules are just organizational modules and don't contain
things themselves, instead placing traits and structs in
sub-modules. Further, its incredibly awkward to have a `scorer`
sub-module, but only have a single struct in it, with the relevant
trait it is the only implementation of somewhere else. Not having
`Score` in the `scorer` sub-module is further confusing because
it's the only module anywhere that references scoring at all.
Return `ClosureReason` from `Channel` chain update methods
This fixes a few `ClosureReason`s and allows us to have
finer-grained user-visible errors when a channel closes due to an
on-chain event.
Automatically close channels that go unfunded for 2016 blocks
As recommended by BOLT 2 added in
https://github.com/lightningnetwork/lightning-rfc/pull/839
Add 'accept_inbound_channels' config option.
Limit minimum output size to the dust limit when RBF-bumping
Check all outputs meet the dust threshold in check_spends!()
Correct txid logging to reverse bytes.
We also take this opportunity to log the channel being closed when
one is closed by an on-chain spend of the funding output.
Ensure current channel state is logged for all channels on startup
Remove user_payment_id
In upcoming commits, we'll be making the payment secret and payment hash/preimage
derivable from info about the payment + a node secret. This means we don't
need to store any info about incoming payments and can eventually get rid of the
channelmanager::pending_inbound_payments map.
Add a new log-level for gossip messages.
Fix MPP routefinding when we first collect 95% of payment value
See comment in new test for more details.
Introduce new helper commit_tx_fee_sat
Cancel the outbound feerate update if above what we can afford
Check we won't overflow `max_dust_htlc_exposure_msat` at outbound feerate update
Re-add `test_max_dust_htlc_exposure`
Introduce CommitmentStats
Check outbound update_fee affordance incremented with holding cell HTLCs
Add anchor support to commitment HTLC outputs
Increase visibility of anchor related methods
Add anchor support to build_htlc_transaction
Adjust HTLC_{SUCCESS,TIMEOUT}_TX_WEIGHT when anchors used
Add test vectors for get_htlc_redeemscript wrt anchors
Generate PaymentPathSuccessful event for each path
A single PaymentSent event is generated when a payment is fulfilled.
This is occurs when the preimage is revealed on the first claimed HTLC.
For subsequent HTLCs, the event is not generated.
In order to score channels involved with a successful payments, the
scorer must be notified of each successful path involved in the payment.
Add a PaymentPathSuccessful event for this purpose. Generate it whenever
a part is removed from a pending outbound payment. This avoids duplicate
events when reconnecting to a peer.
Make Channel::commit_tx_fee_msat static and take fee explicitly
This may avoid risk of bugs in the future as it requires the caller
to think about the fee being used, not just blindly use the current
(committed) channel feerate.
Rewrite test_update_fee_that_funder_cannot_afford to avoid magic
Instead of magic hard-coded constants, its better for tests to
derive the values used so that they change if constants are changed
and so that it is easier to re-derive constants in the future as
needed.
Correct initial commitment tx fee affordability checks on open
Previously, we would reject inbound channels if the funder wasn't
able to meet our channel reserve on their first commitment
transaction only if they also failed to push enough to us for us
to not meet their initial channel reserve as well.
There's not a lot of reason to care about us meeting their reserve,
however - its largely expected that they may not push enough to us
in the initial open to meet it, and its not actually our problem if
they don't.
Further, we used our own fee, instead of the channel's actual fee,
to calculate fee affordability of the initial commitment
transaction.
We resolve both issues here, rewriting the combined affordability
check conditionals in inbound channel open handling and adding a
fee affordability check for outbound channels as well.
The prior code may have allowed a counterparty to start the channel
with "no punishment" states - violating the reason for the reserve
threshold.
Test fixed channel reserve checks on channel open
Fix compilation with the max_level_trace feature
Test all log-limiting features in CI
Support `logger::Record` in C by String-ing the fmt::Arguments
This adds a new (non-feature) cfg argument `c_bindings` which will
be set when building C bindings. With this, we can (slightly) tweak
behavior and API based on whether we are being built for Rust or C
users.
Ideally we'd never need this, but as long as we can keep the API
consistent-enough to avoid material code drift, this gives us a
cheap way of doing the "right" thing for both C and Rust when the
two are in tension.
We also move lightning-background-processor to support the same
MSRV as the main lightning crate, instead of only
lightning-net-tokio's MSRV.
Make `Score : Writeable` in c_bindings and impl on `LockedScore`
Ultimately we likely need to wrap the locked `Score` in a struct
that exposes writeable somehow, but because all traits have to be
fully concretized for C bindings we'll still need `Writeable` on
all `Score` in order to expose `Writeable` on the locked score.
Otherwise, we'll only have a `LockedScore` with a `Score` visible
that only has the `Score` methods, never the original type.
Seal `scoring::Time` and only use `Instant` or `Eternity` publicly
`scoring::Time` exists in part to make testing the passage of time
in `Scorer` practical. To allow no-std users to provide a time
source it was exposed as a trait as well. However, it seems
somewhat unlikely that a no-std user is going to have a use for
providing their own time source (otherwise they wouldn't be a
no-std user), and likely they won't have a graph in memory either.
`scoring::Time` as currently written is also exceptionally hard to
write C bindings for - the C bindings trait mappings relies on the
ability to construct trait implementations at runtime with function
pointers (i.e. `dyn Trait`s). `scoring::Time`, on the other hand,
is a supertrait of `core::ops::Sub` which requires a `sub` method
which takes a type parameter and returns a type parameter. Both of
which aren't practical in bindings, especially given the
`Sub::Output` associated type is not bound by any trait bounds at
all (implying we cannot simply map the `sub` function to return an
opaque trait object).
Thus, for simplicity, we here simply seal `scoring::Time` and make
it effectively-private, ensuring the bindings don't need to bother
with it.
Implement Clone, Hash, PartialEq for ClosingTransaction
This is a public struct intended to be used as an object by users,
so it should likely have common implementations, given they're
trivial.
Implement Clone for InvalidShutdownScript
Users hopefully shouldn't have much of a reason to use this, but
the bindings may need it to ensure no leaking pointers over an ffi.
Store holder channel reserve and max-htlc-in-flight explicitly
Previously, `holder_selected_channel_reserve_satoshis` and
`holder_max_htlc_value_in_flight_msat` were constant functions
of the channel value satoshis. However, in the future we may allow
allow users to specify it. In order to do so, we'll need to track
them explicitly, including serializing them as appropriate.
We go ahead and do so here, in part as it will make testing
different counterparty-selected channel reserve values easier.
Explicitly support counterparty setting 0 channel reserve
A peer providing a channel_reserve_satoshis of 0 (or less than our
dust limit) is insecure, but only for them. Because some LSPs do it
with some level of trust of the clients (for a substantial UX
improvement), we explicitly allow it. Because its unlikely to
happen often in normal testing, we test it explicitly here.
Fix regression when reading `Event::PaymentReceived` in some cases
For some reason rustc was deciding on a type for the `Option` being
deserialized for us as `_user_payment_id`. This really, really,
absolutely should have been a compile failure - the type (with
methods called on it!) was ambiguous! Instead, rustc seems to have
been defaulting to `Option<()>`, causing us to read zero of the
eight bytes in the `user_payment_id` field, which returns an
`Err(InvalidValue)` error as TLVs must always be read fully.
This should likely be reported to rustc as its definitely a bug,
but I cannot seem to cause the same error on any kinda of
vaguely-minimized version of the same code.
Found by `chanmon_consistency` fuzz target.
Fix compilation in `payment` rustdoc examples
The samples were not valid rust, but previous versions of rustc had
a bug where they were accepted anyway. Latest rustc beta no longer
accepts these.
Score successful payment paths
Expand the Score trait with a payment_path_successful function for
scoring successful payment paths. Called by InvoicePayer's EventHandler
implementation when processing PaymentPathSuccessful events. May be used
by Score implementations to revert any channel penalties that were
applied by calls to payment_path_failed.
Decay channel failure penalty upon success
If a payment failed to route through a channel, a penalty is applied to
the channel in the future when finding a route. This penalty decays over
time. Immediately decay the penalty by one half life when a payment is
successfully routed through the channel.
Fix shift overflow in Scorer::channel_penalty_msat
An unchecked shift of more than 64 bits on u64 values causes a shift
overflow panic. This may happen if a channel is penalized only once and
(1) is not successfully routed through and (2) after 64 or more half
life decays. Use a checked shift to prevent this from happening.
Allow missing-docs on test-only macros
Prefer fully-specified paths in test macros
This avoids macros being context-specific use-dependent.
Continue after a single failure in `ChannelMonitor::update_monitor`
`ChannelMonitorUpdate`s may contain multiple updates, including, eg
a payment preimage after a commitment transaction update. While
such updates are generally not generated today, we shouldn't return
early out of the update loop, causing us to miss any updates after
an earlier update fails.
Always return failure in `update_monitor` after funding spend
Previously, monitor updates were allowed freely even after a
funding-spend transaction confirmed. This would allow a race
condition where we could receive a payment (including the
counterparty revoking their broadcasted state!) and accept it
without recourse as long as the ChannelMonitor receives the block
first, the full commitment update dance occurs after the block is
connected, and before the ChannelManager receives the block.
Obviously this is an incredibly contrived race given the
counterparty would be risking their full channel balance for it,
but its worth fixing nonetheless as it makes the potential
ChannelMonitor states simpler to reason about.
The test in this commit also tests the behavior changed in the
previous commit.
Drop `MonitorUpdateErr` in favor of opaque errors.
We don't expect users to ever change behavior based on the string
contained in a `MonitorUpdateErr`, except log it, so there's little
reason to not just log it ourselves and return a `()` for errors.
We do so here, simplifying the callsite in `ChainMonitor` as well.
Make APIError debug output more clear by including the variant
Add a simple test for ChainMonitor MonitorUpdate-holding behavior
Add a test for MonitorEvent holding when they complete out-of-order
Add trivial test of monitor update failure during block connection
Remove OnionV2 parsing support
OnionV2s don't (really) work on Tor anymore anyway, and the field
is set for removal in the BOLTs [1]. Sadly because of the way
addresses are parsed we have to continue to understand that type 3
addresses are 12 bytes long. Thus, for simplicity we keep the
`OnionV2` enum variant around and just make it an opaque 12 bytes,
with the documentation updated to note the deprecation.
[1] https://github.com/lightning/bolts/pull/940
Ensure ChannelManager methods are idempotent
During event handling, ChannelManager methods may need to be called as
indicated in the Event documentation. Ensure that these calls are
idempotent for the same event rather than panicking. This allows users
to persist events for later handling without needing to worry about
processing the same event twice (e.g., if ChannelManager is not
persisted but the events were, the restarted ChannelManager would return
some of the same events).
Define public getters for all feature flags
There's not a ton of reason not to do this, and moving it to the
macro removes some code.
Support the `channel_type` feature bit.
Note that this feature bit does absolutely nothing. We signal it
(as we already support channel type negotiation), but do not bother
to look to see if peers support it, as we don't care - we simply
look for the TLV entry and deduce if a peer supports channel type
negotiation from that.
The only behavioral change at all here is that we don't barf if a
peer sets channel type negotiation to required via the feature bit
(instead of failing the channel at open-time), but of course no
implementations do this, and likely won't for some time (if ever -
you can simply fail channels with unknown types later, and there's
no reason to refuse connections, really).
As defined in https://github.com/lightning/bolts/pull/906
Reduce force-closures with user fee estimators which round poorly
See comment for more
Upgrade to codecov uploader v2
Some time ago codecov stopped supporting their old v1 uploader, and
it seems they've now finally turned it off, so we aren't getting
any coverage reports anymore. Hopefully upgrading is pretty trivial.
Getter for the total channel balance
The existing balance getters subtract reserve, this one does not.
Separate ChannelAnnouncement and ChannelUpdate broadcast conditions
When a `ChannelUpdate` message is generated for broadcast as a part
of a `BroadcastChannelAnnouncement` event, it may be newer than our
previous `ChannelUpdate` and need to be broadcast. However, if the
`ChannelAnnouncement` had already been seen we wouldn't
re-broadcast either message as the `handle_channel_announcement`
call would fail, short-circuiting the condition to broadcast both.
Instead, we split the broadcast of each message as well as the
conditional so that we always attempt to handle each message and
update our local graph state, then broadcast the message if its
update was processed successfully.
Update `ChannelUpdate::timestamp` when channels are dis-/en-abled
We update the `Channel::update_time_counter` field (which is copied
into `ChannelUpdate::timestamp`) only when the channel is
initialized or closes, and when a new block is connected. However,
if a peer disconnects or reconnects, we may wish to generate
`ChannelUpdate` updates in between new blocks. In such a case, we
need to make sure the `timestamp` field is newer than any previous
updates' `timestamp` fields, which we do here by simply
incrementing it when the channel status is changed.
As a side effect of this we have to update
`test_background_processor` to ensure it eventually succeeds even
if the serialization of the `ChannelManager` changes after the test
begins.
Re-broadcast our own gossip even if its same as the last broadcast
Even if our gossip hasn't changed, we should be willing to
re-broadcast it to our peers. All our peers may have been
disconnected the last time we broadcasted it.
Add a comment describing `update_time_counter` and when its updated
Add a comment describing the timestamp field's use
DRY up payment failure macros in functional_test_utils
... with a more extensible expectation-checking framework for them.
Add a variant to `PendingOutboundPayment` for retries-exceeded
When a payer gives up trying to retry a payment, they don't know
for sure what the current state of the event queue is.
Specifically, they cannot be sure that there are not multiple
additional `PaymentPathFailed` or even `PaymentSuccess` events
pending which they will see later. Thus, they have a very hard
time identifying whether a payment has truly failed (and informing
the UI of that fact) or if it is still pending. See [1] for more
information.
In order to avoid this mess, we will resolve it here by having the
payer give `ChannelManager` a bit more information - when they
have given up on a payment - and using that to generate a
`PaymentFailed` event when all paths have failed.
This commit adds the neccessary storage and changes for the new
state inside `ChannelManager` and a public method to mark a payment
as failed, the next few commits will add the new `Event` and use
the new features in our `PaymentRetrier`.
[1] https://github.com/lightningdevkit/rust-lightning/issues/1164
Expose an event when a payment has failed and retries complete
When a payment fails, a payer needs to know when they can consider
a payment as fully-failed, and when only some of the HTLCs in the
payment have failed. This isn't possible with the current event
scheme, as discovered recently and as described in the previous
commit.
This adds a new event which describes when a payment is fully and
irrevocably failed, generating it only after the payment has
expired or been marked as expired with
`ChannelManager::mark_retries_exceeded` *and* all HTLCs for it
have failed. With this, a payer can more simply deduce when a
payment has failed and use that to remove payment state or
finalize a payment failure.
Use `Event::PaymentFailed` in `InvoicePayer` to remove retry count
This finally fixes the bug described in the previous commits where
we retry a payment after its retry count has expired due to early
removal of the payment from the retry count tracking map. A test is
also added which demonstrates the bug in previous versions and
which passes now.
Fixes #1164.
Make attempting to retry a succeeded payment an APIError, not Route
This is symmetric with the new failure once a payment is abandoned.
Updates Fee estimator docs to include a max return value for get_est_sat_per_1000_weight
Updates docs to include a max return value for get_est_sat_per_1000_weight
Adds max to both conversions
Additional detail
Add mpp_timeout and invalid_onion_payload descriptions & handling
Pin tokio to 1.14.0 in CI for older rustc's
Build no-std on 1.47 now that core2 supports it
DRY up network_graph tests substantially with message creation fns
Drop `allow_wallclock_use` feature in favor of simply using `std`
Fixes #1147.
Add a method to prune stale channels from `NetworkGraph`
We define "stale" as "haven't heard an updated channel_update in
two weeks", as described in BOLT 7.
We also filter out stale channels at write-time for `std` users, as
we can look up the current time.
Reject channel_update messages with timestamps too old or new
Because we time out channel info that is older than two weeks now,
we should also reject new channel info that is older than two
weeks, in addition to rejecting future channel info.
Automatically prune NetworkGraph of stale channels hourly in BP
Add get_inbound_payment_key_material to KeysInterface
This will allow us to retrieve key material for encrypting/decrypting inbound
payment info, in upcoming commits
Macro-ize checking that the total value of an MPP's parts is sane
This DRY-ed code will be used in upcoming commits when we stop storing inbound
payment data
Add get_single_block to chacha20 module
In the next commit, we'll want to get a single block from a chacha stream that
takes a 16-byte nonce.
Drop need to store pending inbound payments
and replace payment_secret with encrypted metadata
See docs on `inbound_payment::verify` for details
Also add min_value checks to all create_inbound_payment* methods
Add new invoice CreationError::InvalidAmount for use in checking `create_inbound_payment`
in an invoice creation utility. Note that if the error type of `create_inbound_payment` ever
changed, we'd be forced to update the invoice utility's callsite to handle the new error
Salt inbound payment ExpandedKey
Leftover feedback from #1177
create_inbound_payment: warn about dup hashes
Leftover feedback from #1177
inbound_payment: DRY verify method for use in getting preimage
in the next commit(s)
inbound_payment: Add utility to get payment preimage given hash/secret
User-requested feature
Remove trailing whitespace in the FeeEstimator interface
Update CHANGELOG for 0.0.104
Bump versions to 0.0.104/invoice 0.12
Swap around generic argument ordering in DefaultRouter for bindings
The bindings generation really should support default generic types
in where clauses, but currently does not. To avoid needing to add
support during the current release process, we simply swap around
the arguments to move them to the first <> instead of the where.
Add a constructor to MultiThreadedLockableScore
...as otherwise the struct is rather useless.
Add a C-not exported tag to `NetGraphMsgHandler.network_graph`
Swap around generic argument ordering in InvoicePayer for bindings
The bindings generation really should support generic bounds other
than Deref::Target in where clauses, but currently does not. To
avoid needing to add support during the current release process,
we simply swap around the arguments to move them to the first <>
instead of the where.
update repo name to use lightningdevkit
Log gossip rejections due to stale channel_updates at GOSSIP level
This further reduces noise at the TRACE level during initial gossip
sync.
Support building `cfg=c_bindings` with `no-std`
This will be needed for JavaScript bindings eventually.
Adapt lightning-invoice to no_std
Do not turn on bech32/std by default for lightning-invoice
Add a new `WarningMessage` message to send and receive warnings
Handle sending and receiving warning messages
Send warning messages when appropriate in gossip handling pipeline
Convert `shutdown` invalid script checks to warning messages
As required by the warning messages PR, we should simply warn our
counterparty in this case and let them try again, continuing to try
to use the channel until they tell us otherwise.
Send `warning` instead of `error` when we incounter bogus gossip
Rely on Error/Warning message data lengths being correct
In https://github.com/lightning/bolts/pull/950, the (somewhat
strange) requirement that error messages be handled even if the
length field is set larger than the size of the package was
removed. Here we change the code to drop the special handling for
this, opting to just fail to read the message if the length is
incorrect.
Set the SigHashType of remote htlc signatures w/ anchors to SinglePlusAnyoneCanPay
Isolated channelmonitor weight unit tests and added anchor loops
make WEIGHT{_REVOKED,}_{OFFERED,RECEIVED}_HTLC functions with opt_anchors parameter
Add unit test coverage for package::weight_{offered,received}_htlc
Convert COMMITMENT_TX_BASE_WEIGHT to anchor-aware function
Convert HTLC_{SUCCESS,TIMEOUT}_TX_WEIGHT to anchor-aware functions
Debit funder's output to cover anchors
Add anchor tests to outbound_commitment_test
Set opt_anchors for calls to CommitmentTransaction::new_with_auxiliary_htlc_data
Fixed comment on weight_received_htlc
Make lockorder consistent in channelmanager
This resolves a lockorder inversion in
`ChannelManager::finalize_claims` where `pending_outbound_payments`
is locked after `pending_events`, opposite of, for example, the
lockorder in `ChannelManager::fail_htlc_backwards_internal` where
`pending_outbound_payments` is locked at the top of the
`HTLCSource::OutboundRoute` handling and then `pending_events` is
locked at the end.
Fix some (non-bug) lock-order-inversions in tests
Check lockorders in tests with a trivial lockorder tracker
Fix build errors
Create script using p2wsh for comparison
Using p2wpkh for generating the payment script
spendable_outputs sanity check
vss96
added a commit
to vss96/rust-lightning
that referenced
this pull request
Jan 20, 2022
Add methods to count total fees and total amount in a Route #999
* Added `get_total_fees` method to route,
to calculate all the fees paid accross each path.
* Added `get_total_amount` method to route,
to calculate the total of actual amounts paid in each path.
Update docs to specify where process events is called
Add Event::ChannelClosed generation at channel shutdown
Rename MonitorEvent::CommitmentTxBroadcasted to CommitmentTxConfirmed
Extend MsgHandleErrInternal with a new chan_id field Option<[u8; 32]>
This field is used in next commit to generate appropriate
ChannelClosed event at `handle_error()` processing.
Add ChannelClosed generation at cooperative/force-close/error processing
When we detect a channel `is_shutdown()` or call on it
`force_shutdown()`, we notify the user with a Event::ChannelClosed
informing about the id and closure reason.
Add `pending_events` deadlock detection in `handle_error`
Bump HTTP read timeout to match reality of Bitcoin Core blocking
Fix future unknown `Event` variant backwards compatibility
In 8ffc2d1742ff1171a87b0410b21cbbd557ff8247, in 0.0.100, we added
a backwards compatibility feature to the reading of `Event`s - if
the type was unknown and odd, we'd simply ignore the event and
treat it as no event. However, we failed to read the
length-prefixed TLV stream when doing so, resulting in us reading
some of the skipped-event data as the next event or other data in
the ChannelManager.
We fix this by reading the varint length prefix written, then
skipping that many bytes when we come across an unknown odd event
type.
Fix a panic in Route's new fee-calculation methods and clean up
This addresses Val's feedback on the new Route fee- and
amount-calculation methods, including fixing the panic she
identified and cleaning up various docs and comments.
Adds Transaction to lighting-block-sync::convert
Includes disclaimer in docs, see https://github.com/rust-bitcoin/rust-lightning/pull/1061#issuecomment-911960862
Rename PaymentFailed -> PaymentPathFailed
Since we don't want to imply to users that a payment has
completely failed when it really has just partially
failed
Add path field to PaymentPathFailed event
Fix windows-only test failure added in #997
This is a trivial bugfix to add a missing test updated required in
PR 997.
Move trait bounds on `wire::Type` from use to the trait itself
`wire::Type` is only (publicly) used as the `CustomMessage`
associated type in `CustomMessageReader`, where it has additional
trait bounds on `Debug` and `Writeable`. The documentation for
`Type` even mentions that you need to implement `Writeable` because
this is the one place it is used.
To make this more clear, we move the type bounds onto the trait
itself and not on the associated type.
This is also the only practical way to build C bindings for `Type`
as we cannot have a concrete, single, `Type` struct in C which only
optionally implements various subtraits, at least not without
runtime checking of the type bounds.
Make `ChainMonitor::get_claimable_balances` take a slice of refs
For the same reason as `get_route`, a slice of objects isn't
practical to map to bindings - the objects in the bindings space
are structs with a pointer and some additional metadata. Thus, to
create a slice of them, we'd need to take ownership of the objects
behind the pointer, place them into a slace, and then restore them
to the pointer.
This would be a lot of memory copying and marshalling, not to
mention wouldn't be thread-safe, which the same function otherwise
would be if we used a slice of references instead of a slice of
objects.
Use Infallible for the unconstructable default custom message type
When we landed custom messages, we used the empty tuple for the
custom message type for `IgnoringMessageHandler`. This was fine,
except that we also implemented `Writeable` to panic when writing
a `()`. Later, we added support for anchor output construction in
CommitmentTransaction, signified by setting a field to `Some(())`,
which is serialized as-is.
This causes us to panic when writing a `CommitmentTransaction`
with `opt_anchors` set. Note that we never set it inside of LDK,
but downstream users may.
Instead, we implement `Writeable` to write nothing for `()` and use
`core::convert::Infallible` for the default custom message type as
it is, appropriately, unconstructable.
This also makes it easier to implement various things in bindings,
as we can always assume `Infallible`-conversion logic is
unreachable.
Drop redundant generic bounds when the trait requires the bounds
Make method time on trait impl explitit to help bindings generator
Associated types in C bindings is somewhat of a misnomer - we
concretize each trait to a single struct. Thus, different trait
implementations must still have the same type, which defeats the
point of associated types.
In this particular case, however, we can reasonably special-case
the `Infallible` type, as an instance of it existing implies
something has gone horribly wrong.
In order to help our bindings code figure out how to do so when
referencing a parent trait's associated type, we specify the
explicit type in the implementation method signature.
Update CHANGELOG for 0.0.101
Bump Crate versions to 0.0.101 (and invoice to 0.9)
Make `NetworkGraph` Clone-able again
There isn't a lot of user-utility for cloning `NetworkGraph`
directly (its a rather large struct, and there probably isn't a lot
of reason to have *multiple* `NetworkGraph`s). Thus, when locks
were pushed down into it, the `Clone`-ability of it was dropped as
well.
Sadly, mapping the Java memory model onto:
* `Read`-ing a `NetworkGraph`, creating a Java-owned
`NetworkGraph` object that the JVM will destruct for us,
* Passing it to a `NetGraphMsgHandler`, which now expects to own
the `NetworkGraph`, including destructing it,
isn't really practical without adding a clone in between.
Given this, and the fact that there's nothing inherently wrong with
clone-ing a `NetworkGraph`, we simply re-add `Clone` here.
Drop broken test that is unfixable due to being undocumented
This should be reverted at some point, but the test is deficient
and breaks on later changes that are important to land ASAP.
Increase our default/minimum dust limit to 354 sat/vbytes
330 sat/vbyte, the current value, is not sufficient to ensure a
future segwit script longer than 32 bytes meets the dust limit if
used for a shutdown script. Thus, we can either check the value
on shutdown or we can simply require segwit outputs and require a
dust value of no less than 354 sat/vbyte.
We swap the minimum dust value to 354 sat/vbyte here, requiring
segwit scripts in a future commit.
See https://github.com/lightningnetwork/lightning-rfc/issues/905
Reduce the maximum allowed counterparty dust limit to 546 sat/vbyte
546 sat/vbyte is the current default dust limit on most
implementations, matching the network dust limit for P2SH outputs.
Implementations don't currently appear to send any larger dust
limits, and allowing a larger dust limit implies higher payment
failure risk, so we'd like to be as tight as we can here.
Require user cooperative close payout scripts to be Segwit
There is little reason for users to be paying out to non-Segwit
scripts when closing channels at this point. Given we will soon, in
rare cases, force-close during shutdown when a counterparty closes
to a non-Segwit script, we should also require it of our own users.
Force-close channels if closing transactions may be non-standard
If a counterparty (or an old channel of ours) uses a non-segwit
script for their cooperative close payout, they may include an
output which is unbroadcastable due to not meeting the network dust
limit.
Here we check for this condition, force-closing the channel instead
if we find an output in the closing transaction which does not meet
the limit.
Rename MIN_DUST_LIMIT_SATOSHIS constant to disambiguate chan vs P2P
While channel and P2P network dust limits are related, they're
ultimately two different things, and thus their constant names
should reference that.
Regenerate PendingHTLCsForwardable on reload instead of serializing
When we are prepared to forward HTLCs, we generate a
PendingHTLCsForwardable event with a time in the future when the
user should tell us to forward. This provides some basic batching
of forward events, improving privacy slightly.
After we generate the event, we expect users to spawn a timer in
the background and let us know when it finishes. However, if the
user shuts down before the timer fires, the user will restart and
have no idea that HTLCs are waiting to be forwarded/received.
To fix this, instead of serializing PendingHTLCsForwardable events
to disk while they're pending (before the user starts the timer),
we simply regenerate them when a ChannelManager is deserialized
with HTLCs pending.
Fixes #1042
Don't apply monitor updates after watch_channel PermFail
The full stack fuzzer found an unreachable panic where we receive a
FundingSigned with a duplicate channel outpoint.
Update Watch docs to disallow dup channel outpoints
on watch_channel
Rename MppId to PaymentId
Leftover from previous PR Jeff feedback.
Useful in upcoming commits as we'll expose this to users for payment retries
Return PaymentId from send_*payment functions
Used in upcoming commits for retries
Refactor send_payment internals for retries
We want to reuse send_payment internal functions for retries,
so some need to now be parameterized by PaymentId to avoid
generating a new PaymentId on retry
Refactor send_payment internals for retries 2
Retrying a partial payment means send_payment_internal needs to be parameterized
by a total payment amount, else 'HTLC values do not match' errors
channelmanager: Add retry data to pending_outbound_payments
Add method to retry payments
Don't remove failed payments when all paths fail
This is because we want the ability to retry completely failed
payments.
Upcoming commits will remove these payments on timeout to prevent
DoS issues
Also test that this removal allows retrying single-path payments
Expire outbound payments after 3 blocks if no parts are pending
Correct step number in `get_route`
Consider many first-hop paths to the same counterparty in routing
Previously we'd simply overwritten "the" first hop path to each
counterparty when routing, however this results in us ignoring all
channels except the last one in the `ChannelDetails` list per
counterparty.
f readability improvements from val
Update Event::PaymentReceived docs since we require payment secret
Users no longer need to verify the amounts of received payments as
the payment secret will protect us against the probing attacks such
verification was intended to fix.
Move tests of payment retries into a new module
Move pending payment tracking to after the new HTLC flies
If we attempt to send a payment, but the HTLC cannot be send due to
local channel limits, we'll provide the user an error but end up
with an entry in our pending payment map. This will result in a
memory leak as we'll never reclaim the pending payment map entry.
Correct error returned when `retry_payment` doesn't have a payment
Add payment_hash to PaymentSent #999
Replace PublicKey with [u8; 33] in NetworkGraph
Adds DiscardFunding event
During the event of a channel close, if the funding transaction
is yet to be broadcasted then a DiscardFunding event is issued
along with the ChannelClose event.
Use local channel state when constructing routes in test macro
This is a bit more realistic and needed to route over non-public
channels.
Fix loop label shadowing warning
Remove special case for onion error expiry_too_far
With channel scoring and payment retries, it is no longer necessary to
have expiry_too_far imply a payment failure.
Pass hop index in construct_onion_keys_callback
This simplifies failing route hop calculation, which will be useful for
later identifying the failing hop for PaymentFailed events.
Clean up fee_insufficient computation
Add failing short channel id to PaymentPathFailed
This will be useful for scoring channels when a payment fails.
Expose ReadOnlyNetworkGraph::get_addresses to C by cloning result
We cannot expose ReadOnlyNetworkGraph::get_addresses as is in C as
it returns a list of references to an enum, which the bindings
dont support. Instead, we simply clone the result so that it
doesn't contain references.
Speed up test_timer_tick_called
Fix unused variable warnings in fuzzer
Replace get_route with get_route_and_payment_hash
The interface for get_route will change to take a scorer. Using
get_route_and_payment_hash whenever possible allows for keeping the
scorer inside get_route_and_payment_hash rather than at every call site.
Replace get_route with get_route_and_payment_hash wherever possible.
Additionally, update get_route_and_payment_hash to use the known invoice
features and the sending node's logger.
Move mpp_failure test to payment_tests.rs
Move `Persist` trait to chainmonitor as that's the only reference
Move ChannelMonitorUpdateErr to chain as it is a chain::Watch val
Make `ChainMonitor::monitors` private and expose monitor via getter
Exposing a `RwLock<HashMap<>>` directly was always a bit strange,
and in upcoming changes we'd like to change the internal
datastructure in `ChainMonitor`.
Further, the use of `RwLock` and `HashMap` meant we weren't able
to expose the ChannelMonitors themselves to users in bindings,
leaving a bindings/rust API gap.
Thus, we take this opportunity go expose ChannelMonitors directly
via a wrapper, hiding the internals of `ChainMonitor` behind
getters. We also update tests to use the new API.
Simplify channelmonitor tests which use chain::Watch and Persister
test_simple_monitor_permanent_update_fail and
test_simple_monitor_temporary_update_fail both have a mode where
they use either chain::Watch or persister to return errors.
As we won't be doing any returns directly from the chain::Watch
wrapper in a coming commit, the chain::Watch-return form of the
test will no longer make sense.
Handle Persister returning TemporaryFailure for new channels
Previously, if a Persister returned a TemporaryFailure error when
we tried to persist a new channel, the ChainMonitor wouldn't track
the new ChannelMonitor at all, generating a PermanentFailure later
when the updating is restored.
This fixes that by correctly storing the ChannelMonitor on
TemporaryFailures, allowing later update restoration to happen
normally.
This is (indirectly) tested in the next commit where we use
Persister to return all monitor-update errors.
Use Persister to return errors in tests not chain::Watch
As ChainMonitor will need to see those errors in a coming PR,
we need to return errors via Persister so that our ChainMonitor
chain::Watch implementation sees them.
Use Persister to return errors in fuzzers not chain::Watch
Fixed 'Advancing Bitcoin' video URL.
Add channel scoring to get_route
Failed payments may be retried, but calling get_route may return a Route
with the same failing path. Add a routing::Score trait used to
parameterize get_route, which it calls to determine how much a channel
should be penalized in terms of msats willing to pay to avoid the
channel.
Also, add a Scorer struct that implements routing::Score with a constant
constant penalty. Subsequent changes will allow for more robust scoring
by feeding back payment path success and failure to the scorer via event
handling.
Return the temporary channel id in success from `create_channel`
This makes it more practical for users to track channels prior to
funding, especially if the channel fails because the peer rejects
it for a parameter mismatch.
Include the user channel id counter in Event::ChannelClosed
This makes it more practical for users to track channels using
their own IDs, especially across funding.
Rename create_channel param to user_channel_id to standardize it
Add CHANGELOG entries for 0.0.102
Bump crate versions to 0.0.102 and lightning-invoice 0.10
Simplify prefers_shorter_route_with_higher_fees
In order to make the scoring tests easier to read, only check the
relevant RouteHop fields. The remaining fields are tested elsewhere.
Expand the test to show the path used without scoring.
Add source and target nodes to routing::Score
Expand routing::Score::channel_penalty_msat to include the source and
target node ids of the channel. This allows scorers to avoid certain
nodes altogether if desired.
Move MonitorEvent serialization to TLV-enum-upgradable from custom
Move the two-AtomicUsize counter in peer_handler to a util struct
We also take this opportunity to drop byte_utils::le64_to_array, as
our MSRV now supports the native to_le_bytes() call.
Move ChannelManager::monitor_updated to a MonitorEvent
In the next commit we'll need ChainMonitor to "see" when a monitor
persistence completes, which means `monitor_updated` needs to move
to `ChainMonitor`. The simplest way to then communicate that
information to `ChannelManager` is via `MonitorEvet`s, which seems
to line up ok, even if they're now constructed by multiple
different places.
Use an opaque type to describe monitor updates in Persist
In the next commit, we'll be originating monitor updates both from
the ChainMonitor and from the ChannelManager, making simple
sequential update IDs impossible.
Further, the existing async monitor update API was somewhat hard to
work with - instead of being able to generate monitor_updated
callbacks whenever a persistence process finishes, you had to
ensure you only did so at least once all previous updates had also
been persisted.
Here we eat the complexity for the user by moving to an opaque
type for monitor updates, tracking which updates are in-flight for
the user and only generating monitor-persisted events once all
pending updates have been committed.
Persist `ChannelMonitor`s after new blocks are connected
This resolves several user complaints (and issues in the sample
node) where startup is substantially delayed as we're always
waiting for the chain data to sync.
Further, in an upcoming PR, we'll be reloading pending payments
from ChannelMonitors on restart, at which point we'll need the
change here which avoids handling events until after the user
has confirmed the `ChannelMonitor` has been persisted to disk.
It will avoid a race where we
* send a payment/HTLC (persisting the monitor to disk with the
HTLC pending),
* force-close the channel, removing the channel entry from the
ChannelManager entirely,
* persist the ChannelManager,
* connect a block which contains a fulfill of the HTLC, generating
a claim event,
* handle the claim event while the `ChannelMonitor` is being
persisted,
* persist the ChannelManager (before the CHannelMonitor is
persisted fully),
* restart, reloading the HTLC as a pending payment in the
ChannelManager, which now has no references to it except from
the ChannelMonitor which still has the pending HTLC,
* replay the block connection, generating a duplicate PaymentSent
event.
Update test_dup_htlc_onchain_fails_on_reload for new persist API
ChannelMonitors now require that they be re-persisted before
MonitorEvents be provided to the ChannelManager, the exact thing
that test_dup_htlc_onchain_fails_on_reload was testing for when it
*didn't* happen. As such, test_dup_htlc_onchain_fails_on_reload is
now testing that we bahve correctly when the API guarantees are not
met, something we don't need to do.
Here, we adapt it to test the new API requirements through
ChainMonitor's calls to the Persist trait instead.
Always release `MonitorEvent`s to `ChannelManager` after 3 blocks
If we have a `ChannelMonitor` update from an on-chain event which
returns a `TemporaryFailure`, we block `MonitorEvent`s from that
`ChannelMonitor` until the update is persisted. This prevents
duplicate payment send events to the user after payments get
reloaded from monitors on restart.
However, if the event being avoided isn't going to generate a
PaymentSent, but instead result in us claiming an HTLC from an
upstream channel (ie the HTLC was forwarded), then the result of a
user delaying the event is that we delay getting our money, not a
duplicate event.
Because user persistence may take an arbitrary amount of time, we
need to bound the amount of time we can possibly wait to return
events, which we do here by bounding it to 3 blocks.
Thanks to Val for catching this in review.
Clarify the contexts in which persist_new_channel may be called
Its somewhat confusing that `persist_new_channel` is called on
startup for an existing channel in common deployments, so we call
it out explicitly.
Make `Channel::revoke_and_ack`'s return tuple a struct
This substantially improves readability at the callsite and in the
function.
Make `Channel::monitor_updating_restored`'s return tuple a struct
This improves readability at the callsite and in the function.
Implement `HashMap` read for `MaybeReadable` values
This allows us to read a `HashMap` that has values which may be
skipped if they are some backwards-compatibility type.
We also take this opportunity to fail deserialization if keys are
duplicated.
Inform ChannelManager when fulfilled HTLCs are finalized
When an HTLC has been failed, we track it up until the point there
exists no broadcastable commitment transaction which has the HTLC
present, at which point Channel returns the HTLCSource back to the
ChannelManager, which fails the HTLC backwards appropriately.
When an HTLC is fulfilled, however, we fulfill on the backwards path
immediately. This is great for claiming upstream HTLCs, but when we
want to track pending payments, we need to ensure we can check with
ChannelMonitor data to rebuild pending payments. In order to do so,
we need an event similar to the HTLC failure event, but for
fulfills instead.
Specifically, if we force-close a channel, we remove its off-chain
`Channel` object entirely, at which point, on reload, we may notice
HTLC(s) which are not present in our pending payments map (as they
may have received a payment preimage, but not fully committed to
it). Thus, we'd conclude we still have a retryable payment, which
is untrue.
This commit does so, informing the ChannelManager via a new return
element where appropriate of the HTLCSource corresponding to the
failed HTLC.
Track payments after they resolve until all HTLCs are finalized
In the next commit, we will reload lost pending payments from
ChannelMonitors during restart. However, in order to avoid
re-adding pending payments which have already been fulfilled, we
must ensure that we do not fully remove pending payments until all
HTLCs for the payment have been fully removed from their
ChannelMonitors.
We do so here, introducing a new PendingOutboundPayment variant
called `Completed` which only tracks the set of pending HTLCs.
Rename payment object vars to refer to payments and not session IDs
Add PaymentSecrets to HTLCSource::OutboundRoute objects
Reload pending payments from ChannelMonitor HTLC data on reload
If we go to send a payment, add the HTLC(s) to the channel(s),
commit the ChannelMonitor updates to disk, and then crash, we'll
come back up with no pending payments but HTLC(s) ready to be
claim/failed.
This makes it rather impractical to write a payment sender/retryer,
as you cannot guarantee atomicity - you cannot guarantee you'll
have retry data persisted even if the HTLC(s) are actually pending.
Because ChannelMonitors are *the* atomically-persisted data in LDK,
we lean on their current HTLC data to figure out what HTLC(s) are a
part of an outbound payment, rebuilding the pending payments list
on reload.
Add some basic test coverage of monitor payment data reloading
Move test_dup_htlc_onchain_fails_on_reload to payment_tests
test_dup_htlc_onchain_fails_on_reload is now more of a
payment_test than a functional_test, testing for handling of
pending payments.
Add a test of an HTLC being fulfilled and then later failed
Peers probably shouldn't do this, but if they want to give us free
money, we should take it and not generate any spurious events.
Define Payee abstraction for use in get_route
A payee can be identified by a pubkey and optionally have an associated
set of invoice features and route hints. Use this in get_route instead
of three separate parameters. This may be included in PaymentPathFailed
later to use when finding a new route.
Remove outdated line from get_route docs
Include PaymentPathRetry data in PaymentPathFailed
When a payment path fails, it may be retried. Typically, this means
re-computing the route after updating the NetworkGraph and channel
scores in order to avoid the failing hop. The last hop in
PaymentPathFailed's path field contains the pubkey, amount, and CLTV
values needed to pass to get_route. However, it does not contain the
payee's features and route hints from the invoice.
Include the entire set of parameters in PaymentPathRetry and add it to
the PaymentPathFailed event. Add a get_retry_route wrapper around
get_route that takes PaymentPathRetry. This allows an EventHandler to
retry failed payment paths using the payee's route hints and features.
Use option TLV decoding for short_channel_id
Using ignorable TLV decoding is only applicable for an Option containing
an enum, but short_channel_id is an Option<u64>. Use option TLV encoding
instead.
Make `Payee::pubkey` pub.
`Payee` is expected to be used by users to get routes for payment
retries, potentially with their own router. Thus, its helpful if it
is pub, even if it is redundant with the last hop in the `path`
field in `Events::PaymentPathFailed`.
Copy `Payee` into `Route`s to provide them to `ChannelManager`
Store `Payee` information in `HTLCSource::OutboundRoute`.
This stores and tracks HTLC payee information with HTLCSource info,
allowing us to provide it back to the user if the HTLC fails and
ensuring persistence by keeping it with the HTLC itself as it
passes between Channel and ChannelMonitor.
Expose log_bytes! macro for use in other crates
Needed to log PaymentHash in the lightning-invoice crate when retrying
payments.
Add PaymentId to PaymentSent event
The payment_hash may not uniquely identify the payment if it has been
reused. Include the payment_id in PaymentSent events so it can
correlated with the send_payment call.
Add PaymentId to PaymentPathFailed event
The PaymentId is needed when retrying payments. Include it in the
PaymentPathFailed event so it can be used in that manner.
Rewrite Invoice's interface in terms of msats
InvoiceBuilder's interface was changed recently to work in terms of
msats. Update Invoice's interface to return the amount in msats, too,
and make amount_pico_btc private.
Unify route finding methods
An upcoming Router interface will be used for finding a Route both when
initially sending a payment and also when retrying failed payment paths.
Unify the three varieties of get_route so the interface can consist of a
single method implemented by the new `find_route` method. Give get_route
pub(crate) visibility so it can still be used in tests.
Add InvoicePayer for retrying failed payments
When a payment fails, it's useful to retry the payment once the network
graph and channel scores are updated. InvoicePayer is a utility for
making payments which will retry any failed payment paths for a payment
up to a configured number of total attempts. It is parameterized by a
Payer and Router for ease of customization and testing.
Implement EventHandler for InvoicePayer as a decorator that intercepts
PaymentPathFailed events and retries that payment using the parameters
from the event. It delegates to the decorated EventHandler after retries
have been exhausted and for other events.
Support paying zero-value invoices
Fail payment retry if Invoice is expired
According to BOLT 11:
- after the `timestamp` plus `expiry` has passed
- SHOULD NOT attempt a payment
Add a convenience method for checking if an Invoice has expired, and use
it to short-circuit payment retries.
Implement Payer and Router for lightning crate
Implements Payer for ChannelManager and Rotuer for find_route, which can
be used to parameterize InvoicePayer when needing payment retries.
Add a utility trait in `router` to get the fees along a given path
Pass the failing/succeeding `Path` to PendingOutboundPayment meths
This will make the next commit much simpler
Track the amount spent on fees as payments are retried
Especially once we merge the `InvoicePayer` logic soon, we'll want
to expose the total fee paid in the `PaymentSent` event.
Correct send-bounding logic in `TestRoutingMessageHandler`
The `cmp::min` appeared to confused `end` for a count.
Add `PeerManager::disconnect_all_peers` to avoid complexity in BP
In the coming commits simply calling `timer_tick_occurred` will no
longer disconnect all peers, so its helpful to have a utility
method.
Constify the ratio in buf limits between forward and init sync msgs
Util-ify enqueueing an encoded message in peer_handler
This marginally simplifies coming commits.
Give peers which are sending us messages longer to respond to ping
See comment for rationale.
Give peers one timer tick to finish handshake before disconnecting
This ensures we don't let a hung connection stick around forever if
the peer never completes the initial handshake.
This also resolves a race where, on receiving a second connection
from a peer, we may reset their_node_id to None to prevent sending
messages even though the `channel_encryptor`
`is_ready_for_encryption()`. Sending pings only checks the
`channel_encryptor` status, not `their_node_id` resulting in an
`unwrap` on `None` in `enqueue_message`.
Log peer public key more thoroughly when logging in peer_handler
Notify scorer of failing payment path and channel
Upon receiving a PaymentPathFailed event, the failing payment may be
retried on a different path. To avoid using the channel responsible for
the failure, a scorer should be notified of the failure before being
used to find a new route.
Add a payment_path_failed method to routing::Score and call it in
InvoicePayer's event handler. Introduce a LockableScore parameterization
to InvoicePayer so the scorer is locked only once before calling
find_route.
Penalize failed channels in Scorer
As payments fail, the channel responsible for the failure may be
penalized. Implement Scorer::payment_path_failed to penalize the failed
channel using a configured penalty. As time passes, the penalty is
reduced using exponential decay, though penalties will accumulate if the
channel continues to fail. The decay interval is also configurable.
Test InvoicePayer in BackgroundProcessor
Proof of concept showing InvoicePayer can be used with an
Arc<ChannelManager> passed to BackgroundProcessor. Likely do not need to
merge this commit.
Move PaymentId to a [u8; 32] in bindings as for other hash objects
This should allow us to fix
https://github.com/lightningdevkit/ldk-garbagecollected/issues/52
Provide payment retry data when an MPP payment failed partially
This will allow `InvoicePayer` to properly retry payments that only
partially failed to send.
Expand `InvoicePayer` documentation somewhat to clarify edge-cases
Dont unwrap `RouteParameter::expiry_time` as users can set it
Users can provide anything they want as `RouteParameters` so we
shouldn't assume any fields are set any particular way, including
`expiry_time` set at all.
Rewrite InvoicePayer retry to correctly handle MPP partial failures
This rewrites a good chunk of the retry logic in `InvoicePayer` to
address two issues:
* it was not considering the return value of `send_payment` (and
`retry_payment`) may indicate a failure on some paths but not
others,
* it was not considering that more failures may still come later
when removing elements from the retry count map. This could
result in us seeing an MPP-partial-failure, failing to retry,
removing the retries count entry, and then retrying other parts,
potentially forever.
Add an integration test for InvoicePayer paying when one part fails
This tests the multi-part-single-failure-immediately fixes in the
previous commit.
Add integration test for InvoicePayerretry on an immediate failure
Check for invoice expiry in InvoicePayer before we send any HTLCs
Parameterize NetGraphMsgHandler with NetworkGraph
NetworkGraph is owned by NetGraphMsgHandler, but DefaultRouter requires
a reference to it. Introduce shared ownership to NetGraphMsgHandler so
that both can use the same NetworkGraph.
Make NetGraphMsgHandler::network_graph private
Since NetworkGraph has shared ownership, NetGraphMsgHandler does not
need to expose its field.
Clarify Scorer docs around penalizing channels
Refactor channel failure penalty logic
Move channel failure penalty logic into a ChannelFailure abstraction.
This encapsulates the logic for accumulating penalties and decaying them
over time. It also is responsible for the no-std behavior. This cleans
up Scorer and will make it easier to serialize it.
Parameterize Scorer by a Time trait
Scorer uses time to determine how much to penalize a channel after a
failure occurs. Parameterizing it by time cleans up the code such that
no-std support is in a single AlwaysPresent struct, which implements the
Time trait. Time is implemented for std::time::Instant when std is
available.
This parameterization also allows for deterministic testing since a
clock could be devised to advance forward as needed.
Implement (de)serialization for Scorer
Scorer should be serialized to retain penalty data between restarts.
Implement (de)serialization for Scorer by serializing last failure times
as duration since the UNIX epoch. For no-std, the zero-Duration is used.
Remove trailing ;s from macro calls to silence new rustc warnings
Rename Payee::new to Payee::from_node_id to clarify it somewhat
This also differentiates it from the bindings default-constructed
`new` method which is constructed when all fields are exposed and
of mappable types.
Make payment_path_failed path type bindings-mappable
The bindings don't currently support passing `Vec`s of objects
which it mappes as "opaque types". This is because it will require
clones to convert its own list of references to Rust's list of
objects.
In the near future we should resolve this limitation, allowing us
to revert this (and make `find_route`'s method signature similarly
cleaner), but for now we must avoid `Vec<OpaqueType>`.
Remove now-unused import in routing/mod.rs
Add `(C-not exported)` tag to a `Payee` modifier with move semantics
This matches the other `Payee` move-modifier functions.
Add `(C-not exported)` tags as required in tuple types
This prepares us for C bindings auto-exporting tuple type fields.
Tweak serialization of ScorerUsingTime for better forward compat
Update CHANGELOG for 0.0.103
Bump crate versions to 0.0.103/invoice 0.11
Fix `cargo doc` on older rustc
Apparently at least rustc 1.48 doesn't support `Self` in doc links,
so we make it explicit.
Add a ChannelTypeFeatures features object for the new channel_type
Its semantics are somewhat different from existing features,
however not enough to merit a different struct entirely.
Specifically, it only supports required features (if you send a
channel_type, the counterparty has to accept it wholesale or try
again, it cannot select only a subset of the flags) and it is
serialized differently (only appearing in TLVs).
Support de/ser of the new channel_type field in open_channel
Support send/recv'ing the new channel_type field in open_channel
This implements the channel type negotiation, though as we currently
only support channels with only static_remotekey set, it doesn't
implement the negotiation explicitly.
0.0.103 CHANGELOG tweaks from Jeff
Add note about PaymentId fields to 0.0.103 changelog
Add SinceEpoch time to test Scorer hermetically
In order to test Scorer hermetically, sleeps must be avoided. Add a
SinceEpoch abstraction for manually advancing time. Implement the Time
trait for SinceEpoch so that it can be used with ScorerUsingTime in
tests.
Add unit tests for Scorer
Test basic and channel failure penalties, including after a
(de-)serialization round trip.
Log before+after ChannelMonitor/Manager updates for visibility
I realized on my own node that I don't have any visibility into how
long a monitor or manager persistence call takes, potentially
blocking other operations. This makes it much more clear by adding
a relevant log_trace!() print immediately before and immediately
after persistence.
Fix to_remote output redeemscript when anchors enabled
Renamed script_for_p2wpkh to get_p2wpkh_redeemscript to match convention
Fix a minor memory leak on PermanentFailure mon errs when sending
If we send a payment and fail to update the first-hop channel state
with a `PermanentFailure` ChannelMonitorUpdateErr, we would have an
entry in our pending payments map, but possibly not return the
PaymentId back to the user to retry the payment, leading to a (rare
and relatively minor) memory leak.
Use upstream rust-bitcoin's dust calculation instead of our own
Not only does this move to common code, but it fixes handling of
all output types except for a few trivial cases.
Be less aggressive in outbound HTLC CLTV timeout checks
We currently assume our counterparty is naive and misconfigured and
may force-close a channel to get an HTLC we just forwarded them.
There shouldn't be any reason to do this - we don't have any such
bug, and we shouldn't start by assuming our counterparties are
buggy. Worse, this results in refusing to forward payments today,
failing HTLCs for largely no reason.
Instead, we keep a fairly conservative check, but not one which
will fail HTLC forwarding spuriously - testing only that the HTLC
doesn't expire for a few blocks from now.
Fixes #1114.
Correct Channel type serialization logic
Currently, we write out the Channel's `ChannelTypeFeatures` as an
odd type, implying clients which don't understand the
`ChannelTypeFeatures` field can simply ignore it. This is obviously
nonsense if the channel type is some future version - the client
needs to fail to deserialize as it doesn't understand the channel's
type.
We adapt the serialization logic here to only write out the
`ChannelTypeFeatures` field if it is something other than
only-static-remote-key, and simply consider that "default" (as it
is the only supported type today). Then, we write out the channel
type as an even TLV, implying clients which do not understand it
must fail to read the `Channel`.
Note that we do not need to bother reserving the TLV type no longer
written as it never appeared in a release (merged post-0.0.103).
Refactor InvoicePayer for spontaneous payments
To support spontaneous payments, InvoicePayer's sending logic must be
invoice-agnostic. Refactor InvoicePayer::pay_invoice_internal such that
invoice-specific code is in pay_invoice_using_amount and the remaining
logic is in pay_internal.
Further refactor the code's payment_cache locking such that it is
accessed consistently when needed, and tidy up the code a bit.
Support spontaneous payments in InvoicePayer
InvoicePayer handles retries not only when handling PaymentPathFailed
events but also for some types of PaymentSendFailure on the initial
send. Expand InvoicePayer's interface with a pay_pubkey function for
spontaneous (keysend) payments. Add a send_spontaneous_payment function
to the Payer trait to support this and implement it for ChannelManager.
Replace expect_value_msat with expect_send
Modify all InvoicePayer unit tests to use expect_send instead of
expect_value_msat, since the former can discern whether the send was for
an invoice, spontaneous payment, or a retry. Updates tests to set payer
expectations if they weren't already and assert these before returning a
failure.
Test retrying payment on partial send failure
Add some test coverage for when InvoicePayer retries within pay_invoice
because the Payer returned a partial failure.
Add PaymentHash parameter to Router::find_route
Implementations of Router may need the payment hash in order to look up
pre-computed routes from a probe for a given payment. Add a PaymentHash
parameter to Router::find_route to allow for this.
Provide `Score` the HTLC amount and channel capacity
This should allow `Score` implementations to make substantially
better decisions, including of the form "willing to pay X to avoid
routing over this channel which may have a high failure rate".
Penalize large HTLCs relative to channels in default `Scorer`
Sending HTLCs which are any greater than a very small fraction of the
channel size tend to fail at a much higher rate. Thus, by default
we start applying a penalty at only 1/8th the channel size and
increase it linearly as the amount reaches the channel's capacity,
20 msat per 1024th of the channel capacity.
Move `Score` into a `scoring` module instead of a top-level module
Traits in top-level modules is somewhat confusing - generally
top-level modules are just organizational modules and don't contain
things themselves, instead placing traits and structs in
sub-modules. Further, its incredibly awkward to have a `scorer`
sub-module, but only have a single struct in it, with the relevant
trait it is the only implementation of somewhere else. Not having
`Score` in the `scorer` sub-module is further confusing because
it's the only module anywhere that references scoring at all.
Return `ClosureReason` from `Channel` chain update methods
This fixes a few `ClosureReason`s and allows us to have
finer-grained user-visible errors when a channel closes due to an
on-chain event.
Automatically close channels that go unfunded for 2016 blocks
As recommended by BOLT 2 added in
https://github.com/lightningnetwork/lightning-rfc/pull/839
Add 'accept_inbound_channels' config option.
Limit minimum output size to the dust limit when RBF-bumping
Check all outputs meet the dust threshold in check_spends!()
Correct txid logging to reverse bytes.
We also take this opportunity to log the channel being closed when
one is closed by an on-chain spend of the funding output.
Ensure current channel state is logged for all channels on startup
Remove user_payment_id
In upcoming commits, we'll be making the payment secret and payment hash/preimage
derivable from info about the payment + a node secret. This means we don't
need to store any info about incoming payments and can eventually get rid of the
channelmanager::pending_inbound_payments map.
Add a new log-level for gossip messages.
Fix MPP routefinding when we first collect 95% of payment value
See comment in new test for more details.
Introduce new helper commit_tx_fee_sat
Cancel the outbound feerate update if above what we can afford
Check we won't overflow `max_dust_htlc_exposure_msat` at outbound feerate update
Re-add `test_max_dust_htlc_exposure`
Introduce CommitmentStats
Check outbound update_fee affordance incremented with holding cell HTLCs
Add anchor support to commitment HTLC outputs
Increase visibility of anchor related methods
Add anchor support to build_htlc_transaction
Adjust HTLC_{SUCCESS,TIMEOUT}_TX_WEIGHT when anchors used
Add test vectors for get_htlc_redeemscript wrt anchors
Generate PaymentPathSuccessful event for each path
A single PaymentSent event is generated when a payment is fulfilled.
This is occurs when the preimage is revealed on the first claimed HTLC.
For subsequent HTLCs, the event is not generated.
In order to score channels involved with a successful payments, the
scorer must be notified of each successful path involved in the payment.
Add a PaymentPathSuccessful event for this purpose. Generate it whenever
a part is removed from a pending outbound payment. This avoids duplicate
events when reconnecting to a peer.
Make Channel::commit_tx_fee_msat static and take fee explicitly
This may avoid risk of bugs in the future as it requires the caller
to think about the fee being used, not just blindly use the current
(committed) channel feerate.
Rewrite test_update_fee_that_funder_cannot_afford to avoid magic
Instead of magic hard-coded constants, its better for tests to
derive the values used so that they change if constants are changed
and so that it is easier to re-derive constants in the future as
needed.
Correct initial commitment tx fee affordability checks on open
Previously, we would reject inbound channels if the funder wasn't
able to meet our channel reserve on their first commitment
transaction only if they also failed to push enough to us for us
to not meet their initial channel reserve as well.
There's not a lot of reason to care about us meeting their reserve,
however - its largely expected that they may not push enough to us
in the initial open to meet it, and its not actually our problem if
they don't.
Further, we used our own fee, instead of the channel's actual fee,
to calculate fee affordability of the initial commitment
transaction.
We resolve both issues here, rewriting the combined affordability
check conditionals in inbound channel open handling and adding a
fee affordability check for outbound channels as well.
The prior code may have allowed a counterparty to start the channel
with "no punishment" states - violating the reason for the reserve
threshold.
Test fixed channel reserve checks on channel open
Fix compilation with the max_level_trace feature
Test all log-limiting features in CI
Support `logger::Record` in C by String-ing the fmt::Arguments
This adds a new (non-feature) cfg argument `c_bindings` which will
be set when building C bindings. With this, we can (slightly) tweak
behavior and API based on whether we are being built for Rust or C
users.
Ideally we'd never need this, but as long as we can keep the API
consistent-enough to avoid material code drift, this gives us a
cheap way of doing the "right" thing for both C and Rust when the
two are in tension.
We also move lightning-background-processor to support the same
MSRV as the main lightning crate, instead of only
lightning-net-tokio's MSRV.
Make `Score : Writeable` in c_bindings and impl on `LockedScore`
Ultimately we likely need to wrap the locked `Score` in a struct
that exposes writeable somehow, but because all traits have to be
fully concretized for C bindings we'll still need `Writeable` on
all `Score` in order to expose `Writeable` on the locked score.
Otherwise, we'll only have a `LockedScore` with a `Score` visible
that only has the `Score` methods, never the original type.
Seal `scoring::Time` and only use `Instant` or `Eternity` publicly
`scoring::Time` exists in part to make testing the passage of time
in `Scorer` practical. To allow no-std users to provide a time
source it was exposed as a trait as well. However, it seems
somewhat unlikely that a no-std user is going to have a use for
providing their own time source (otherwise they wouldn't be a
no-std user), and likely they won't have a graph in memory either.
`scoring::Time` as currently written is also exceptionally hard to
write C bindings for - the C bindings trait mappings relies on the
ability to construct trait implementations at runtime with function
pointers (i.e. `dyn Trait`s). `scoring::Time`, on the other hand,
is a supertrait of `core::ops::Sub` which requires a `sub` method
which takes a type parameter and returns a type parameter. Both of
which aren't practical in bindings, especially given the
`Sub::Output` associated type is not bound by any trait bounds at
all (implying we cannot simply map the `sub` function to return an
opaque trait object).
Thus, for simplicity, we here simply seal `scoring::Time` and make
it effectively-private, ensuring the bindings don't need to bother
with it.
Implement Clone, Hash, PartialEq for ClosingTransaction
This is a public struct intended to be used as an object by users,
so it should likely have common implementations, given they're
trivial.
Implement Clone for InvalidShutdownScript
Users hopefully shouldn't have much of a reason to use this, but
the bindings may need it to ensure no leaking pointers over an ffi.
Store holder channel reserve and max-htlc-in-flight explicitly
Previously, `holder_selected_channel_reserve_satoshis` and
`holder_max_htlc_value_in_flight_msat` were constant functions
of the channel value satoshis. However, in the future we may allow
allow users to specify it. In order to do so, we'll need to track
them explicitly, including serializing them as appropriate.
We go ahead and do so here, in part as it will make testing
different counterparty-selected channel reserve values easier.
Explicitly support counterparty setting 0 channel reserve
A peer providing a channel_reserve_satoshis of 0 (or less than our
dust limit) is insecure, but only for them. Because some LSPs do it
with some level of trust of the clients (for a substantial UX
improvement), we explicitly allow it. Because its unlikely to
happen often in normal testing, we test it explicitly here.
Fix regression when reading `Event::PaymentReceived` in some cases
For some reason rustc was deciding on a type for the `Option` being
deserialized for us as `_user_payment_id`. This really, really,
absolutely should have been a compile failure - the type (with
methods called on it!) was ambiguous! Instead, rustc seems to have
been defaulting to `Option<()>`, causing us to read zero of the
eight bytes in the `user_payment_id` field, which returns an
`Err(InvalidValue)` error as TLVs must always be read fully.
This should likely be reported to rustc as its definitely a bug,
but I cannot seem to cause the same error on any kinda of
vaguely-minimized version of the same code.
Found by `chanmon_consistency` fuzz target.
Fix compilation in `payment` rustdoc examples
The samples were not valid rust, but previous versions of rustc had
a bug where they were accepted anyway. Latest rustc beta no longer
accepts these.
Score successful payment paths
Expand the Score trait with a payment_path_successful function for
scoring successful payment paths. Called by InvoicePayer's EventHandler
implementation when processing PaymentPathSuccessful events. May be used
by Score implementations to revert any channel penalties that were
applied by calls to payment_path_failed.
Decay channel failure penalty upon success
If a payment failed to route through a channel, a penalty is applied to
the channel in the future when finding a route. This penalty decays over
time. Immediately decay the penalty by one half life when a payment is
successfully routed through the channel.
Fix shift overflow in Scorer::channel_penalty_msat
An unchecked shift of more than 64 bits on u64 values causes a shift
overflow panic. This may happen if a channel is penalized only once and
(1) is not successfully routed through and (2) after 64 or more half
life decays. Use a checked shift to prevent this from happening.
Allow missing-docs on test-only macros
Prefer fully-specified paths in test macros
This avoids macros being context-specific use-dependent.
Continue after a single failure in `ChannelMonitor::update_monitor`
`ChannelMonitorUpdate`s may contain multiple updates, including, eg
a payment preimage after a commitment transaction update. While
such updates are generally not generated today, we shouldn't return
early out of the update loop, causing us to miss any updates after
an earlier update fails.
Always return failure in `update_monitor` after funding spend
Previously, monitor updates were allowed freely even after a
funding-spend transaction confirmed. This would allow a race
condition where we could receive a payment (including the
counterparty revoking their broadcasted state!) and accept it
without recourse as long as the ChannelMonitor receives the block
first, the full commitment update dance occurs after the block is
connected, and before the ChannelManager receives the block.
Obviously this is an incredibly contrived race given the
counterparty would be risking their full channel balance for it,
but its worth fixing nonetheless as it makes the potential
ChannelMonitor states simpler to reason about.
The test in this commit also tests the behavior changed in the
previous commit.
Drop `MonitorUpdateErr` in favor of opaque errors.
We don't expect users to ever change behavior based on the string
contained in a `MonitorUpdateErr`, except log it, so there's little
reason to not just log it ourselves and return a `()` for errors.
We do so here, simplifying the callsite in `ChainMonitor` as well.
Make APIError debug output more clear by including the variant
Add a simple test for ChainMonitor MonitorUpdate-holding behavior
Add a test for MonitorEvent holding when they complete out-of-order
Add trivial test of monitor update failure during block connection
Remove OnionV2 parsing support
OnionV2s don't (really) work on Tor anymore anyway, and the field
is set for removal in the BOLTs [1]. Sadly because of the way
addresses are parsed we have to continue to understand that type 3
addresses are 12 bytes long. Thus, for simplicity we keep the
`OnionV2` enum variant around and just make it an opaque 12 bytes,
with the documentation updated to note the deprecation.
[1] https://github.com/lightning/bolts/pull/940
Ensure ChannelManager methods are idempotent
During event handling, ChannelManager methods may need to be called as
indicated in the Event documentation. Ensure that these calls are
idempotent for the same event rather than panicking. This allows users
to persist events for later handling without needing to worry about
processing the same event twice (e.g., if ChannelManager is not
persisted but the events were, the restarted ChannelManager would return
some of the same events).
Define public getters for all feature flags
There's not a ton of reason not to do this, and moving it to the
macro removes some code.
Support the `channel_type` feature bit.
Note that this feature bit does absolutely nothing. We signal it
(as we already support channel type negotiation), but do not bother
to look to see if peers support it, as we don't care - we simply
look for the TLV entry and deduce if a peer supports channel type
negotiation from that.
The only behavioral change at all here is that we don't barf if a
peer sets channel type negotiation to required via the feature bit
(instead of failing the channel at open-time), but of course no
implementations do this, and likely won't for some time (if ever -
you can simply fail channels with unknown types later, and there's
no reason to refuse connections, really).
As defined in https://github.com/lightning/bolts/pull/906
Reduce force-closures with user fee estimators which round poorly
See comment for more
Upgrade to codecov uploader v2
Some time ago codecov stopped supporting their old v1 uploader, and
it seems they've now finally turned it off, so we aren't getting
any coverage reports anymore. Hopefully upgrading is pretty trivial.
Getter for the total channel balance
The existing balance getters subtract reserve, this one does not.
Separate ChannelAnnouncement and ChannelUpdate broadcast conditions
When a `ChannelUpdate` message is generated for broadcast as a part
of a `BroadcastChannelAnnouncement` event, it may be newer than our
previous `ChannelUpdate` and need to be broadcast. However, if the
`ChannelAnnouncement` had already been seen we wouldn't
re-broadcast either message as the `handle_channel_announcement`
call would fail, short-circuiting the condition to broadcast both.
Instead, we split the broadcast of each message as well as the
conditional so that we always attempt to handle each message and
update our local graph state, then broadcast the message if its
update was processed successfully.
Update `ChannelUpdate::timestamp` when channels are dis-/en-abled
We update the `Channel::update_time_counter` field (which is copied
into `ChannelUpdate::timestamp`) only when the channel is
initialized or closes, and when a new block is connected. However,
if a peer disconnects or reconnects, we may wish to generate
`ChannelUpdate` updates in between new blocks. In such a case, we
need to make sure the `timestamp` field is newer than any previous
updates' `timestamp` fields, which we do here by simply
incrementing it when the channel status is changed.
As a side effect of this we have to update
`test_background_processor` to ensure it eventually succeeds even
if the serialization of the `ChannelManager` changes after the test
begins.
Re-broadcast our own gossip even if its same as the last broadcast
Even if our gossip hasn't changed, we should be willing to
re-broadcast it to our peers. All our peers may have been
disconnected the last time we broadcasted it.
Add a comment describing `update_time_counter` and when its updated
Add a comment describing the timestamp field's use
DRY up payment failure macros in functional_test_utils
... with a more extensible expectation-checking framework for them.
Add a variant to `PendingOutboundPayment` for retries-exceeded
When a payer gives up trying to retry a payment, they don't know
for sure what the current state of the event queue is.
Specifically, they cannot be sure that there are not multiple
additional `PaymentPathFailed` or even `PaymentSuccess` events
pending which they will see later. Thus, they have a very hard
time identifying whether a payment has truly failed (and informing
the UI of that fact) or if it is still pending. See [1] for more
information.
In order to avoid this mess, we will resolve it here by having the
payer give `ChannelManager` a bit more information - when they
have given up on a payment - and using that to generate a
`PaymentFailed` event when all paths have failed.
This commit adds the neccessary storage and changes for the new
state inside `ChannelManager` and a public method to mark a payment
as failed, the next few commits will add the new `Event` and use
the new features in our `PaymentRetrier`.
[1] https://github.com/lightningdevkit/rust-lightning/issues/1164
Expose an event when a payment has failed and retries complete
When a payment fails, a payer needs to know when they can consider
a payment as fully-failed, and when only some of the HTLCs in the
payment have failed. This isn't possible with the current event
scheme, as discovered recently and as described in the previous
commit.
This adds a new event which describes when a payment is fully and
irrevocably failed, generating it only after the payment has
expired or been marked as expired with
`ChannelManager::mark_retries_exceeded` *and* all HTLCs for it
have failed. With this, a payer can more simply deduce when a
payment has failed and use that to remove payment state or
finalize a payment failure.
Use `Event::PaymentFailed` in `InvoicePayer` to remove retry count
This finally fixes the bug described in the previous commits where
we retry a payment after its retry count has expired due to early
removal of the payment from the retry count tracking map. A test is
also added which demonstrates the bug in previous versions and
which passes now.
Fixes #1164.
Make attempting to retry a succeeded payment an APIError, not Route
This is symmetric with the new failure once a payment is abandoned.
Updates Fee estimator docs to include a max return value for get_est_sat_per_1000_weight
Updates docs to include a max return value for get_est_sat_per_1000_weight
Adds max to both conversions
Additional detail
Add mpp_timeout and invalid_onion_payload descriptions & handling
Pin tokio to 1.14.0 in CI for older rustc's
Build no-std on 1.47 now that core2 supports it
DRY up network_graph tests substantially with message creation fns
Drop `allow_wallclock_use` feature in favor of simply using `std`
Fixes #1147.
Add a method to prune stale channels from `NetworkGraph`
We define "stale" as "haven't heard an updated channel_update in
two weeks", as described in BOLT 7.
We also filter out stale channels at write-time for `std` users, as
we can look up the current time.
Reject channel_update messages with timestamps too old or new
Because we time out channel info that is older than two weeks now,
we should also reject new channel info that is older than two
weeks, in addition to rejecting future channel info.
Automatically prune NetworkGraph of stale channels hourly in BP
Add get_inbound_payment_key_material to KeysInterface
This will allow us to retrieve key material for encrypting/decrypting inbound
payment info, in upcoming commits
Macro-ize checking that the total value of an MPP's parts is sane
This DRY-ed code will be used in upcoming commits when we stop storing inbound
payment data
Add get_single_block to chacha20 module
In the next commit, we'll want to get a single block from a chacha stream that
takes a 16-byte nonce.
Drop need to store pending inbound payments
and replace payment_secret with encrypted metadata
See docs on `inbound_payment::verify` for details
Also add min_value checks to all create_inbound_payment* methods
Add new invoice CreationError::InvalidAmount for use in checking `create_inbound_payment`
in an invoice creation utility. Note that if the error type of `create_inbound_payment` ever
changed, we'd be forced to update the invoice utility's callsite to handle the new error
Salt inbound payment ExpandedKey
Leftover feedback from #1177
create_inbound_payment: warn about dup hashes
Leftover feedback from #1177
inbound_payment: DRY verify method for use in getting preimage
in the next commit(s)
inbound_payment: Add utility to get payment preimage given hash/secret
User-requested feature
Remove trailing whitespace in the FeeEstimator interface
Update CHANGELOG for 0.0.104
Bump versions to 0.0.104/invoice 0.12
Swap around generic argument ordering in DefaultRouter for bindings
The bindings generation really should support default generic types
in where clauses, but currently does not. To avoid needing to add
support during the current release process, we simply swap around
the arguments to move them to the first <> instead of the where.
Add a constructor to MultiThreadedLockableScore
...as otherwise the struct is rather useless.
Add a C-not exported tag to `NetGraphMsgHandler.network_graph`
Swap around generic argument ordering in InvoicePayer for bindings
The bindings generation really should support generic bounds other
than Deref::Target in where clauses, but currently does not. To
avoid needing to add support during the current release process,
we simply swap around the arguments to move them to the first <>
instead of the where.
update repo name to use lightningdevkit
Log gossip rejections due to stale channel_updates at GOSSIP level
This further reduces noise at the TRACE level during initial gossip
sync.
Support building `cfg=c_bindings` with `no-std`
This will be needed for JavaScript bindings eventually.
Adapt lightning-invoice to no_std
Do not turn on bech32/std by default for lightning-invoice
Add a new `WarningMessage` message to send and receive warnings
Handle sending and receiving warning messages
Send warning messages when appropriate in gossip handling pipeline
Convert `shutdown` invalid script checks to warning messages
As required by the warning messages PR, we should simply warn our
counterparty in this case and let them try again, continuing to try
to use the channel until they tell us otherwise.
Send `warning` instead of `error` when we incounter bogus gossip
Rely on Error/Warning message data lengths being correct
In https://github.com/lightning/bolts/pull/950, the (somewhat
strange) requirement that error messages be handled even if the
length field is set larger than the size of the package was
removed. Here we change the code to drop the special handling for
this, opting to just fail to read the message if the length is
incorrect.
Set the SigHashType of remote htlc signatures w/ anchors to SinglePlusAnyoneCanPay
Isolated channelmonitor weight unit tests and added anchor loops
make WEIGHT{_REVOKED,}_{OFFERED,RECEIVED}_HTLC functions with opt_anchors parameter
Add unit test coverage for package::weight_{offered,received}_htlc
Convert COMMITMENT_TX_BASE_WEIGHT to anchor-aware function
Convert HTLC_{SUCCESS,TIMEOUT}_TX_WEIGHT to anchor-aware functions
Debit funder's output to cover anchors
Add anchor tests to outbound_commitment_test
Set opt_anchors for calls to CommitmentTransaction::new_with_auxiliary_htlc_data
Fixed comment on weight_received_htlc
Make lockorder consistent in channelmanager
This resolves a lockorder inversion in
`ChannelManager::finalize_claims` where `pending_outbound_payments`
is locked after `pending_events`, opposite of, for example, the
lockorder in `ChannelManager::fail_htlc_backwards_internal` where
`pending_outbound_payments` is locked at the top of the
`HTLCSource::OutboundRoute` handling and then `pending_events` is
locked at the end.
Fix some (non-bug) lock-order-inversions in tests
Check lockorders in tests with a trivial lockorder tracker
Fix build errors
Create script using p2wsh for comparison
Using p2wpkh for generating the payment script
spendable_outputs sanity check
Fix unused return warning for 'transmute_copy'.
Create dependabot.yml
Signed-off-by:…
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds
Transactiontolighting-block-sync::convertso both calls to Rust based transaction deserialisers and calls tobitcoin-cliwhich return transaction data can be converted intoTransaction(e.g.signrawtransactionwithwalletorgetrawtransaction.