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>
While we were unsetting the `payment->cmd` in case of a success to signal that
we should not return to the JSON-RPC command twice, we were not doing that in
the case of failures. This was causing multiple responses to a single incoming
command, and `lightningd` was correctly killing the plugin. This issue was
introduced through early returns (anything setting `payment->abort=true`) and
was caused in Rusty's case through an MPP timeout.
Fixes#3847
Reported-by: Rusty Russell <@rustyrussell>
Signed-off-by: Christian Decker <@cdecker>
fundpsbt forces the caller to manually add their weight * feerate
to the satoshis they ask for. That means no named feerates.
Instead, create a startweight parameter and do the calc for them
internally, and return the feerate we used (and, while we're at it,
the estimated final weight).
This API change is best done now, as it would otherwise have to
be appended as a parameter.
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.
We're actually going to deprecate this, so don't add new features!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: ***REMOVE*** JSON-API: `txprepare` returns a psbt version of the created transaction
It uses reservations heavily, and assumed we generated change, etc.
It's now a simpler test, in many ways.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
reserveinputs marks UTXOs reserved for 12 hours, so we won't select them
for spending: unreserveinputs marks them available again.
Exposes param_psbt() for wider use.
Disabled the test_sign_and_send_psbt since we're altering the API;
the final patch restores it.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Technically an API break, but nobody relies on these I hope!
Note that the feerates warning was buried inside the style object:
it should be top-level.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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.
Since we change the interface for `pay` and `paystatus` we need a way to
determine which one to expect. This just parses the COMPAT_Vxyz flags and
allows us to check if, based on the configuration, we are in compat mode for
that set of changes or not.
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>
It looked like we weren't printing the address on closing outputs.
But we are, because the 'scriptPubkey' field is in the 'outputs' db
table since 0.7.3 (66a47d2761).
So make the logic clearer, and remove a completely bogus comment (UTXOs
with closing_info are definitely spendable!).
We export the json_add_utxos() for future use, too.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
These are pulled from wallet/wallet.c, with the fix now that we grind sigs.
This reduces the fees we pay slightly, as you can see in the coinmoves changes.
I now print out all the coin moves in suitable format before we match:
you only see this if the test fails, but it's really helpful.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
By setting nLocktime to the current block, the reorg test
"test_funding_reorg_remote_lags" actually drops the funding transaction
entirely when a reorg happens.
Except the 1 in 10 cases where nLocktime is randomly set to 1-10
blocks earlier.
This implies, strongly, that we hit "restart" too often on Travis.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
There is a race between `getroute` learning that our peer accepts TLVs and us
initiating the payment. Waiting for announcements ensures we always use TLVs,
matching our expectation in the test / plugin.
Changelog-Changed: `txprepare` now prepares transactions whose `nLockTime` is set to the tip blockheight, instead of using 0. `fundchannel` will use `nLockTime` set to the tip blockheight as well.
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.
If the daemon already knows about the channel before it was stopped,
it won't get this message from gossipd. That's OK, since we explicitly
test for the channel being active two lines down.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Our existing coin_moves tracking logic assumed that any tx we had an
input in belonged to *all* of our wallet (not a bad assumption as long
as there was no way to update a tx that spends our wallets)
Now that we've got `signpsbt` implemented, however, we need to be
careful about how we account for withdrawals. For now we do a best guess
at what the feerate is, and lump all of our spent outputs as a
'withdrawal' when it's impossible to disambiguate
Changelog-Added: JSON-RPC: new call `signpsbt` which will add the wallet's signatures to a provided psbt
Changelog-Added: JSON-RPC: new call `sendpsbt` which will finalize and send a signed PSBT
Reserve and unreserve wallet UTXOs using a PSBT which includes those
inputs.
Note that currently we unreserve inputs everytime the node restarts.
This will be addressed in a future commit.
Changelog-Added: JSON-RPC: Adds two new rpc methods, `reserveinputs` and `unreserveinputs`, which allow for reserving or unreserving wallet UTXOs
Changelog-Changed: `fundchannel_cancel` will now succeed even when executed while a `fundchannel_complete` is ongoing; in that case, it will be considered as cancelling the funding *after* the `fundchannel_complete` succeeds.
Let me introduce the concept of "Sequential Consistency":
All operations on parallel processes form a single total order agreed upon by all processes.
So for example, suppose we have parallel invocations of `fundchannel_complete` and `fundchannel_cancel`:
+--[fundchannel_complete]-->
|
--[fundchannel_start]-+
|
+--[fundchannel_cancel]---->
What "Sequential Consistency" means is that the above parallel operations can be serialized as a single total order as:
--[fundchannel_start]--[fundchannel_complete]--[fundchannel_cancel]-->
Or:
--[fundchannel_start]--[fundchannel_cancel]--[fundchannel_complete]-->
In the first case, `fundchannel_complete` succeeds, and the `fundchannel_cancel` invocation also succeeds, sending an `error` to the peer to make them forget the chanel.
In the second case, `fundchannel_cancel` succeeds, and the succeeding `fundchannel_complete` invocation fails, since the funding is already cancelled and there is nothing to complete.
Note that in both cases, `fundchannel_cancel` **always** succeeds.
Unfortunately, prior to this commit, `fundchannel_cancel` could fail with a `Try fundchannel_cancel again` error if the `fundchannel_complete` is ongoing when the `fundchannel_cancel` is initiated.
This violates Sequential Consistency, as there is no single total order that would have caused `fundchannel_cancel` to fail.
This commit is a minimal patch which just reschedules `fundchannel_cancel` to occur after any `fundchannel_complete` that is ongoing.
We passed below the floor when the user specified `1000perkb`.
Matt Whitlock says :
I was withdrawing with feerate=1000perkb, which should be the minimum-allowed fee rate. Indeed, bitcoin-cli getmempoolinfo reports:
{
"loaded": true,
"size": 15097,
"bytes": 9207924,
"usage": 32831760,
"maxmempool": 64000000,
"mempoolminfee": 0.00001000,
"minrelaytxfee": 0.00001000
}
Changelog-fixed: rpc: The `feerate` parameters now correctly handle the standardness minimum when passed as `perkb`.
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Reported-by: Matt Whitlock
We erase peer data after the last channel close transaction for that
peer is 100 blocks deep. We were failing to finish the migration because
the peer_id lookup on these was failing.
Now we ignore any channel with a null peer_id.
Fixes#3768
We use a database snapshot with 3 channels -- two of which have HTLCs
dangling and one is an initial open channel tx in the 'old' tx hex
format in last_tx and confirm that they are successfully updated to PSBT
format on start.
Spec is wrong (it says it should be compulsory), and Eclair doesn't set it
at all, leading to an error when they send their announcement_signatures.
Fixes: #3703
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-changed: large-channels: negotiate successfully with Eclair nodes.
There are various places where our tests failed with
--enable-expimental-features. And our plugin test overlapped an
existing feature.
We make our expected_feature functions more generic, and use them
everywhere.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Much nicer for grepping, since `{ "foo": { "bar": [7] } }` is turned into
`foo.bar[0]=7`.
Changelog-Added: cli: New `--flat` mode for easy grepping.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
-H removes the top-level if there's only one, and 'format-hint'
breaks this heuristic, so we end up with:
```
help=command=autocleaninvoice [cycle_seconds] [expired_by]
category=plugin
description=Set up autoclean of expired invoices.
verbose=Perform cleanup every {cycle_seconds} (default 3600), or disable autoclean if 0. Clean up expired invoices that have expired for {expired_by} seconds (default 86400).
command=check command_to_check
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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>
It's possible for our peer to publish a commitment tx that has already
updated our balance for an htlc before we've completed removing it from
our commitment tx (aka before we've updated our balance). This used to
crash, now we just update our balance (and the channel balance logs!)
and keep going.
If they've removed anything from our balance, we'll end up counting it
as chain_fees below. Not ideal but fine... probably.
This moves the notification for our coin spends from when it's
successfully submited to the mempool to when they're confirmed in a
block.
We also add an 'informational' notice tagged as `spend_track` which
can be used to track which transaction a wallet output was spent in.
Previously we were annotating every movement with the blockheight of
lightningd at notification time. Which is lossy in terms of info, and
won't be helpful for reorg reconciliation. Here we switch over to
logging chain moves iff they've been confirmed.
Next PR will fix this up for withdrawals, which are currently tagged
with a blockheight of zero, since we log on successful send.
On node start we replay onchaind's transactions from the database/from
our loaded htlc table. To keep things tidy, we shouldn't notify the
ledger about these, so we wrap pretty much everything in a flag that
tells us whether or not this is a replay.
There's a very small corner case where dust transactions will get missed
if the node crashes after the htlc has been added to the database but
before we've successfully notified onchaind about it.
Notably, most of the obtrusive updates to onchaind wrappings are due to
the fact that we record dust (ignored outputs) before we receive
confirmation of its confirmation.
Mostly we update existing tests to account for channel balances. In a
few places, new tests were needed as there wasn't an existing pathway
that tested the chain-fees for a few penalty cases
For cheats, we do a little bit of weird accounting. First we 'update'
our on-ledger balance to be the entirety of the channel's balance. Then,
as outputs get resolved, we record the fees and outputs as withdrawals
from this amount.
It's possible that they might successfully 'cheat', in which case we
record those as 'penalty' but debits (not credits).
The previous patch changed the gossip_store, but in a trivial way.
The next patch will implement upgrading, so this is the test.
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>
The previous implementation was a bit lazy: in particular, since we didn't
remember the disabled plugins, we would load them on rescan.
Changelog-Changed: config: the `plugin-disable` option works even if specified before the plugin is found.
That's more convenient for most callers, which don't need a fmt.
Fixed-by: Darosior <darosior@protonmail.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Vincent
ZmnSCPxj jxPCSnmZ
Rusty Russell
Christian Decker
niftynei
Antoine Poinsot