It's not all that rare to do these operations, and requiring annotations
for it is a little painful.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: New option `--important-plugin` loads a plugin is so important that if it dies, `lightningd` will exit rather than continue. You can still `--disable-plugin` it, however, which trumps `--important-plugin` and it will not be started at all.
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.
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.
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
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>
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.
Reduces VALGRIND=1 node_factory.line_graph(5) time on my laptop from 42s to 36s.
This is simply because forking all the subdaemons just to check the
version is very expensive under valgrind.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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>
`listconfigs` calls were setting the description twice and was using the
pointer to the boolean value as the boolean value, resulting in always
returning `true`.
This happens to be an edge case with the way we use `sendonion` in
MPP. `sendonion` does not attempt to recover the route even if we supply the
shared secrets (it'd require us to map forwarding channels to the nodes etc),
so `failnode` will always be unset, unless it is the first hop, which gets
stored. This is not a problem if it weren't for the fact that we don't store
the partial route, consisting solely of the channel leading to the first hop,
therefore the assertion that either both are NULL or both aren't fails on the
first hop.
This went unnoticed since with MPP we have more concurrent payments in flight,
increasing the chances of a exhausted first hop considerably.
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>
This is the only place outside the wallet code where we create
a 'struct utxo', so it makes sense for us to move that logic inside
the wallet.
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>
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-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're going to use the hsm for a migration, so we need to set up the HSM
before we get to the wallet migration code.
All that this requires is removing the places in HSM init that we touch
the database struct -- easy enough to accomplish by passing the required
field back out from init, and then associating it onto the wallet after
it's been initialized.
For the moment it's a complete tx, but in future designs we might only
be given the specific input which closes the channel.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It returns NULL, so you can simply `return fromwire_fail(...)`
if you want to return NULL in this case. Use that more.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We did not take the value of --commit-fee into account : this removes
the unused option from lightningd and instead registers it in bcli,
where we set the actual feerate of commitment transactions. This also
corrects the documentation.
Changelog-Fixed: config: we now take the --commit-fee parameter into account.
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Since we now over-write the wally malloc/free functions, we need to do
so for tests as well. Here we pull up all of the common setup/teardown
logic into a separate place, and update the tests that use libwally to
use the new common_setup core
Changelog-None
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.
These are incoming from onchaind, so the result of any transactions
we've created or outputs we own as a result of a channel closure. These
go into the 'wallet' account.
HTLCs trigger a coin movement only when their final form (state) is
reached. This prevents us from needing to concern ourselves with
retries, as well as being the absolutely most correct in terms of
answering the question 'when has the money irrevocably changed hands'.
All coin movements should pass this bar, for ultimate accounting
correctness
Adds a new plugin notification for getting information about coin
movements. Also includes two 'helper' notification methods that can be
called from within lightningd. Separated from the 'common' set because
the lightningd struct is required to finalize the blockheight etc
Changelog-Added: Plugins: new notification type 'coin_movement'
The current plan for coin movements involves tagging
origination/destination htlc's with a separate tag from 'routed' htlcs
(which pass through our node). In order to do this, we need a persistent flag on
incoming htlcs as to whether or not we are the final destination.
`lightningd` passes in all the known penalty_bases when starting a new
`channeld` instance, which tracks them internally, eventually matching them
with revocations and passing them back to `lightningd` so it can create the
penalty transaction. From here it is just a small step to having `channeld`
also generate the penalty transaction if desired.
When we have only a single member in a TLV (e.g. an optional u64),
wrapping it in a struct is awkward. This changes it to directly
access those fields.
This is not only more elegant (60 fewer lines), it would also be
more cache friendly. That's right: cache hot singles!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
There's no reason to assign the plugin vars inside the callback, so do
that outside, and the tal_steal() is redundant (the plugin is already
the conn parent).
And reduce duplication by making plugin_conn_finish call plugin_kill:
just make sure we don't call plugin_conn_finish again if plugin_kill
is called externally.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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.
1. Make the destructor call check_plugins_resolved(),
unless it was uninitialized (`opt_disable_plugin`).
2. Remove redundant list_del (destructor already does it).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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>
This is what I expected from plugin_kill, and now all the callers do the
equivalent anywat, it's easy.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Instead of calling plugin_kill() and returning, have them
uniformly return an error string or NULL, and have the top
level (plugin_read_json) do the plugin_kill() call.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This means we now clean up options in startup plugins (that was only
done by dynamic code!), and now they both share the 60 second timeout
instead of 20 seconds for dynamic.
For the dynamic case though, it's 60 seconds to both complete
getmanifest and init, which seems fair.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This will allow the dynamic starting code to use them too.
Also lets us move dev_debug_subprocess under #if DEVELOPER.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This will let us unify the startup and runtime-started infrastructure.
Note that there are two kinds of notifications:
1. Starting a single plugin (i.e. `plugin start`)
2. Starting multiple plugins (i.e. `plugin rescan` or `plugin startdir`).
In the latter case, we want the command to complete only once *all*
the plugins are dead/finished.
We also call plugin_kill() in all cases, and correctly return afterwards
(it matters once we use the same paths for dynamic plugins, which don't
cause a fatal error if they don't startup).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Now we know whether the command completed or not, we can correctly
call command_still_pending() if it didn't complete.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The symptom (under heavy load and valgrind) in test_plugin_command:
lightningd: common/json_stream.c:237: json_stream_output_: Assertion `!js->reader' failed.
This is because we try to call `getmanifest` again on `pay` which has not yet
responded to init.
The minimal fix for this is to keep proper state, so we can tell the
difference between "not yet called getmanifest" and "not yet finished
init".
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I noticed the following in logs for tests/test_connection.py::test_feerate_stress:
```
DEBUG 022d223620a359a47ff7f7ac447c85c46c923da53389221a0054c11c1e3ca31d59-chan#1: Failing HTLC 18446744073709551615 due to peer death
DEBUG 022d223620a359a47ff7f7ac447c85c46c923da53389221a0054c11c1e3ca31d59-chan#1: local_routing_failure: 8194 (WIRE_TEMPORARY_NODE_FAILURE)
```
This is because it reports the (transient) node_failure error, because
our channel_failure message is incomplete. Fix this wart up.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Vincenzo Palazzo
Rusty Russell
ZmnSCPxj jxPCSnmZ
Christian Decker
niftynei
Antoine Poinsot
rbndg