Fail holding-cell AddHTLCs on Channel deser to match disconnection

As Channel::write says in the comment at the top: "we write out as
if remove_uncommitted_htlcs_and_mark_paused had just been called",
except that we previously deliberately included holding-cell
AddHTLC events in the serialization. On the flip side, in
remove_uncommitted_htlcs_and_mark_paused, we removed pending
AddHTLC events under the assumption that, if we can't forward
something ASAP, its better to fail it back to the origin than to
sit on it for a while.

Given there's likely to be just as large a time-lag between
ser/deserialization as between when a peer dis/reconnects, there
isn't much of a reason for this difference. Worse, we debug_assert
that there are no pending AddHTLC holding cell events when doing a
reconnect, so any tests or fuzzers which deserialized a
ChannelManager with AddHTLC events would panic.

We resolve this by adding logic to fail any holding-cell AddHTLC
events upon deserialization, in part because trying to forward it
before we're sure we have an up-to-date chain is somewhat risky -
the sender may have already gone to chain while our upstream has
not.

Merge pull request #611 from valentinewallace/fix-missing-htlc-claim

Tell ChannelMonitors about HTLCs fulfilled after channel close

Claim HTLC output on-chain if preimage is recv'd after force-close

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>

Split channelmonitor's broadcast_by_holder_state

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.

Extend update_monitor logging

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.

Update monitor with preimage after channel close

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.

Add prev_channel_outpoint to previous hop data

This will be used in upcoming commits to allow us to update a channel
monitor with a preimage after its channel has closed.

Merge pull request #721 from TheBlueMatt/2020-09-649-bindings

Bindings Updates for #649

Merge pull request #745 from ariard/2020-11-getting-started

Add a Getting Started in CONTRIBUTING.md

Merge pull request #746 from valentinewallace/remove-todo

Remove unnecessary todo

Add a Getting Started

Fix by Steve Lee <moneyball@users.noreply.github.com>

Remove unnecessary todo

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

[bindings] update generated code

Update demo.cpp for new tuple code

[bindings] Un-Box Tuple mapping

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.

[bindings] Fix typo in opaque struct docs found by Val

[bindings] Use the same SipHash keys to make C++ header stable

[bindings] Use enum to describe deref'ing needed for Option<> inners

Update bindings to latest upstream code

Drop the now-unused usizeslice bindings struct

Update bindings demo apps for new code upstream

Move a struct in bindings up to define it before it is used

This is a limitations in the bindings crate, but not one that's
going to be fixed right now.

[bindings] Drop one static-lifetime restriction and check success

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.

[bindings] Handle type X = Y aliasing in type resolution

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.

[bindings] Handle ::-prefixed paths in a few places

[bindings] Remove some uneccessary no-multi-ident path restrictions

[bindings] Avoid guessing whether resolved type is a ref in blocks

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.

[bindings] Support mapping slices which contain tuples (with refs)

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.

[bindings] Give Transaction objects a buffer-is-owned flag.

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++.

Merge pull request #681 from valentinewallace/data-persister-refactor

Add ChannelDataPersister trait and point ChainMonitor to it.

Fix clippy

Rename ChannelMonitor::write_for_disk --> serialize_for_disk

This function does not necessarily write to disk, it can serialize to anything
that implements Writer.

Test that Persist temp and perm failures behave as expected.

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.

Add a sample module FilesystemPersister.

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.

Integrate Persist into ChainMonitor.

- 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.

Add Persist trait.

Implementors of Persist are responsible for persisting new ChannelMonitors
and ChannelMonitor updates to disk/backups.

Put test utilities behind a feature flag.

The utilities will still be part of cfg(test). The purpose of this
is to enable other crates in the workspace to use the test utilities.

Merge pull request #653 from ariard/2020-06-fix-outputs-tracking

Add outpoint index in watch_outputs to fix tracking

Assert on correct registeration of outputs index

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.

[bindings] Add support for `Option<T>` where T is a mapped trait

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.

Ignore cbindgen version in latest-bindings-in-git check.

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.

[bindings] Use == null() instead of is_null() to avoid ambiguity

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.

[bindings] Include a GenericTypes context in more places

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.

[bindings] Push generic resolution into resolve_path

Like the previous commit pushing into maybe_resolve_path, this
makes generic resolution a part of type resolution everywhere.

[bindings] Push generic resolution into maybe_resolve_path

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.

Support `use ident;` in bindings

