Instead of simply always considering a payment-send failure as
acceptable (and aborting fuzzing), we check that a payment send
failure is from a list of errors that we know we can hit, mostly
around maxing out our channel balance.
Critically, we keep going after hitting an error, as there's no
reason channels should get out of sync even if a send fails.
If we receive a preimage for an outgoing HTLC that solves an output on a
backwards force-closed channel, we need to claim the output on-chain.
Note that this commit also gets rid of the channel monitor redundantly setting
`self.counterparty_payment_script` in `check_spend_counterparty_transaction`.
Co-authored-by: Antoine Riard <ariard@student.42.fr>
Co-authored-by: Valentine Wallace <vwallace@protonmail.com>
Now callers will separately retrieve the claim requests/
holder revokable script and the new watched holder outputs.
This will be used in the next commit for times when we
need to get holder claim requests, but don't have access to
the holder commitment transaction.
Helpful for debugging. I also included the change in the provide_preimage method
signature which will be used in an upcoming commit, because commit-wise it was
easier to combine the changes.
If the channel is hitting the chain right as we receive a preimage,
previous to this commit the relevant ChannelMonitor would never
learn of this preimage.
The ChannelMonitor already monitors the chain for counterparties
revealing preimages, and will give the HTLCSources back to the
ChannelManager for claiming. Thus it's unnecessary for the ChannelManager
to monitor these HTLCs itself.
See is_resolving_htlc_output:
- if the counterparty broadcasted and then claimed one of the HTLCs we
offered them, line 2015 is where the ChannelMonitor gives the ChannelManager
the HTLC source
- if we broadcasted and they claimed an HTLC we offered them, line 2025 is
where the ChannelMonitor gives the ChannelManager the HTLC source
Because the C++ wrappers require being able to memset(0) the C
structs to skip free(), we'd previously mapped tuples with two
pointer indirections. However, because all other types already
support memset(0)'ing to disable free() logic, we can skip the
pointer indirections and the behavior is still correct.
In general we should stop enforcing that all lifetimes are static
- we may take references from C and its up to reviewing the diff on
the bindings changes and the user(s) to ensure lifetimes are valid.
Also asserts a success criteria that was missed before.
For non-generic type aliases which are meant as convinient aliases
for more complex types, we need to store the aliased type (with all
paths made absolute) and use that in type resolution.
The most code by far is just making all the paths in a type absolute
but its not too bad either.
In some cases, things which are a Rust Reference (ie slices), we
may still want to map them as a non-reference and need to put a
"mut " in front of the variable name in a function decl. This
worked fine by just checking for the slice case, except that we
are about to add support for type aliases, which no longer match
the naive case.
Instead, we can just have the types module print out the C type and
check if it begins with a '&' to figure out if it is a reference.
New work upstream puts tuples in slices, which is a very reasonable
thing to expect, however we don't know how to generate conversions
for such objects. Making it more complicated, upstream changes also
include references to things inside such slices, which requires
special handling to avoid creating dangling references.
This adds support for converting such objects, noting that slices
need to be converted first into Vecs which own their underlying
objects and then need to map any reference types into references.
A lot of our container mapping depends on the `is_owned` flag
which we have for in-crate mapped objects to map references and
non-references into the same container type. Transaction was
mapped to two completely different types (a slice and a Vec type),
which led to a number of edge cases in the bindings generation.
Specifically, I spent a few days trying to map
`[(A, &Transaction)]` properly and came up empty - we map slices
into the same types as Vecs (and rely on the `is_owned` flag to
avoid double-free) and the lack of one for `Transaction` would have
required a special-case in numerous functions.
Instead, we just add a flag in `Transaction` to mirror what we do
for in-crate types and check it before free-ing any underlying
memory.
Note that, sadly, because the c_types objects aren't mapped as a
part of our C++ bindings generation, you have to manually call
`Transaction_free()` even in C++.
If a persister returns a temporary failure, the channel monitor should be able
to be put on ice and then revived later. If a persister returns a permanent
failure, the channel should be force closed.
Intended to be a cross-platform implementation of the
channelmonitor::Persist trait.
This adds a new lightning-persister crate, that uses the
newly exposed lightning crate's test utilities.
Notably, this crate is pretty small right now. However, due to
future plans to add more data persistence (e.g. persisting the
ChannelManager, etc) and a desire to avoid pulling in filesystem
usage into the core lightning package, it is best for it to be
separated out.
Note: Windows necessitates the use of OpenOptions with the `write`
permission enabled to `sync_all` on a newly opened channel's
data file.
- The ChainMonitor should:
Whenever a new channel is added or updated, these updates
should be conveyed to the persister and persisted to disk.
Even if the update errors while it's being applied, the
updated monitor still needs to be persisted.
We remove test_no_failure_dust_htlc_local_commitment from our test
framework as this test deliberately throwing junk transaction in
our monitoring parsing code is hitting new assertions.
This test was added in #333, but it sounds as an oversight as the
correctness intention of this test (i.e verifying lack of dust
HTLCs canceling back in case of junk commitment transaction) doesn't
currently break.
When mapping an `Option<T>` where T is a mapped trait, we need to
move out of the `*mut T`, however the current generation results in
a `*const T` and a conversion that just takes a reference to the
pointed-to object. This is because the only place this code was
previously used was for slices, which *do* need a reference.
Additionally, we need to know how to convert `Option` containers
which do not contain an opaque type.
Sadly, the easiest way to get the desired result is to add another
special case in container mapping, keeping the current behavior for
slices, but moving out of the pointed-to object for other types.
Currently the check_binidngs GitHub CI job always fails when there
is a new cbindgen release because the cbindgen version is in the
generated header file. When the new version doesn't change the
generated header except for the version number, we should ignore
the difference.
When we have a `Trait` wrapped as an `Option<Trait>`, we called
`<*const Trait>.is_null()` which resulted in rustc trying to take
the most braindead option of dereferencing the whole thing and
hitting a recursive dereference since we
`impl Deref<Target=Trait> for Trait` for all our traits.
Instead, we can be explicit and just compare the pointer directly
with `std::ptr::null()` which avoids this.
A few places got a None in the previous commit to avoid increasing
the diff size. However, it makes sense to have GenericTypes contexts
there, so we pipe them through the neccessary places.
This pushes down the logic to check if a given Path is, in fact,
a reference to a generic into the common maybe_resolve_path instead
of doing it redundantly in a few different places. Net loss of LoC.
This test is a mutation to underscore the detetection logic bug
we had before #653. HTLC value routed is above the remaining
balance, thus inverting HTLC and `to_remote` output. HTLC
will come second and it wouldn't be seen by pre-#653 detection
as we were eneumerate()'ing on a watched outputs vector (Vec<TxOut>)
thus implictly relying on outputs order detection for correct
spending children filtering.
Previously, outputs were monitored based on txid and an index yelled
from an enumeration over the returned selected outputs by monitoring
code. This is always have been broken but was only discovered while
introducing anchor outputs as those ones rank always first per BIP69.
We didn't have test cases where a HTLC was bigger than a party balance
on a holder commitment and thus not ranking first.
Next commit introduce test coverage.
