Previously, we'd always add up to the maximum number of hop hints
(and beyond!) when selecting hop hints in our first pass. This
change updates hop hint selection to always stick to our hop hint
limit, and to the "hop hint factor" that we scale our invoices by.
This change will result in selecting fewer channels in our first
pass if their total inbound capacity reaches our hop hint factor.
This prevents us from revealing as many private channels as
before, but has the downside of providing fewer options for
payers.
Update our test to assert that we have hop hints present when we
expect them, and fix the "alice is a private node" issue that was
previously preventing us from adding hop hints. Asserting that we
have hop hints present in this itest ensures that we'll fail our
itests if a change to SelectHopHints results in our no longer
having hints in this secnario.
Update SelectHopHints to return a set of hop hints that can be
converted to route hints / functional options by the caller. This
change allows external code to call SelectHopHints to get a set of
route hints and use them as it likes, rather than limiting the
return value to functional options for invoice creation.
Pass htlc amount down to the channel so that we don't need to rely
on minHtlc (and pad it when the channel sets a 0 min htlc). Update
test to just check some sane values since we're no longer relying
on minHtlc amount at all.
The monitoring server still needs to be enabled using prometheus.enable,
so including this in the default build does not add an additional http
server unless the user opts in.
This commit adds height-based invoice expiry for hodl invoices
that have active htlcs. This allows us to cancel our intentionally
held htlcs before channels are force closed. We only add this for
hodl invoices because we expect regular invoices to automatically
be resolved.
We still keep hodl invoices in the time-based expiry queue,
because we want to expire open invoices that reach their timeout
before any htlcs are added. Since htlcs are added after the
invoice is created, we add new htlcs as they arrive in the
invoice registry. In this commit, we allow adding of duplicate
entries for an invoice to be added to the expiry queue as each
htlc arrives to keep implementation simple. Our cancellation
logic can already handle the case where an entry is already
canceled, so this is ok.
If we have processed a terminal state while we're pathfinding
for another shard, the payment loop should not error out on
ErrPaymentTerminal. Instead, it would wait for our shards to
complete then cleanly exit.
Move our more generic terminal check forward so that we only
need to handle a single class of expected errors. This change
is mirrored in our mock, and our reproducing tests are updated
to assert that this move catches both classes of errors we get.
Add an additional stuck-payment case, where our payment gets
a terminal error while it has other htlcs in-flight, and a
shard fails with ErrTerminalPayment. This payment also falls in
our class of expected errors, but is not currently handled. The
mock is updated accordingly, using the same ordering as in our
real RegisterAttempt implementation.
This commit adds a test which demonstrates that payments can
get stuck if we receive a payment failure while we're pathfinding
for another shard, then try to dispatch a shard after we've
recorded a permanent failure. It also updates our mock to
only consider payments with no in-flight htlcs as in-flight,
to more closely represent our actual RegisterAttempt.
This commit adds a step to our payment lifecycle test to add
control over when we find a path for our payment, This is
required for testing race conditions around pathfinding
completing and payment failures being reported.
Update our single shard success case to use a route which
splits the payment amount in half. This change still tests
the case where reveal of the preimage counts as a success,
even if we don't have the full amount. This change is made
to cut down on potential races in this test case. While we
are waiting for collectResultAsync to report a success, the
payment lifecycle will continue trying to dispatch shards.
In the case where we send 1/4 of the payment amount, we
send 1 or 2 more shards, depending on how long collectAsync
takes. Reducing this test to send 1/2 of the payment amount
means that we will always only try one more shard before
waiting for our shard.
This commit updates our mock to more closely follow the behavior of the
switch for mocked calls to GetPaymentResult. As it stands, our tests
send a test-created error from the switch when we want to mock shutdown.
In reality, the switch will close its result channel, so we update this
test to follow that behavior. This matters for the commit that follows,
because we start checking the error our payments return. If we have an
error from the switch, our tests will fail with an error that we do
not encounter in practice.
In our payment lifecycle tests, we have two goroutines that
compete for the lock in our mock control tower: the resumePayment
loop which tries to call RegisterAttempt, and the collectResult
handler which is launched in a goroutine by collectResultAsync
and is responsible for various settle/fail calls.
The order that the lock is acquired by these goroutines is
arbitrary, and can lead to flakes in our tests if the step
that we do not intend to execute first gets the lock (eg,
we want to fail a payment in collectResult, but RegisterAttempt
gets there first). This commit moves contention for this lock
after our mock's various "state driving" channels, so that the
lock will be acquired in the order that the test intends it.
Now that we run each test individually, we don't need to buffer
our mock's channels anymore. This helps to tighten our test loop,
which currently can move on from a step before it's actually
been processed by the mock. This removal ensures that our payment
loop processes each of the test's steps before moving on to the
next once.
Update our payment lifecycle test to run each test case with
a fresh router. This prevents test cases from interacting with
each other. Names are also added for easy debugging.
As is, we don't check that our SendPayment call in
TestRouterPaymentStateMachine completes. This makes it easier
to create malformed tests that just run through steps but leave
the SendPayment call hanging. This commit adds a check that we
have completed our payment to help catch tests like this. We
also remove an unused quit channel.
Our aggregate htlc test depends on our previous behavior
where recipients would allow channels with pending hold
invoice htlcs to force close. Now that we have an expiry
watcher to prevent these force closes, we can't rely on
this for tests because the recipient will cancel the htlcs
back before they expire.
This commit adds a test for a hold invoice which is accepted
off-chain, and held by the recipient until it expired and
the payer force-closes the channel. With this test we
demonstrate two bugs in our handling of hold invoice state
in the invoice registry when we expire on chain:
- Htlcs not updated: even when we've timed out, we don't
update the htlc state accordingly.
- Invoice can be settled: the invoice can be settled even
though it's expired on chain.
Reproduce the case where we allow settling of invoices that have
htlcs that have actually timed out on chain. This bug can rarely
occur if a hodl invoice goes to chain and is manually settled
after it has timed out. Funds are SAFU, but this could be a
headache because the invoice says it's settled when no funds
were claimed.
This commit updates our multi-hop force close test to use a hodl
invoice so that we can reproduce some bugs which will require
the preimage for the invoice that is timed out on chain.
This PR updates the hold invoice itest to create a private
channel, and sets the private option on the invoices created
to add coverage for the addition of hop hints.
All of the other mission control exported functions acquire their locks
immediately, and do not lock in the subsequent unexported functions.
This commit moves the lock up for the report payment functions so that
mission control's config values are covered by this lock, in preparation
for allowing config to be updated at runtime. Moving this lock means
that we will hold the lock for the additional time it takes to store a
single result, AddResult, to the store.
We are going to use the config struct to allow getting and setting
of the mission control config in the commits that follow. Self node
is not something we want to change, so we move it out for better
separation.
Previously, the verbose output of listsweeps would fail if we did not
find some sweeps in our wallet's listtransactions output. This could be
the case for sweeps that were rbf-ed, so the endpoint would fail. This
commit also updates the listsweeps endpoint to always check against the
wallet, so that we do not return these discarded sweeps that never
confirmed.
To allow us to grab all of the information we need for our channel arbs
in a more efficient way on startup, we add an optional tx to our lookup
functions required on start.
The getblockchaininfo call in bitcoind uses a commonly used lock,
csmain, in bitcoind. This made the endpoint unsuitable for a health
check, because some nodes were seeing waits up to 5 minutes (!). This
commit updates our health check function to use the uptime api, provided
our bitcoind version is > 0.15, when the api was added. We do not need
to switch our health check for btcd, because it has more granular
locking.
Add more fields to channel acceptor response so that users can have more
fine grained control over their incoming channels. With our chained
acceptor, it is possible that we get inconsistent responses from
multiple chained acceptors. We create a conjugate repsponse from all the
set fields in our various responses, but fail if we get different, non-
zero responses from our various acceptors. Separate merge functions are
used per type so that we avoid unexpected outcomes comparing interfaces
(panic on comparing types that aren't comparable), with casting used
where applicable to avoid code duplication.
This commit adds an optional error message to the channel acceptor's
reponse to allow operators to inform (or insult) unsuccessful channel
initiators as to the reason for their rejection.
This field is added in addition to the existing accept field to maintain
backwards compatibity. If we were to deprecate accept and interpret a
non-nil error as rejecting the channel, then received a response with
accept=false and a nil error, the server cannot tell whether this is a
legacy rejection or new mesage type acceptance (due to nil error),
so we keep both fields.
This commit moves and partially refactors the channel acceptor logic
added in c2a6c86e into the channel acceptor package. This allows us to
use the same logic in our unit tests as the rpcserver, rather than
needing to replicate it in unit tests.
Two changes are made to the existing implementation:
- Rather than having the Accept function run a closure, the closure
originally used in the rpcserver is moved directly into Accept
- The done channel used to signal client exit is moved into the acceptor
because the rpc server does not need knowledge of this detail (in
addition to other fields required for mocking the actual rpc).
Crediting orginal committer as co-author:
Co-authored-by: Crypt-iQ
To allow nodes more control over the amount of time that their funds
will be locked up, we add a MaxLocalCSVDelay option which sets the
maximum csv delay we will accept for all channels. We default to the
existing constant of 10000, and set a sane minimum on this value so that
clients cannot set unreasonably low maximum csv delays which will result
in their node rejecting all channels.
The disk availability health check is less critical than our chain
access check, and may break existing setups (particularly mobile) if we
enable it by default. Here we disable by default, but leave our other
default values in so that it can easily be flipped on.
Since we store all-time flap count for a peer, we add a cooldown factor
which will discount poor flap counts in the past. This is only applied
to peers that have not flapped for at least a cooldown period, so that
we do not downgrade our rate limiting for badly behaved peers.
Since we will use peer flap rate to determine how we rate limit, we
store this value on disk per peer per channel. This allows us to
restart with memory of our peers past behaviour, so we don't give badly
behaving peers have a fresh start on restart. Last flap timestamp is
stored with our flap count so that we can degrade this all time flap
count over time for peers that have not recently flapped.
To prevent flapping peers from endlessly dos-ing us with online and
offline events, we rate limit the number of events we will store per
period using their flap rate to determine how often we will add their
events to our in memory list of online events.
Since we are tracking online events, we need to track the aggregate
change over the rate limited period, otherwise we will lose track of
a peer's current state. For example, if we store an online event, then
do not store the subsequent offline event, we will believe that the
peer is online when they actually aren't. To address this, we "stage"
a single event which keeps track of all the events that occurred while
we were rate limiting the peer. At the end of the rate limting period,
we will store the last state for that peer, thereby ensureing that
we maintain our record of their most recent state.
When dealing with online events, we actually need to track our events
by peer, not by channel. All we need to track channels is to have a
set of online events for a peer which at least contain those events.
This change refactors chanfitness to track by peer.
We currently query the store for uptime and lifespan individually. As
we add more fields, we will need to add more queries with this design.
This change combines requests into a single channel infor request so
that we do not need to add unnecessary boilerplate going forward.
To get our uptime, we first filter our event log to get online periods.
This change updates this code to be tolerant of consecutive online or
offline events in the log. This will be required for rate limiting,
because we will not record every event for anti-dos reasons, so we could
record an online event, ignore an offline event and then record another
offline event. We could just ignore this duplicate event, but we will
also need this tolerance for when we persist uptime and our peers
can have their last event before restart as an online event and record
another online event when we come back up.
carla