Somehow we'd never had any cases of it and it requires an extra
(trivial) match arm.

Add README note about key validity assertions at the FFI boundary

Add test_htlc_no_detection

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.

Add transaction index in watched_outputs

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.

Merge pull request #738 from TheBlueMatt/2020-10-opt-test

Fix passing -O1 to build from `cargo test`

Merge pull request #740 from rloomba/rloomba/trivial-clippy-fixes

trivial clippy warning fixes

trivial changes to fix clippy::write_with_newline warnings

Merge pull request #739 from rloomba/rloomba/add-clippy-to-travis

Add clippy to Travis integration

add linting to Github CI

add clippy to travis integration

fix all clippy::redundant_field_names warnings

Fix passing -O1 to build from `cargo test`

In 9e03087d6acbc876a5ad1c9b9d8746bf18d5ca86 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.

Merge pull request #734 from TheBlueMatt/2020-10-ci-no-git-conf

Don't let CI fail to check commits b/c git isn't configured in CI

Merge pull request #722 from TheBlueMatt/2020-09-broken-fn

Drop the redundant/broken `ChannelMonitor::get_monitored_outpoints`

Drop now-unused Vec of outpoints in remote-commitment-tx-tracking

This nearly fully reverts 6f08779b0439e7e4367a75f4ee88de093dfb68cb,
removing the extra data storage that it added.

