We were basing the 2016 block timeout on the blockchain height that we had
processed at the time completed the funding, which could lag considerably
behind the network-wide blockheight. For example this could happen if we
started with `--rescan` from a low height, taking some time to catch up.
This means that we could end up forgetting channels even before reaching the
network blockheight. This patch instead uses the headercount reported by the
backend plugin if we aren't caught up yet. While the chances of this happening
are still there, the window it might happen are much reduced, since headers
can be synced in a couple of minutes.
Reported-by: Riccardo Masutti
Changelog-Changed: Funding timeout is now based on the header count reported by the bitcoin backend instead of our current blockheight which might be lower.
Arguably a low-priority bug since no current node ever generates routehints longer
than one hop.
However, it is possible as an edge case, if the destination is directly accessible
*and* supports multiple channels, that we route through the destination, one of the
*other* channels it has not in the routehint, to the entry point, and then through
the routehint.
This change removes the risk of the above edge case.
Changelog-None: arguably a low-priority bug.
We've had problems with blocksonly in the past and bitcoind still allows
to use outdated (thus potentially dangerous estimates) while running
bitcoin with -blocksonly.
ZmnSCPxj mentionned that we still don't document this, but i figured
that in this specific case an explicit check and error seems preferable.
Changelog-Added: We now explicitly check at startup that our default Bitcoin backend (bitcoind) does relay transactions.
Proposed-by: ZmnSCPxj <zmnscpxj@protonmail.com>
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
This allows us to kill two birds with one stone: once connected, we use
the output of the successful call to do some sanity checks (only
checking the version for now, but more are yet to come!).
Changelog-Added: We now explicitly check at startup the version of our default Bitcoin backend (bitcoind).
Co-Authored-by: ZmnSCPxj <zmnscpxj@protonmail.com>
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
We'd previously take the failed attempt and estimate the failing channel's
capacity at 3/4 of the attempted amount, which is rather aggressive. This
reduces this aggressiveness to use the exact amount tried, but excluding on
equality. This still skips attempting the same route with the same amount, but
also permits attempts that are in the range [3/4, 1] of the failed attempt
amount to still be attempted.
Changelog-Changed: JSON-RPC: `delinvoice` will now report specific error codes: 905 for failing to find the invoice, 906 for the invoice status not matching the parameter.
the way we use PSBTs to sign things requires that we have the
scriptpubkey available on the utxo so we can populate the witness-utxo
field with it.
this causes problems if we don't already have the scriptpubkey cached in
the database, as in *some* cases we require a round trip to the HSM to
populate them
to get over this hump, we backfill any and all missing scriptpubkey
information for the utxo's that we hold in our wallet.
this will allow us to clean up the NULL handling of missing
scriptpubkeys.
we're about to add a migration that requires access to the bip32_key
in order to calculate missing scriptpubkeys.
prior to this patch, we don't have access to the bip32 key in the db
migration, as it's set on the wallet but after the db migrations are
run.
here we patch it through so that every migration can access it
```
# Excludes channel, then ignores routehint which includes that, then
# it excludes other channel.
> assert len(status) == 2
E assert 1 == 2
E -1
E +2
```
The invoice we use at the end has a routehint: 50% of the time it's
to l2 (which fails), 50% to l5 (which succeeds).
Change it to create invoice before channel with l5 so it does the
retry like we expect here.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We sum up the amounts just like we do with `amount_sent`, however we may have
parts that don't have that annotation, so make it optional.
Suggested-by: Rusty Russell <@rustyrussell>
While not directly necessary, it still feeds the `listpays` result, and so we
should pass it along if we can, so we don't have to rely solely on the
`amount_sent` field, which includes the fees.
Reported-by: Rusty Russell <@rustyrussell>
Since we started using `sendonion` in the `pay` plugin we no longer
automatically have the `amount` annotation on (partial) payments. This
replicates the issue so we can fix it.
Reported-by: Rusty Russell <@rustyrussell>
There were no channel updates in my log; because sendonion doesn't know the
actual node_ids or channel_ids, we can't tell gossipd what node/channel it was
so it can no longer remove them on PERM errors.
However, we can tell it the error message so it can apply the update.
Fixes: #3877
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And document the partid arg.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `sendonion` has a new optional `bolt11` argument for when it's used to pay an invoice.
The shortcut in the retry_mod that we can skip retrying if getroute fails or
we have no result is only valid if the parameters don't change. As we iterate
through the routehints the parameters change, and so we must signal to the
retry_mod that it can retry even in those cases.
The child payments will sometimes depend on the step of the parent, and making
sure that the parent state is correct before we create the children is
therefore important.
This uses @cdecker's idea of excluding the routehinted channel from the route,
and also consumes the route hints as it goes so that it makes progress.
I don't know if this is correct, but it reliably passes tests/test_pay.py::test_tlv_or_legacy
now.
We store an offset of the current routehint in the modifier data. It gets
incremented on retry, and it gets reset to 0 on split. This is because once we
split we have a different amount and a previously unusable routehint becomes
usable again.
We have two places we need to do that now: in the root payment after we
checked if the destination is reachable, and in any other payment directly in
the initialization-step callback.
It was spread over the step callback, but we only need to initialize the
routehints array there, child-payments can just inherit most of the information.
This does two things: it checks if the destination of the payment is at all
reachable without routehints, and if it is it adds a direct attempt as option
to the routehints in the form of a NULL routehint. It also simplifies the
selection of the routehint since the direct case is no longer special, instead
we just return a NULL routehint as if it were a normal routehint.
The adaptive MPP test was showing an issue with always using a routehint, even
when it wasn't necessary: we would insist on routhing to the entrypoint of the
routehint, even through the actual destination. If a channel on that loop
would result being over capacity we'd slam below 0, and then increase again by
unapplying the route. The solution really is not to insist on routing through
a routehint, so we implement random skipping of routehints, and we rotate them
if we have multiples.
As the hints get new fields added it is easy to forget to amend one of the
places we create them, since we already have an update method let's use that
to handle all additions to the array of known channel hints.
We were removing the current hint from the list and not inheriting the current
routehint, so we'd be forgetting a hint at each retry. Now we keep the array
unchanged in the root, and simply skip the ones that are not usable given the
current information we have about the channels (in the form of channel_hints).
Fixes#3861
This may be related to the issue #3862, however the water was muddied by it
being the wrong error to return, and the node should not expect this courtesy
feature to be present at all...
There is little point in trying to split if the resulting HTLCs exceed the
maximum number of HTLCs we can add to our channels. So abort if a split would
result in more HTLCs than our channels can support.
The presplit modifier could end up exceeding the maximum number of HTLCs we
can add to a channel right out the gate, so we switch to a dynamic presplit if
that is the case. The presplit will now at most use 1/3rd of the available
HTLCs on the channels if the normal split would exceed the number of availabe
HTLCs. And we also abort early if we don't have a sufficient HTLCs available.
It turns out that by aggressively splitting payments we may end up exhausting
the number of HTLCs we can add to a channel quickly. By tracking the number of
HTLCs we can still add, and excluding the channels to which we cannot add any
more we increase the route diversity, and avoid quickly exhausting the HTLC
budget.
In the next commit we'll also implement an early abort if we've exhausted all
channels, so we don't end up splitting indefinitely and we can also optimize
the initial split to not run afoul of that limit.
Changelog-Fixed: plugin: `bcli` no longer logs a harmless warning about being unable to connect to the JSON-RPC interface.
Changelog-Added: plugin: Plugins can opt out of having an RPC connection automatically initialized on startup.
This PR includes the fix discussed on PR #3855. This fix was tested with the use case described inside the issue and worked.
Fixes: #3855
Changelog-None
The code had incorrect assertions, partially because it didn't clearly
distinguish errors from the final node (which, barring blockheight issues,
mean a complete failre) and intermediate nodes.
In particular, we can't trust the *values*, so we need to distinguish
these by the *sender*.
If a route is of length 2 (A, B):
- erring_index == 0 means us complaining about channel A.
- erring_index == 1 means A.node complaining about channel B.
- erring_index == 2 means the final destination node B.node.
This is particularly of note because Travis does NOT run test_pay_routeboost!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Christian Decker
ZmnSCPxj jxPCSnmZ
Antoine Poinsot
niftynei
Rusty Russell
Vincent