We don't want to exit just because channel parameter negotiation
failed, but we do want to tell the master if it was a channel we were
trying to fund.
Note that lightningd still needs to fail the funding cmd if it gets a
fromwire_opening_fundee (they raced us and won), or an outright
failure.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Previously master would fail once the channel has been negotiated,
which is terrible, since the funder will have already broadcast tx.
Now we tell them if we have an active channel, and update if it goes away.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We now simply maintain a pubkey set for connected peers (we only care
if there's a reconnect), not the entire peer structure.
lightningd no longer queries us for getpeers: it knows more than we do
already.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Prior to this, lightningd would hand uninteresting peers back to connectd,
which would then return it to lightningd if it sent a non-gossip msg,
or if lightningd asked it to release the peer.
Now connectd hands the peer to lightningd once we've done the init
handshake, which hands it off to openingd.
This is a deep structural change, so we do the minimum here and cleanup
in the following patches.
Lightningd:
1. Remove peer_nongossip handling from connect_control and peer_control.
2. Remove list of outstanding fundchannel command; it was only needed to
find the race between us asking connectd to release the peer and it
reconnecting.
3. We can no longer tell if the remote end has started trying to fund a
channel (until it has succeeded): it's very transitory anyway so not
worth fixing.
4. We now always have a struct peer, and allocate an uncommitted_channel
for it, though it may never be used if neither end funds a channel.
5. We start funding on messages for openingd: we can get a funder_reply
or a fundee, or an error in response to our request to fund a channel.
so we handle all of them.
6. A new peer_start_openingd() is called after connectd hands us a peer.
7. json_fund_channel just looks through local peers; there are none
hidden in connectd any more.
8. We sometimes start a new openingd just to send an error message.
Openingd:
1. We always have information we need to accept them funding a channel (in
the init message).
2. We have to listen for three fds: peer, gossip and master, so we opencode
the poll.
3. We have an explicit message to start trying to fund a channel.
4. We can be told to send a message in our init message.
Testing:
1. We don't handle some things gracefully yet, so two tests are disabled.
2. 'hand_back_peer .*: now local again' from connectd is no longer a message,
openingd says 'Handed peer, entering loop' once its managing it.
3. peer['state'] used to be set to 'GOSSIPING' (otherwise this field doesn't
exist; 'state' is now per-channel. It doesn't exist at all now.
4. Some tests now need to turn on IO logging in openingd, not connectd.
5. There's a gap between connecting on one node and having connectd on
the peer hand over the connection to openingd. Our tests sometimes
checked getpeers() on the peer, and didn't see anything, so line_graph
needed updating.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
openingd calculates our reserve based on the channel amount (even if
we're funding, to keep the calculation in one place), but it wasn't
reporting it back to the master daemon. We initialized it to 0 so that
valgrind wouldn't get upset, as it's part of a structure we send over
the wire.
Have openingd report back, and also initialize it to an impossible value
as extra assurance. And remove a stray (harmless but weird) semicolon.
Reported-by: Gálli Zoltán
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's only used so we can timeout being fundee after a few hundred
blocks, but when openingd is started for idle connections, the
difference can be huge.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Checking in the master doesn't help anything, and it's weird.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
1diff --git a/connectd/connect.c b/connectd/connect.c
index 138b73fc..b01d1546 100644
Fortunately, we hit the assert in wallet_peer_delete() if this happens,
since there are still active channels.
This latent bug becomes far more likely in followup patches, where
openingd is used for idle peers.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This gives the network options a chance to load from arguments before usage
exits, so that the proper defaults are shown.
This didn't work:
lightningd --mainnet --help
before it showed testnet defaults, now it shows mainnet defaults.
Signed-off-by: William Casarin <jb55@jb55.com>
This adds one line with the onion and the channel_update we extract from
it. This in turn allows us to check that the channel_update in the onion is not
type prefixed, and that we patch it correctly before passing it to gossipd.
As was pointed out by @robtex we have underspecified the format of the nested
`channel_update` in the onionreply: lnd and eclair inserted the raw
channel_update without the type prefix, while we went for the full wire format,
including the type prefix. While we agreed that with the type it is more
flexible, and consistent, we decided to adapt to the majority and at least be
compatibly broken.
This commit takes care of being able to interpret either format correctly. It's
not perfect since signatures can happen to start with 0x0102 (the channel_update
type) but that'll happen only once ever 65k failures.
The easiest way to do this is to play with the 'wallet_tx' semantics
and have 'amount' have meaning even when 'all_funds' is set.
Note that we change the string 'Cannot afford funding transaction' to
'Cannot afford transaction' as this code is also used for withdrawls.
Inspired-by: molz on #c-lightning
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In several places we use low-level tal functions because we want the
label to be something other than the default. ccan/tal is adding
tal_*_label so replace them and shim it for now.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
tal_count() is used where there's a type, even if it's char or u8, and
tal_bytelen() is going to replace tal_len() for clarity: it's only needed
where a pointer is void.
We shim tal_bytelen() for now.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Valgrind error file: valgrind-errors.772802
==772802== Invalid read of size 1
==772802== at 0x4C32D04: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==772802== by 0x14479C: escape (json_escaped.c:41)
==772802== by 0x144B6C: json_escape (json_escaped.c:117)
==772802== by 0x118518: json_getnodes_reply (gossip_control.c:209)
==772802== by 0x139394: sd_msg_reply (subd.c:281)
==772802== by 0x139972: sd_msg_read (subd.c:418)
==772802== by 0x17ABB1: next_plan (io.c:59)
==772802== by 0x17B6A9: do_plan (io.c:387)
==772802== by 0x17B6E7: io_ready (io.c:397)
==772802== by 0x17D2C8: io_loop (poll.c:310)
==772802== by 0x121973: main (lightningd.c:450)
==772802== Address 0x6fe5168 is 0 bytes after a block of size 72 alloc'd
==772802== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==772802== by 0x18843E: allocate (tal.c:245)
==772802== by 0x18899D: tal_alloc_ (tal.c:421)
==772802== by 0x188B5E: tal_alloc_arr_ (tal.c:464)
==772802== by 0x119BAB: fromwire_gossip_getnodes_entry (gossip_msg.c:35)
==772802== by 0x15CCD6: fromwire_gossip_getnodes_reply (gen_gossip_wire.c:111)
==772802== by 0x118436: json_getnodes_reply (gossip_control.c:192)
==772802== by 0x139394: sd_msg_reply (subd.c:281)
==772802== by 0x139972: sd_msg_read (subd.c:418)
==772802== by 0x17ABB1: next_plan (io.c:59)
==772802== by 0x17B6A9: do_plan (io.c:387)
==772802== by 0x17B6E7: io_ready (io.c:397)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This seems like a premature optimization: it tried to cut down the number of
allocations by reusing the same `struct invoice_details` while iterating through
a number of results. But this sidesteps the checks by `valgrind` and we'd miss a
missing field that was set by the previous iteration.
Reported-by: @rustyrussell
Signed-off-by: Christian Decker <@cdecker>
Several users have noticed that they cannot pay satoshis.place or similar places
that have tiny payment amounts if they are not directly connected. This is due
to the forwarding fee dominating the transferred amount.
This commit adds a new option, exempting tiny fees (up to 5 satoshis by default)
from having to pass the maxfeepercent flag. While we could have told users to
tweak maxfeepercent I think it is usefull to have a default exemption.
[Squashed --RR]
Developer errors result in command_fail being called
just like other errors. The bad_programmer() Test is now updated
and passing.
Signed-off-by: Mark Beckwith <wythe@intrig.com>
They now just call command_fail() and cause param() to return false.
Temporarily disabled all the run-param.c tests that redirect
asserts so CI would still pass.
Signed-off-by: Mark Beckwith <wythe@intrig.com>
We use these for receiving arrays at init time, we should also use them
for fulfull/fail of HTLCs in normal operation. That we we benefit from all
those assertions.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The master tells us the short_channel_id of the outgoing channel when
failing an HTLC, but channeld didn't store it anywhere. It also
didn't tell channeld the short_channel_id in the case where we're
reconnecting and it's feeding us an array of failed htlcs.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We used to just manually set ROUTING_FLAGS_DISABLED, but that means we
then suppressed the real channel_update because we thought it was a
duplicate!
So use a local flag: set it for the channel when the peer disconnects,
and clear it when channeld sends a local update.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Just log the failed ones, not every connection and successful commands.
Before (VALGRIND=0 -n10):
111 passed, 1 skipped in 175.78 seconds
After:
111 passed, 1 skipped in 173.92 seconds
111 passed, 1 skipped in 164.16 seconds
111 passed, 1 skipped in 171.30 seconds
111 passed, 1 skipped in 180.05 seconds
111 passed, 1 skipped in 180.04 seconds
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This needs to be done separately from the rest of the daemon since we can
otherwise not make sure that it happens before the DB is freed and we might
still need the DN, and be running in a DB transaction, for some destructors to
run.
gossip_getnodes_entry was used by gossipd for reporting nodes, and for
reporting peers. But the local_features field is only available for peers,
and most other fields are only available from node_announcement.
Note that the connectd change actually means we get less information
about peers: gossipd used to do the node lookup for peers and include the
node_announcement information if it had it.
Since generate_wire.py can't create arrays-of-arrays, we add a 'struct
peer_features' to encapsulate the two feature arrays for each peer, and
for convenience we add it to lightningd/gossip_msg.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This patch guts gossipd of all peer-related functionality, and hands
all the peer-related requests to channeld instead.
gossipd now gets the final announcable addresses in its init msg, since
it doesn't handle socket binding any more.
lightningd now actually starts connectd, and activates it. The init
messages for both gossipd and connectd still contain redundant fields
which need cleaning up.
There are shims to handle the fact that connectd's wire messages are
still (mostly) gossipd messages.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
connectd has a dedicated fd to gossipd, so it can ask for a new gossip_fd
for a peer.
gossipd has a standalone routine to create a remote peer (this will
eventually be the only way gossipd creates a new peer).
For now lightningd creates a socketpair but doesn't run connectd, so
gossipd never sees any requests on this fd.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is just copying most of gossipd/gossip.c into connectd/connect.c.
It shares the same wire format as gossipd during transition, and changes
are deliberately minimal.
It also has an additional message 'connect_reconnected' which it sends
to the master daemon to tell it to kill a peer; gossipd relied on
closing the gossipfd to do this, but connectd doesn't maintain an fd
with remote peers.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This will avoid us having to round-trip to the HSM each time we want it.
For now we still derive it, too, and assert it's correct.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This means onchaind doesn't need the per-channel secret at all (aka. peer seed)
so we remove that from the onchaind_init message.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Really, we should have a 'struct point' since we don't use all points
as pubkeys. But this is the minimal fix to avoid type cast nastiness.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Removed `json_get_params`.
Also added json_tok_percent and json_tok_newaddr. Probably should
have been a separate PR but it was so easy.
[ Squashed comment update for gcc workaround --RR ]
Signed-off-by: Mark Beckwith <wythe@intrig.com>
connect_control.c, dev_ping.c, gossip_control.c, invoice.c.
This converts about 50% of all calls of `json_get_params` to `param`.
After trying (and failing) to squash and rebase #1682 I just made a new branch
from a patch file and closed#1682.
Signed-off-by: Mark Beckwith <wythe@intrig.com>
I crashed the HSMD, and it gave no output at all. That's because we
were only reading the status fd when we were waiting for a reply.
Fix this by using a separate request fd and status fd, which also means
that hsm_sync_read() is no longer required.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We need this later, to generate its seed. When we switch to lnd's key system,
we'll only need this, and not peerid.
Note also that the peerid is not just for messages any more, too.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Well, it's generated by shachain, so technically it is a sha256, but
that's an internal detail. It's a secret.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
- fixes problem with polling interval > 150 * 0.9
- fixes log message 'feerate hit floor' at every feerate change
- smoothed fee now reaches 90% of (exp weighted) fee estimates polled in last
120s, independent of polling interval
- only apply smoothing when effect > 10 percent so it doesn't correct forever
- fix indentation
This is a cosmetic change only. No functional changes.
I shortened the names of macros and changed param_parse() to param().
Also went through params.h with a fine-toothed comb and updated the comments
to reflect the current API.
I wanted to change the files:
params.c -> param.c
params.h -> param.h
run-params.c -> run->param.c
but that confused `git diff` for params.h so its best left for another PR.
I'm keeping #1682 updated locally with all these changes.
Signed-off-by: Mark Beckwith <wythe@intrig.com>
@wythe points out that many cases want a default value, not NULL.
Nicer to do it in the param_parse() call.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I'm not completely convinced that it's only ever set to a failcode
with the BADONION bit set, especially after the previous patches in
this series. Now that channeld can handle arbitrary failcodes passed
this way, simply rename it.
We add marshalling assertions that only one of failcode and failreason
is set, and we unmarshal an empty 'fail' to NULL (just the the
generated unmarshalling code does).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
==1224== Uninitialised byte(s) found during client check request
==1224== at 0x152CAD: memcheck_ (mem.h:247)
==1224== by 0x152D18: towire (towire.c:17)
==1224== by 0x152DA1: towire_u16 (towire.c:28)
==1224== by 0x142189: towire_failed_htlc (htlc_wire.c:29)
==1224== by 0x16343F: towire_channel_init (gen_channel_wire.c:596)
==1224== by 0x115C2C: peer_start_channeld (channel_control.c:249)
==1224== by 0x131701: peer_connected (peer_control.c:503)
==1224== by 0x117820: gossip_msg (gossip_control.c:182)
==1224== by 0x139D97: sd_msg_read (subd.c:500)
==1224== by 0x139676: read_fds (subd.c:327)
==1224== by 0x179D52: next_plan (io.c:59)
==1224== by 0x17A84F: do_plan (io.c:387)
==1224== Address 0x1ffefffabe is on thread 1's stack
==1224== in frame #2, created by towire_u16 (towire.c:26)
Followed by:
2018-06-18T21:53:04.129Z lightningd(1224): 03933884aaf1d6b108397e5efe5c86bcf2d8ca8d2f700eda99db9214fc2712b134 chan #1: Peer permanent failure in CHANNELD_NORMAL: lightning_channeld: received ERROR channel d0101486543e1a8b6871556a4fe1fba4ad4d83ce7f6f92919fd17bd1545d2fd5: UpdateFailMalformedHtlc message doesn't have BADONION bit set
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
typesafe_cb isn't suitable here, as it is simply a conditional cast,
and the result is passed through '...' and doesn't matter.
Reported-by: @wythe
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
json_listpeers returns an array of peers, and an array of nodes: the latter
is a subset of the former, and is used for printing alias/color information.
This changes it so there is a 1:1 correspondance between the peer information
and nodes, meaning no more O(n^2) search.
If there is no node_announce for a peer, we use a negative timestamp
(already used to indicate that the rest of the gossip_getnodes_entry
is not valid).
Other fixes:
1. Use get_node instead of iterating through the node map.
2. A node without addresses is perfectly valid: we have to use the timestamp
to see if the alias/color are set. Previously we wouldn't print that
if it didn't also advertize an address.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
There doesn't seeem to be a need for this anymore (unless I'm missing something).
I added the sendpay_nulltok() unit test to confirm.
Signed-off-by: Mark Beckwith <wythe@intrig.com>
@rustyrussell showed we don't need temporary objects for params.
This means params no longer need a tal context.
Signed-off-by: Mark Beckwith <wythe@intrig.com>
@wythe points out we don't need to keep the around now param_is_set()
is removed. We can in fact go further and avoid marshalling them into
temporary objects at the caller altogether.
This means internally we have an array of struct param, rather than an
array of 'struct param *', which causes most of the noise in this
patch.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We're using a macro anyway, so appending "" make it a compile-time check.
Complicates testing a bit, since we actually use generated names there.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is a bit more natural, IMHO. The only issue is that json_tok_tok is
special, so we end up with param_opt_tok() if you really want an optional
generic token.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is part of #1464 and incorporates Rusty's suggested updates from #1569.
See comment in param.h for description, here's the basics:
unsigned cltv;
const jsmntok_t *note;
u64 msatoshi;
struct param * mp;
if (!param_parse(cmd, buffer, tokens,
param_req("cltv", json_tok_number, &cltv),
param_opt("note", json_tok_tok, ¬e),
mp = param_opt("msatoshi", json_tok_u64, &msatoshi),
NULL))
return;
if (param_is_set(mp))
do_something()
There is a lot of developer mode code to make sure we don't make mistakes,
like trying to unmarshal into the same variable twice or adding a required param
after optional.
During testing, I found a bug (of sorts) in the current system. It allows you
to provide two named parameters with the same name without error; e.g.:
# cli/lightning-cli -k newaddr addresstype=p2sh-segwit addresstype=bech32
{
"address": "2N3r6fT65PhfhE1mcMS6TtcdaEurud6M7pA"
}
It just takes the first and ignores the second. The new system reports this as an
error for now. We can always change this later.
structeq() is too dangerous: if a structure has padding, it can fail
silently.
The new ccan/structeq instead provides a macro to define foo_eq(),
which does the right thing in case of padding (which none of our
structures currently have anyway).
Upgrade ccan, and use it everywhere. Except run-peer-wire.c, which
is only testing code and can use raw memcmp(): valgrind will tell us
if padding exists.
Interestingly, we still declared short_channel_id_eq, even though
we didn't define it any more!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
`getinfo` has been providing the blockheight for a good while and doesn't
require the `DEVELOPER=1` flag during compilation, so it should be the preferred
method to retrieve the blockchain height.
Implements an EWMA for the fee estimation. Achieves 90% influence of the newer
fee after 5 minutes, and adjusts to the polling rate that is configured.
Gossipd will ignore the second one, but doing it in the front end
gives an explicit error message.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
New codes: FUND_MAX_EXCEEDED, FUND_CANNOT_AFFORD, FUND_DUST_LIMIT_UNMET.
The error message "Cannot afford fee" was not exactly correct because
it would also occur if the amount requested could not be afforded. So
I changed it to the more generic "Cannot afford transaction".
Other things:
* Fixed off-by-one satoshi in fundchannel manpage.
* Changed 'arror' to 'error' because we are not pirates.
Turn req_running into a pointer to the current bcli structure, which means
the leak detection can find it.
Also suppress leaks in the case where we're only attached to a timer
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
During a meeting earlier this week we agreed with Eclair to temporarily
increase the final CLTV delta in our invoices to establish
compatibility with the already deployed Eclair wallets. They in turn
agreed to remove the enforcement of higher final CLTV deltas, or bump
it locally should it not match their expectations as allowed by
BOLT 11. This has since been implemented in ACINQ/eclair#627.
satoshis.place was slowing to a crawl, c-lightning was unresponsive.
Logs revealed charged doing many, many listinvoice <label> RPCs.
We were iterating the entire db every time: stop that!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The fee range can sometimes cause channels to be closed when the estimator
jumps. This has been the case a few times in the last months, and causes a
number of channels to be closed, and issue reports to be filed.
Increasing this from 5x to 10x should get rid of 84%+ of these
closures (measured based on 1h windows over the last 6 months and assuming
worst case situations).
Signed-off-by: Christian Decker <decker.christian@gmail.com>
I still believe that 2 weeks is way too much, but we were promised that these
defaults would be slowly reduced to saner values as the stability increases.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Compares the `blocknum` in the `short_channel_id` with the range of blocks we
store in the database and abort if we should have known about it. Avoids
bombarding `bitcoind` with requests for channels that have already been spent or
were invalid in the first place.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Since we currently only (ab)use it to send everything, we need a way to
generate boutique queries for testing.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We're telling gossipd about disconnections anyway, so let's just use that signal
to disable both sides of the channel.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
This was failing some of our integration tests, i.e., the ones closing a channel
and not waiting for sigexchange. The remote node would often not be quick enough
to send us its disabling channel_update, and hence we'd still remember the
incoming direction. That could then be sent out as part of an invoice, and fail
subsequently. So just set both directions to be disabled and let the onchain
spend clean up once it happens.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Until now, `command_fail()` reported an error code of -1 for all uses.
This PR adds an `int code` parameter to `command_fail()`, requiring the
caller to explicitly include the error code.
This is part of #1464.
The majority of the calls are used during parameter validation and
their error code is now JSONRPC2_INVALID_PARAMS.
The rest of the calls report an error code of LIGHTNINGD, which I defined to
-1 in `jsonrpc_errors.h`. The intention here is that as we improve our error
reporting, all occurenaces of LIGHTNINGD will go away and we can eventually
remove it.
I also converted calls to `command_fail_detailed()` that took a `NULL` `data`
parameter to use the new `command_fail()`.
The only difference from an end user perspecive is that bad input errors that
used to be -1 will now be -32602 (JSONRPC2_INVALID_PARAMS).
This resolves the problem where both channeld and gossipd can generate
updates, and they can have the same timestamp. gossipd is always able
to generate them, so can ensure timestamp moves forward.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Because we have too many which are never used and I don't want to document
them.
1. Remove unused anchor_onchain_wait. When implemented, it should be
hardcoded to 100 or more.
2. Remove anchor_confirms_max. 10 always reasonable, and we can readd
an override option should someone need it.
3. max_htlc_expiry should be the same as locktime_max (which increases
from 3 to 5 days by default): they're both a limit on how long
funds can be locked up.
4. channel_update_interval should always be a dev option.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