Drop the redundant/broken `ChannelMonitor::get_monitored_outpoints`

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 6f08779b0439e7e4367a75f4ee88de093dfb68cb,
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 73dce207dd0ea6c3ac57af3ebb8b87ee03e82c9e,
`get_outputs_to_watch` was added which was overly cautious to
ensure nothing was missed. Still, the author of 73dce207dd0ea6c3ac5
(me) seemed entirely unaware of the work in 6f08779b0439e7e4367a75f
(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()`.

Merge pull request #716 from TheBlueMatt/2020-09-fee-tests

(Fix and) Test that txn pay at least a minimum relay fee in functional tests

Test that txn pay at least a minimum relay fee in functional tests

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.

Fix feerate calculation on closing transactions

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.

Fix max fee_satoshis constant to be 21 million, not 2.1 million.

Though hopefully we never see a fee of 2.1 million BTC, either...

Don't let CI fail to check commits b/c git isn't configured in CI

This fixes #733 by just setting a dummy git name/email before
calling `git rebase` in CI.

Merge pull request #731 from TheBlueMatt/2020-10-fix-big-lock

Actually hold the total_consistency_lock instead of take-and-drop

Merge pull request #730 from TheBlueMatt/2020-09-no-rescan

Drop last bits of rescan as its too complicated to be worth having

Actually hold the total_consistency_lock instead of take-and-drop

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.

Drop last bits of rescan as its too complicated to be worth having

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.

Drop stale comment about a rescan that doesn't happen in tests

Merge pull request #729 from casey/readme

Add content to readme

Fix capitalization Gleb suggested

Clarify that lightning-net-tokio does networking only

Reword first paragraph and add tagline

Add content to readme

- Change header to "Rust-Lightning"
- Add crate and documentation badges
- Add introductory paragraph
- Move previous intro content under "Status" heading

Merge pull request #649 from jkczyz/2020-06-refactor-chain-listener

Refactor chain monitoring

Merge branch '2020-06-refactor-chain-listener-move-chainmonitor' into 2020-06-refactor-chain-listener

Move ln/channelmonitor.rs to chain/chainmonitor.rs

Move channelmonitor.rs from ln to chain module

Given the chain::Watch interface is defined in terms of ChannelMonitor
and ChannelMonitorUpdateErr, move channelmonitor.rs from the ln module
to the chain module.

Merge ChainMonitor impl blocks

Define type alias for enumerated transaction data

Transaction data from a block may be filtered before it is passed to
block_connected functions, which may need the index of each transaction
within the block. Rather than define each function in terms of a slice
of tuples, define a type alias for the slice where it can be documented.

Replace WatchEvent usage with get_outputs_to_watch

Outputs to watch are tracked by ChannelMonitor as of
73dce207dd0ea6c3ac57af3ebb8b87ee03e82c9e. Instead of determining new
outputs to watch independently using ChainWatchedUtil, do so by
comparing against outputs already tracked. Thus, ChainWatchedUtil and
WatchEvent are no longer needed.

Include funding TXO in outputs to watch

The funding TXO was never added to ChannelMonitor's outputs_to_watch in
73dce207dd0ea6c3ac57af3ebb8b87ee03e82c9e. Include it when constructing a
ChannelMonitor.

Fix architecture diagram arrow directions

Arrows should signify "calls" or "generates" unless noted.

Fix architecture diagram line length

Replace WatchEventProvider with chain::Filter

WatchEventProvider served as a means for replacing ChainWatchInterface.
However, it requires users to explicitly fetch WatchEvents, even if not
interested in them. Replace WatchEventProvider by chain::Filter, which
is an optional member of ChainMonitor. If set, interesting transactions
and output spends are registered such that blocks containing them can be
retrieved from a chain source in an efficient manner.

This is useful when the chain source is not a full node. For Electrum,
it allows for pre-filtered blocks. For BIP157/158, it serves as a means
to match against compact filters.

Remove ChainListener

BlockNotifier was removed in the previous commit, thus ChainListener is
no longer needed. Instead, anything needing chain events should be
notified directly.

Remove BlockNotifier

BlockNotifier is a convenience for handing blocks to listeners. However,
it requires that each listener conforms to the ChainListener interface.
Additionally, there are only two listeners, ChannelManager and
ChainMonitor, the latter of which may not be used when monitoring
channels remotely. Remove BlockNotifier since it doesn't provide much
value and constrains each listener to a specific interface.

Remove Key parameter from ChainMonitor

ChainMonitor's template Key parameter was meant to allow supporting
both local monitoring, where Key=OutPoint, and watchtowers, where Key=
(PublicKey, u32). Use OutPoint directly since the watchtower case will
not be supported this way.

Rename SimpleManyChannelMonitor to ChainMonitor

ManyChannelMonitor was renamed chain::Watch in the previous commit. Use
a more concise name for an implementation that monitors the chain for
channel activity. Future work will parameterize the struct to allow for
different varieties of persistence. Thus, users usually will be able to
use ChainMonitor directly rather than implementing a chain::Watch that
wraps it.

Replace ManyChannelMonitor with chain::Watch

Rename ManyChannelMonitor to chain::Watch and move to chain/mod.rs,
where chain-related interfaces live. Update the documentation for
clarity and to conform to rustdoc formatting.

Remove ChainWatchInterface

The remaining use of ChainWatchInterface was replaced by chain::Access
in the previous commit, and thus it is no longer needed.

Replace ChainWatchInterface in NetGraphMsgHandler

ChainWatchInterface was intended as an interface for watching rather
than accessing the chain. Remove get_chain_utxo and add chain::Access
trait for this behavior. Wrap it with an Option in NetGraphMsgHandler in
order to simplify the error interface.

Remove ChainWatchInterface from channelmonitor.rs

Use of ChainWatchInterface was replaced with WatchEvent in the previous
commit. Remove it from the parameterization of SimpleManyChannelMonitor
since it is no longer needed.

Replace use of ChainWatchInterface with WatchEvent

SimpleManyChannelMonitor is parameterized by ChainWatchInterface to
signal what transactions and outputs to watch for on chain. The
interface has grown to cover chain access (via get_chain_utxo) and block
block filtering (via filter_block and reentered), which has added
complexity for implementations and user (see ChainWatchInterfaceUtil).

Pull the watch functionality out as a first step to eliminating
ChainWatchInterface entirely.

Add test utilities for {dis}connecting a block

Replace direct uses of BlockNotifier in functional tests with utility
functions. This is in preparation for signaling watch events back via a
refactoring of ManyChannelMonitor and ChainWatchInterface. Those events
will be processed by connect_block.

Replace BlockNotifier with Node in test utilities

Change confirm_transaction and connect_blocks to take a Node instead of
a BlockNotifier. This is in preparation for signaling watch events back
via a refactoring of ManyChannelMonitor and ChainWatchInterface.

Simplify BlockNotifier tests

Use a simple ChainListner implementation rather than large test objects
for testing BlockNotifier.

Remove unregister_single_listener_ref_test since it is redundant with
unregister_single_listener_test.

Remove unnecessary clone() calls.

Align ChannelMonitor interface with ChainListener

ChannelMonitor has block_connected and block_disconnected methods called
by <SimpleManyChannelMonitor as ChainListener>. Use similar parameters
in ChannelMonitor such that transformations are not needed and the
interface is more closely aligned with ChainListener.