The way PeerHandler was written, it was supposed to remove from
self.peers iff the API docs indicate that disconnect_event should
NOT be called (and otherwise rely on disconnect_event to do so).
Sadly, the implementation was way out of whack with reality - in
the implementation, essentially anywhere where PeerHandler
originated the disconnection, the peer was removed and no
disconnect_event was expected. The docs, however, indicated that
disconnect_event should nearly only be called, only not doing so
when the initial handshake message never completed.
We opt to change the docs, mostly, as well as clean up the
ping/pong handling somewhat and rename a few functions to clarify
what they actually do.
This is important for a number of reasons:
* Firstly, I hit this trying to implement rescan in the demo
bitcoinrpc client - if individual ChannelMonitors are out of
sync with each other, we cannot add them all into a
ManyChannelMonitor together and then rescan, but need to rescan
them individually without having to do a bunch of manual work.
Of the three return values in ChannelMonitor::block_connected,
only the HTLCsource stuff that is moved here makes no sense to
be exposed to the user.
* Secondly, the logic currently in ManyChannelMonitor cannot be
reproduced by the user! HTLCSource is deliberately an opaque
type but we use its data to decide which things to keep when
inserting into the HashMap. This would prevent a user from
properly implementing a replacement ManyChannelMonitor, which is
unacceptable.
* Finally, by moving the tracking into ChannelMonitor, we can
serialize them out, which prevents us from forgetting them when
loading from disk, though there are still other races which need
to be handled to make this fully safe (see TODOs in
ChannelManager).
This is safe as no two entries can have the same HTLCSource across
different channels (or, if they did, it would be a rather serious
bug), though note that, IIRC, when this code was added, the
HTLCSource field in the values was not present.
We also take this opportunity to rename the fetch function to match
our other event interfaces, makaing it clear that by calling the
function the set of HTLCUpdates will also be cleared.
Additional changes:
* Update fuzz crate to match ChannelManager's new API
* Update lightning-net-tokio library to match ChannelManager's new ChannelMonitor Deref API
* Update tests to match ChannelManager's new ChannelMonitor Deref API
This exposes the latest Init-context features in the ChannelDetails
passed to the Router during route calculation, which combines those
with the Node-context features tracked from node_announcements to
provide the latest Node-context features in RouteHop structs.
Fields are also added for Channel-context features, though those are
only partially used since no such features are defined today anyway.
These will be useful when determining whether to use new
TLV-formatted onion hop datas when generating onions for peers.
full_stack_target found a crash where we may overflow ruring fee
calculation if a transaction appears on-chain with massive value
available for us to claim. Since these transactions are clearly
bogus, we shouldn't allow full_stack_target to connect them, but
we also improve the error generated by explicitly panicing on them.
Previously, in each of our fuzz tests we had a dummy test which
had a hard-coded hex string which it passed into the fuzz target
so that when a failing test case was found, its hex could be
copied into the test and you could run cargo test to analyze the
failure. However, this was somewhat unwieldy as converting large
tests back and forth between hex and raw files is quite annoying.
Instead, we replace each of those tests with a test in each target
that looks for files in fuzz/test_cases and runs each file it finds.
Since we're editing every bin target anyway, we also automate adding
no_main to libfuzzer builds with #![cfg_attr].
The Features::new() method is nonsense and doesn't describe what
features were being set - we introduce an empty() and supported()
constructors instead.
This merges local and global features into one struct, which is
parameterized by where it appers. The parameterization restricts
which queries can be made and which features can be set, in line
with the latest BOLT 9.
Closes#427.
Simplify interfaces between ChannelMessageHandler and PeerManager,
by switching all ChannelMessageHandler errors to HandleError sent
internally instead of being return. With further refactors in Router
and PeerChannelEncryptor, errors management on the PeerManager-side
won't be splitted between try_potential_handleerror and HandleError
processing.
Inside ChannelManager, we now log MsgHandleErrInternal and send
ErrorAction to PeerManager.
On a high-level, it should allow client using API to be more flexible
by polling events instead of waiting function call returns.
We also update handle_error macro to take channel_state_lock from
caller which should avoid some deadlock potential for some edges
cases.
Filter out IgnoreError in handle_error macro, update test in
consequence.
Instead of having in-memory access to the list of private keys
associated with a channel, we should have a generic API which
allows us to request signing, allowing the user to store private
keys any way they like.
The first step is the (rather mechanical) process of templating
the entire tree of ChannelManager -> Channel impls by the
key-providing type. In a later commit we should expose only public
keys where possible.
This resolves a regression introduced in "Implement bumping engine in
ChannelMonitor::block_connected" in which not all inputs are checked.
Several opportunities to clarify and clean up comments are also taken.
Fix test_bump_penalty_txn_on_revoked_htlcs as now remote claim txn
build the same way than us are going to be register as cleaning
pending_claim_request after ANTI_REORG_DELAY. It means during this
delay we are going to generate invalid bumped claiming txn on
already claimed outpoints. Previously these txn weren't issued
because all their outpoints would have been removed.
Fix full_stack_target by adding more input for FuzzEstimator
A pending claim request may contain a set of multiple outpoints.
If one or multiple of them get claimed by remote party, our in-flight
claiming transactions aren't valid anymore so we need to react
quickly and regenerate claiming transaction with accurate set.
However, a claimed outpoint may be disconnected and we need to resurrect
back outpoint among set of orignal pending claim request.
To guarantee consistency of contentious claimed outpoint we cache it
as OnchainEvent::ContentionsOutpoint and only delete it after
ANTI_REORG_DELAY.
Fix test broken by change, partial claiming on revoked txn
force us to regenerate txn
Given they are only signed by us we can RBF at wish
Fix tests broken by introduction of more txn broadcast
(channel_monitor_network_test)
Add locktime in RemoteHTLC as it's needed to generate
timeout txn.
Add RBF-bumping of justice txn, given they are only signed by us we
can RBF at wish.
Aggregation of bump-candidates and more aggresive bumping heuristics
are left open
Fix tests broken by introduction of more txn broadcast.
Some tests may have a relaxed check (claim_htlc_ouputs_single_tx)
as broadcast bumped txn are now interwining in previous broadcast ones
and breaking simple expectations
Use bumping engine to rebuild claiming transaction in case of partial-
claim of its outpoints set.
This avoids a crash where a channel with a duplicate id is created
immediately after another is closed, where the other still has a
pending funding generation event. Resulting in funding generation
being passed to the wrong channel (which isn't in an appropriate
state).
This simplifies chanmon_fail_consistency greatly and makes it a
tiny bit more realistic. It also sets us up for more
disconnect-relatex events without the complexity blowing up.
