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++.
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.
In 9e03087d6a we started setting
`opt-level` only on profile.test and not profile.dev. When that
commit was authored I tested only that rustc was being called with
opt-level set in its flags, not that the resulted run ran at the
speed I expected. It seems profile.test isn't applied properly to
dependencies or so, resulting in tests running much slower than
they do at profile.dev.opt-level=1.
In review of the final doc changes in #649, I noticed there
appeared to be redundant monitored-outpoints function in
`ChannelMonitor` - `get_monitored_outpoints()` and
`get_outputs_to_watch()`.
In 6f08779b04,
get_monitored_outpoints() was added, with its behavior largely the
same as today's - only returning the set of remote commitment txn
outputs that we've learned about on-chain. This is clearly not
sufficient, and in 73dce207dd,
`get_outputs_to_watch` was added which was overly cautious to
ensure nothing was missed. Still, the author of 73dce207dd
(me) seemed entirely unaware of the work in 6f08779b04
(also me), despite the function being the literal next function in
the same file. This is presumably because it was assumed that
`get_monitored_outpoints` referred to oupoints for which we should
monitor for spends of (which is true), while `get_outputs_to_watch`
referred to outpouts which we should monitor for the transaction
containing said output (which is not true), or something of that
nature. Specifically, it is the expected behavior that the only
time we care about `Filter::register_tx` is for the funding
transaction (which we aren't aware of the inputs of), but for all
other transactions we register interest on the basis of an outpoint
in the previous transaction (ie via `Filter::register_output`).
Here we drop the broken-on-day-one `get_monitored_outpoints()`
version, but assert in testing that the values which it would return
are all present in `get_outputs_to_watch()`.
This also pays a fee on the transactions we generate in response to
SpendableOutputDescriptors in tests.
This fixes the known issues in #630, though we should test for
standardness in other ways as well.
This resolves a number of bugs around how we calculate feerates on
closing transactions:
* We previously calculated the weight wrong both by always
counting two outputs instead of counting the number of outputs
that actually exist in the closing transaction and by not
counting the witness redeemscript.
* We use assertions to check the calculated weight matches what we
actually build (with debug_assertions for variable-length sigs).
* As an additional sanity check, we really should check that the
transaction had at least min-relay-fee when we were the channel
initator.
It was noticed (via clippy) by @casey that we were taking and then
immediately dropping the total_consistency_lock because `let _ =`
doesn't actually bind the response to anything. This appears to be
a consequence of wanting `if let Some(_) =` to not hold a ref to
the contained value at all, but is relatively surprising to me.
Previously, we had a concept of "rescaning" blocks when we detected
a need to monitor for a new set of outputs in future blocks while
connecting a block. In such cases, we'd need to possibly learn about
these new spends later in the *same block*, requiring clients who
filter blocks to get a newly-filtered copy of the same block. While
redoing the chain access API, it became increasingly clear this was
an overly complicated API feature, and it seems likely most clients
will not use it anyway.
Further, any client who *does* filter blocks can simply update their
filtering algorithm to include any descendants of matched
transactions in the filter results, avoiding the need for rescan
support entirely.
Thus, it was decided that we'd move forward without rescan support
in #649, however to avoid significant further changes in the
already-large 649, we decided to fully remove support in a
follow-up.
Here, we remove the API features that existed for rescan and fix
the few tests to not rely on it.
After this commit, we now only ever have one possible version of
block connection transactions, making it possible to be
significantly more confident in our test coverage actually
capturing all realistic scenarios.
