We generally make no effort to ensure all writes are buffered in
lower-level objects, so wrapping write calls in `BufWriter` may
substantially improve performance in some cases. This is especially
important now that we block the sample node exit until the
`NetworkGraph` has been written out, which includes many small-ish
writes.
With this change, shutdown of the sample node on a relatively
underpowered device went from 15-30 seconds of CPU time to a second
or two, plus IO sync time.
Instead of creating a separate trait for persisting NetworkGraph, use and rename the existing ChannelManagerPersister to handle them both. persist_graph is then called on removal of stale channels and on exit.
If we are in the middle of persisting an update to a
`ChannelMonitor` when we shutdown (or crash), we'll start up with
a .tmp file lying around. We should ignore it, as failure to
return from the update call should have prevented the
`ChannelManager` from taking any irrevocable action based on the
update.
We're somewhat protected from any filesystem inconsistency behavior
as the `ChannelManager` will refuse to load if we're outright
missing `ChannelMonitor`s.
Fixes#1330.
`cargo bench` sets `cfg(test)`, causing us to hit some test-only
code in the router when benchmarking, throwing off our benchmarks
substantially. Here we swap from the `unstable` feature to a more
clearly internal feature (`_bench_unstable`) and also checking for
it when enabling test-only code.
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.
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.
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.
std::io::ErrorKind is a `#[non_exhaustive]` enum as more specific
error types are to be added in the future. It was unclear in the
docs until very recently, however, that this is to be done by
re-defining `ErrorKind::Other` errors to new enum variants. Thus,
our tests which check explicitly for `ErrorKind::Other` as a
result of trying to access a directory as a file were incorrect.
Sadly, these generated no meaningful feedback from rustc at all,
except that they're suddenly failing in rustc beta!
After some back-and-forth, it seems rustc is moving forward
breaking existing code in future versions, so we move to the
"correct" check here, which is to check the raw IO error.
See rust-lang/rust#86442 and rust-lang/rust#85746 for more info.
Like the payment_secret parameter, this paramter has been the source
of much confusion, so we just drop it.
Users should prefer to do this check when registering the payment
secret instead of at claim-time.
There's little reason for the HashMap - the ChannelMonitors are
already unique (enforced by file names), and the eventual HashMap
that users need when deserializing the `ChannelManager` is a
slightly different form (it requires no BlockHash entry).
When we force-close a channel, for whatever reason, it is nice to
send an error message to our peer. This allows them to closes the
channel on their end instead of trying to send through it and
failing. Further, it may induce them to broadcast their commitment
transaction, possibly getting that confirmed and saving us on fees.
This commit adds a few more cases where we should have been sending
error messages but weren't. It also includes an almost-global
replace in tests of the second argument in
`check_closed_broadcast!()` from false to true (indicating an error
message is expected). There are only a few exceptions, notably
those where the closure is the result of our counterparty having
sent *us* an error message.
Many functional tests rely on being able to call block_connected
arbitrarily, jumping back in time to confirm a transaction at a
specific height. Instead, this takes us one step towards having a
well-formed blockchain in the functional tests.
We also take this opportunity to reduce the number of blocks
connected during tests, requiring a number of constant tweaks in
various functional tests.
Co-authored-by: Valentine Wallace <vwallace@protonmail.com>
Co-authored-by: Matt Corallo <git@bluematt.me>
Sadly the connected-in-order tests have to be skipped in our normal
test suite as many tests violate it. Luckily we can still enforce
it in the tests which run in other crates.
Co-authored-by: Matt Corallo <git@bluematt.me>
Co-authored-by: Jeffrey Czyz <jkczyz@gmail.com>
