We split `send_invoice` offers inoo offerout (for want of a better name).
This simplifies the API.
Also took the opportunity to move the `vendor` tag to immediately
follow `description` (our tests use arguments by keywords, so no
change there).
Suggested-by: shesek
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
refund offers are implied send_invoice offers. And send_invoice offers
are implied single-use offers, though it can also make sense to have
a non-send_invoice offer be single-use.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
1. Hoist 7200 constant into the bolt12 heade2.
2. Make preimage the last createinvoice arg, so we could make it optional.
3. Check the validity of the preimage in createinvoice.
4. Always output used flag in listoffers.
5. Rename wallet offer iterators to offer_id iterators.
6. Fix paramter typos.
7. Rename `local_offer_id` parameter to `localofferid`.
8. Add reference constraints on local_offer_id db fields.
9. Remove cut/paste comment.
10. Clarify source of fatal() messages in wallet.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We were blindly initiating the keysend payment, which could lead to
confusing outcomes. This adds a very specific error message to the
error returned.
Changelog-Fixed: keysend: Keysend now checks whether the destination supports keysend before attempting a payment. If not a more informative error is returned.
This will allow nodes (with log-level=debug) to gather how many payments
are made without payment_secrets. We need to know this so we know when
we can make them compulsory.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In both cases the flakyness arises from the destination not knowing about the
modified fees of the forwarding node, thus including the outdated details in
the routehint, and the sender being unlucky and always trying with the
routehint anyway.
The long-term solutions to this is going to be #4111, this commit just reduces
the flakyness to get back to business.
Changelog-Added: JSON-RPC: delpay a new method to delete the payment completed or failed.
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Fixes: #3926
(probably)
Changelog-Fixed: pay: Also limit the number of splits if the payee seems to have a low number of channels that can enter it, given the max-concurrent-htlcs limit.
I screwed up the rotation logic in an earlier varient of this PR, and
it lead me to discover why test_mpp_interference_2 was flaky.
Really, we should keep a fuzzy estimator of how much payment is
outstanding, but in practice rotation is probably good enough.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
With a feerate of 7500perkw and subtracting 660 sats for anchors, a
20,000 sat channel has capacity about 9800 sat, below our default:
You gave bad parameters: channel capacity with funding 20000sat, reserves 546sat/546sat, max_htlc_value_in_flight_msat is 18446744073709551615msat, channel capacity is 9818sat, which is below 10000000msat
So bump channel amounts.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We're failing this too often: we'd fail it more but it's disabled
with VALGRIND (it shouldn't be: @slow_test removes VALGRIND if SLOW_MACHINE
is set).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is the simplest possible fix: increase the target amount until we get
the desired number of parts, while still bucketizing payments together that
are in approximately the same size.
The current logic puts all payments that are in the range x < amount <= 16*x
in the same bucket, making them harder to distinguish.
Changelog-Fixed: pay: The `presplit` modifier now supports large payments without exhausting the available HTLCs.
Anchor outputs break many assumptions in our tests:
1. Remove some hardcoded numbers in favor of a fee calc, so we only have to
change in one place.
FIXME: This should also be done for elements!
2. Do binary search to get feerate for a given closing fee.
3. Don't assume output #0: anchor outputs perturb them.
4. Don't assume we can make 1ksat channels (anchors cost 660 sats!).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reported-by: ZmnSCPxj
Signed-off-by: Christian Decker <@cdecker>
Changelog-Fixed: pay: Correct a case where we put the sub-payment value instead of the *total* value in the `total_msat` field of a multi-part payment.
The worst effect is that unpublished nodes are harder to pay, but
even published ones make us do unnecessary work, since we are
losing routehints from the published ones that could help us
actually route better to them.
listpays: make doc-all missed
Changelog-Added: JSON-RPC: `listpays` can be used to query payments using the `payment_hash`
Changelog-Added: JSON-RPC: `listpays` now includes the `payment_hash`
And when it's set, and we're SLOW_MACHINE, simply disable valgrind.
Since Travis (SLOW_MACHINE=1) only does VALGRIND=1 DEVELOPER=1 tests,
and VALGRIND=0 DEVELOPER=0 tests, it was missing tests which needed
DEVELOPER and !VALGRIND.
Instead, this demotes them to non-valgrind tests for SLOW_MACHINEs.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I started replacing all get_node() calls, but got bored, so then just did the
tests which call get_node() 3 times or more.
Ends up not making a measurable speed difference, but it does make some
things neater and more standard.
Times with SLOW_MACHINE=1 (given that's how Travis tests):
Time before (non-valgrind):
393 sec (had 3 failures?)
Time after (non-valgrind):
410 sec
Time before (valgrind):
890 seconds (had 2 failures)
Time after (valgrind):
892 sec
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: pay: Fixed a bug where routehints would be ignored if the payment exceeded 10,000 satoshi. This is particularly bad if the payee is only reachable via routehints in an invoice.
```
# 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>
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>
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.
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 test had part 1 and 2 backward, but still worked. When I copied that to
*after* the test had succeeded, it complained. It should always complain,
to catch bugs.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This wasn't important before, but now we have MPP it's good to enforce.
Reported-by: Christian Decker
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This exercises something that is simply not possible without MPP, i.e., the
bundling of multiple paths to get sufficient capacity to perform the payment.
Changelog-Added: The MPP presplit modifier splits large payments into 10k satoshi parts to maximize chances of performing the payment and to obfuscate the overall amount being sent.
As suggested during the paymod-03 review it is better to activate the new code
right away, and give users an escape hatch to use the legacy code instead. The
way I implemented it allows using either `legacypay` or `pay` and then set
`legacy` to switch to the other implementation.
Changelog-Added: JSON-RPC: The `pay` command now uses the new payment flow, the new `legacypay` command can be used to issue payment with the legacy code if required.
Suggested-by: Rusty Russell <@rustyrussell>
Suggested-by: ZmnSCPxj <@zmnscpxj>
This makes use of the payment modifier structure to just add the preimage to
the TLV payload for the last hop.
Changelog-Added: JSON-RPC: The `keysend` command allows sending to a node without requiring an invoice first.
This commit collects the changes required to the tests caused by the changes
to the `pay` and `paystatus` commands. They are also rather good hints as to
what these changes entail.
Make sure we've actually confirmed the HTLC; if it's not confirmed yet
then we won't fast-fail it, and we'll timeout instead:
```
> l1.rpc.waitsendpay(payment_hash=inv['payment_hash'], timeout=TIMEOUT, partid=1)
E AssertionError: Pattern 'WIRE_PERMANENT_CHANNEL_FAILURE \\(reply from remote\\)' not found in "RPC call failed: method: waitsendpay, payload: {'payment_hash': 'c186643391469aa8190415496c85b1eb789cb2b756a76d4c9ce21dd34c698d92', 'timeout': 30, 'partid': 1}, error: {'code': 200, 'message': 'Timed out while waiting'}"
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The status of what started as a simple JSON-RPC call is now spread across an
entire tree of partial payments and payment attempts. So we collect the status
in a single struct in order to report back success of failure.
This commit can be reverted/skipped once we have implemented all the logic and
have feature parity with the normal `pay`. It's main purpose is to expose the
unfinished functionality to test it, without completely breaking the existing
`pay` command.
We've been seeing some Travis timeouts under VALGRIND, with the
10 second timeout here: use TIMEOUT as per standard.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And the percentage of the initial amount, not the constently increasing
one !
Changelog-Fixed: pay: we now respect maxfeepercent, even for tiny amounts.
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Previously we've used the term 'funder' to refer to the peer
paying the fees for a transaction; v2 of openchannel will make
this no longer true. Instead we rename this to 'opener', or the
peer sending the 'open_channel' message, since this will be universally
true in a dual-funding world.
The documentation was wrong, and I copied my mistake to `libplugin` where it
was then ignored instead of ORed into the node's featurebits. This fixes both.
We weren't actually waiting until l3 got the channel_update from l2,
so it might not be able to create the routehint.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
When route returns a result which is too expensive, we try to figure out which
hop is most expensive to exclude it for next time.
If it's a single-hop route, we don't count it, since the first hop is free.
That's not usually a problem, since single-hop routes can't exceed our limits
(they're always "free"!).
But if we are using a routehint, the total cost could exceed our limits,
even if the start of the routehint is a single hop away.
This reproduces that test case.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is what actually lets us pay blinded invoices.
Unfortunately, our internal logic assumes every hop in a path has a
next `short_channel_id`, so we have to use a dummy. This is
sufficient for testing, however.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Sending update_fee immediately after channel establishment seems to
upset LND, so work around it by deferring it. The reason we increase
the fee after establishment is because now we might need to close the
channel in a hurry due to htlcs, but until there are htlcs that's
unnecessary.
Fixes: #3596
Changelog-Changed: Added workaround for lnd rejecting our commitment_signed when we send an update_fee after channel confirmed.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The spec states that invoices with an amount, but lacking a multiplier, should
be interpreted as integer Bitcoin amounts:
`amount`: optional number in that currency, followed by an optional
`multiplier` letter. The unit encoded here is the 'social' convention of a
payment unit -- in the case of Bitcoin the unit is 'bitcoin' NOT satoshis.
Suggested-by: Stefano Pellegrini <@St333p>
Signed-off-by: Christian Decker <@cdecker>
Changelog-Fixed: invoice: The invoice parser assumed that an amount without a multiplier was denominated in msatoshi instead of bitcoins.
A CONSERVATIVE/3 target for them.
Some noisy changes to the tests as we had to update the estimatesmartfee
mock.
Changelog-Changed: We now use a higher feerate for resolving onchain HTLCs and for penalty transactions
Rusty Russell
Christian Decker
Michael Schmoock
niftynei
Jan Sarenik
Vincenzo Palazzo
ZmnSCPxj jxPCSnmZ
Vincent
Antoine Poinsot