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>
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.
This in addition removes the init fixed timeout hack.
Changelog-fixed: We now *always* die if our Bitcoin backend failed unexpectedly.
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Commit 9aedb0c61f changed this from allocating off `c` to allocating
off NULL, knowing that it's tal_steal() in the callback. But before
that, it can be detected as a mem leak:
```
@pytest.fixture
def teardown_checks(request):
"""A simple fixture to collect errors during teardown.
We need to collect the errors and raise them as the very last step in the
fixture tree, otherwise some fixtures may not be cleaned up
correctly. Require this fixture in all other fixtures that need to either
cleanup before reporting an error or want to add an error that is to be
reported.
"""
errors = TeardownErrors()
yield errors
if errors.has_errors():
# Format a nice list of everything that went wrong and raise an exception
request.node.has_errors = True
> raise ValueError(str(errors))
E ValueError:
E Node errors:
E Global errors:
E - Node /tmp/ltests-iz9y1chb/test_hsmtool_secret_decryption_1/lightning-1/ has memory leaks: [
E {
E "backtrace": [
E "ccan/ccan/tal/tal.c:442 (tal_alloc_)",
E "lightningd/jsonrpc.c:848 (parse_request)",
E "lightningd/jsonrpc.c:941 (read_json)",
E "ccan/ccan/io/io.c:59 (next_plan)",
E "ccan/ccan/io/io.c:407 (do_plan)",
E avis/build/ElementsProject/lightning/lightningd/../plugins/pay
```
Reported-by: @niftynei
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The plugin can basically return whatever it thinks the preimage is, but we
weren't handling the case in which it doesn't actually match the hash. If it
doesn't match now we just return an error claiming we don't have any matching
invoice.
We use the new function `plugins_free` to define the correct deallocation
order on shutdown, since under normal operation the allocation tree is
organized to allow plugins to terminate and automatically free all dependent
resources. During shutdown the deallocation order is under-defined since
siblings may get freed in any order, but we implicitly rely on them staying
around.
One is called on every plugin return, and tells us whether to continue;
the other is only called if every plugin says ok.
This works for things like payload replacement, where we need to process
the results from each plugin, not just the final one!
We should probably turn everything into a chained callback next
release.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
They callback must take ownership of the payload (almost all do, but
now it's explicit).
And since the payload and cb_arg arguments to plugin_hook_call_() are
always identical, make them a single parameter.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We have several of these, and they're not always called obvious things like
"delete" or "free". `STEALS` provides a strong hint here.
I only added it to a couple I knew about off the top of my head.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This makes testing easier, and makes sense: lightningd might not
*know* about other connected channels, depending on gossip, but if the
user specifies it we should obey it.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: JSON: `invoice` `exposeprivatechannels` now includes explicitly named channels even if they seem like dead-ends.
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>
This will be used when we want to specify these in a route. But for now, they
only alter gossipd, which always sets them to NULL.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Note that it's channeld which calculates the shared secret, too. This
minimizes the work that lightningd has to do, at cost of passing this
through.
We also don't yet save the blinding field(s) to the database.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This requires us to call ecdh() in the corner case where the blinding seed
is in the TLV itself (which is the case for the start of a blinded route).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We now track all pending RPC passthrough calls, and terminate them with an
error if the plugin dies.
Changelog-Fixed: JSON-RPC: Pending RPC method calls are now terminated if the handling plugin exits prematurely.
Use `LC_ALL=C sort` instead of `sort` so that mocks get sorted in
the same way on all developers' environments.
Re-record the result of `make update-mocks`.
Changelog-None
This happened on my testnet node because I've been failing to reconnect to
a node which created a channel and never exchanged announcement sigs.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
common/onion is going to need to use this for the case where it finds a blinding
seed inside the TLV. But how it does ecdh is daemon-specific.
We already had this problem for devtools/gossipwith, which supplied a
special hsm_do_ecdh(). This just makes it more general.
So we create a generic ecdh() interface, with a specific implementation
which subdaemons and lightningd can use.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We currently abuse the added_htlc and failed_htlc messages to tell channeld
about existing htlcs when it restarts. It's clearer to have an explicit
'existing_htlc' type which contains all the information for this case.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's almost always "their_features" and "our_features" respectively, so
make those names clear.
Suggested-by: @cdecker
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Note that now we check capacity once we've figured out which peer, which
broke a test (we returned "unknown peer" instead of "capacity exceeded"),
so we rework that too.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is useful in general, but in particular it allows fundchannel to avoid YA
query to figure out if it can wumbo.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON: `connect` returns `features` of the connected peer on success.
Shows what features we use in various contexts, including those added
by plugins in getmanifest.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: Plugin: `feature_set` object added to `init`
Turns out that unnecessary: all callers can access the feature_set,
so make it much more like a normal primitive.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This cleans up the boutique handling of features, and importantly, it
means that if a plugin says to offer a feature in init, we will now
*accept* that feature.
Changelog-Fixed: Plugins: setting an 'init' feature bit allows us to accept it from peers.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is to prepare for dynamic features, including making plugins first
class citizens at setting them.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Removed: JSON: `listnodes` `globalfeatures` output (`features` since in 0.7.3).
Changelog-Removed: JSON: `listpeers` `localfeatures` and `globalfeatures` output (`features` since in 0.7.3).
Changelog-Removed: JSON: `peer_connected` hook `localfeatures` and `globalfeatures` output (`features` since in 0.7.3).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Removed: JSON: `fundchannel` and `fundchannel_start` `satoshi` parameter removed (renamed to `amount` in 0.7.3).
This adapts our fee estimations requests to the Bitcoin backend to the
new semantic, and batch the requests.
This makes our request for fees much simpler, and leaves some more
flexibility for a plugin to do something smart (it could still lie before
but now it's explicit, at least.) as we don't explicitly request
estimation for a specific mode and a target.
Changelog-Changed: We now batch the requests for fee estimation to our Bitcoin backend.
Changelog-Changed: We now get more fine-grained fee estimation from our Bitcoin backend.
We kept track of an URGENT, a NORMAL, and a SLOW feerate. They were used
for opening (NORMAL), mutual (NORMAL), UNILATERAL (URGENT) transactions
as well as minimum and maximum estimations, and onchain resolution.
We now keep track of more fine-grained feerates:
- `opening` used for funding and also misc transactions
- `mutual_close` used for the mutual close transaction
- `unilateral_close` used for unilateral close (commitment transactions)
- `delayed_to_us` used for resolving our output from our unilateral close
- `htlc_resolution` used for resolving onchain HTLCs
- `penalty` used for resolving revoked transactions
We don't modify our requests to our Bitcoin backend, as the next commit
will batch them !
Changelog-deprecated: The "urgent", "slow", and "normal" field of the `feerates` command are now deprecated.
Changelog-added: The fields "opening", "mutual_close", "unilateral_close", "delayed_to_us", "htlc_resolution" and "penalty" have been added to the `feerates` command.
This allows us to set more fine-grained feerate for onchain resolution.
We still give it the same feerate for all types, but this will change as
we move feerates to bcli.
My node crashed as follows:
lightningd: lightningd/peer_control.c:957: peer_connected: Assertion `!peer->uncommitted_channel' failed.
In the logs I found:
Running lightning_openingd: Cannot allocate memory
Which reveals that we're not freeing uc in that path!
Changelog-Fixed: Fix assertion on reconnect if we fail to run openingd.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
For messages, we use the onion but payload lengths 0 and 1 aren't special.
Create a flag to disable that logic.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
also: convert the stored int value from 'int' to 's64'
atoi fails silently, returning a zero. instead we use the more robust
strtoll which will allow us fail with an error.
we also make the parsing for bools stricter, only allowing plausibly
boolean values to parse.
We were nesting like the following:
```json
{"params": {
"rpc_command": {
"rpc_command": {
}
}
}
```
This is really excessive, so we unwrap once, and now have the following:
```json
{"params": {
"rpc_command": {
}
}
```
Still more wrapping than necessary (the method is repeated in the `params`
object), but it's getting closer.
Changelog-Deprecated: JSON-RPC: Removed double wrapping of `rpc_command` payload in `rpc_command` JSON field.
Suggested-by: @fiatjaf
Signed-off-by: Christian Decker <@cdecker>
Before this patch we would only update `channel->last_tx` with the newly
proposed closure tx from the peer if the fee of the new one was lower.
In negotiations where we are at the higher end and the peer starts
lower, all peer's subsequent proposals will be higher than his initial
proposal and in this case we would never update `channel->last_tx`
and would wrongly broadcast his initial proposal at the end of the
negotiation.
Fixes https://github.com/ElementsProject/lightning/issues/3549
Changelog-Fixed: Always broadcast the latest close transaction at the end of the close fee negotiation, instead of sometimes broadcasting the peer's initial closing proposal.
Does the allocation and copying; this is useful because we can
avoid being fooled into doing giant allocations.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
ChangeLog-Added: New `getsharedsecret` command, which lets you compute a shared secret with this node knowing only a public point. This implements the BOLT standard of hashing the ECDH point, and is incompatible with ECIES.
Even without optimization, it's faster to walk all the channels than
ping another daemon and wait for the response.
Changelog-Changed: Forwarding messages is now much faster (less inter-daemon traffic)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Instead of saving a stripped_update, we use the new
local_fail_in_htlc_needs_update.
One minor change: we return the more correct
towire_temporary_channel_failure when the node is still syncing.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The idea is that gossipd can give us the cupdate we need for an error, and
we wire things up so that we ask for it (async) just before we send the
error to the subdaemon.
I tried many other things, but they were all too high-risk.
1. We need to ask gossipd every time, since it produces these lazily
(in particular, it doesn't actually generate an offline update unless
the channel is used).
2. We can't do async calls in random places, since we'll end up with
an HTLC in limbo. What if another path tries to fail it at the same time?
3. This allows us to use a temporary_node_failure error, and upgrade it
when gossipd replies. This doesn't change any existing assumptions.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is a common thing to do, so create a macro.
Unfortunately, it still needs the type arg, because the paramter may
be const, and the return cannot be, and C doesn't have a general
"(-const)" cast.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
common should not include specific per-daemon files. Turns out this
caused a lot of indirect includes to be exposed.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It was a pointer into the list of plugins for the hook, but it was rather
unstable: if a plugin exits after handling the event we could end up skipping
a later plugin. We now rely on the much more stable `call_chain` list, so we
can clean up that useless field.
We are attaching the destructor to notify us when the plugin exits, but we
also need to clear them once the request is handled correctly, so we don't
call the destructor when it exits later.
We make the current state of `lightningd` explicit so we don't have to
identify a shutdown by its side-effects. We then use this in order to prevent
the killing and freeing of plugins to continue down the chain of registered
plugins.
We were waiting for both stdin and stdout to close, however that resulted in
us deferring cleanup indefinitely since we did not poll stdout for being
writable most of the time. On the other hand we are almost always polling
the plugin's stdout, so that notifies us as soon as the plugin stops.
Changelog-Fixed: plugin: Plugins no longer linger indefinitely if their process terminates
Before this patch we used to send `double`s over the wire by just
copying them. This is not portable because the internal represenation
of a `double` is implementation specific.
Instead of this, multiply any floating-point numbers that come from
the outside (e.g. JSONs) by 1 million and round them to integers when
handling them.
* Introduce a new param_millionths() that expects a floating-point
number and returns it multipled by 1000000 as an integer.
* Replace param_double() and param_percent() with param_millionths()
* Previously the riskfactor would be allowed to be negative, which must
have been unintentional. This patch changes that to require a
non-negative number.
Changelog-None
This completes the conversion; any in-flight HTLC failures get turned into temporary_node_failures.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This cleans up the "local failure" callers for incoming HTLCs to hand
an onionreply instead of making us generate it from the code inside
make_failmsg.
(The db path still needs make_failmsg, so that's next).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-deprecated: Plugins: htlc_accepted_hook "failure_code" only handles simple cases now, use "failure_message".
Unfortunately the invoice_payment_hook can give us a failcode, so I simply
restrict it to the two sensible ones.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-deprecated: plugins: invoice_payment_hook "failure_code" only handles simple cases now, use "failure_message".
We tell channeld that an htlc is bad by sending it a 'struct
failed_htlc'. This usually contains an onionreply to forward, but for
the case where the onion itself was bad, it contains a failure code
instead.
This makes the "send a failed_htlc for a bad onion" a completely
separate code path, then we can work on removing failcodes from the
other path.
In several places 'failcode' is now changed to 'badonion' to reflect
that it can only be a BADONION failcode.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
At the moment, we store e.g. WIRE_TEMPORARY_CHANNEL_FAILURE, and then
lightningd has a large demux function which turns that into the correct
error message.
Such an enum demuxer is an anti-pattern.
Instead, store the message directly for output HTLCs; channeld now
sends us an error message rather than an error code.
For input HTLCs we will still need the failure code if the onion was
bad (since we need to prompt channeld to send a completely different
message than normal), though we can (and will!) eliminate its use in
non-BADONION failure cases.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We're going to change our internal structure next, so this is preparation.
We populate existing errors with temporary node failures, for simplicity.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Instead of making it ourselves, lightningd does it. Now we only have
two cases of failed htlcs: completely malformed (BADONION), and with
an already-wrapped onion reply to send.
This makes channeld's job much simpler.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I hadn't realized that lightningd asks gossipd every time we forward
a payment. But I'm going to abuse it here to get the latest channel_update,
otherwise (as lightningd takes over error message generation) lightningd
needs to do an async request at various painful points.
So have gossipd tell us the lastest update (stripped so compatible with
the strange in-onion-error format).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Turn it into temporary node failure: this only happens if we restart
with a failed htlc in, but it's clearer and more robust to handle it
generically.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
For incoming htlcs, we need failure details in case we need to
re-xmit them. But for outgoing htlcs, lightningd is telling us it
already knows they've failed, so we just need to flag them failed
and don't need the details.
Internally, we set the ->fail to a dummy non-NULL value; this is
cleaned up next.
This matters for the next patch, which moves onion handling into
lightningd.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
1. forward_htlc sets hout to NULL.
2. forward_htlc passes &hout to send_htlc_out.
3. forward_htlc checks the failcode and frees(NULL) and sets hout to NULL
(again). This in fact covers every failcode which send_htlc_out returns.
We should ensure send_htlc_out sets *houtp to NULL on failure; in fact,
both callers pass houtp, so we can make it unconditional.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We don't compile with NDEBUG defined, but if we did, this code would
vanish. I did a quick audit, inspired by @ZmnSCPxj.
I actually hacked up something to compile with NDEBUG (many unused vars
resulted, and of course unit tests are allowed to rely on assert()), and
after this the testsuite still passes.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
