We already have 'struct node', so rename 'struct routing_channel' to
'struct chan', and 'struct node_connection' to 'struct half_chan'.
Other minor changes:
1. rstate->channels -> rstate->chanmap.
2. 'connections' -> 'half'.
3. connection_to -> half_chan_to
4. connection_from -> half_chan_from
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The containing `struct routing_channel` contains src and dst, so
remove them. However, the channel_update msgidx does belong int
`struct node_connection` along with the channel_update.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Returning the separate first routing_channel was a weird API: just
return the entire array. Sure, we have to treat the first node a bit
differently (because we don't charge ourselves fees), but it's still
simpler.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
To remove the redundant fields in `struct node_connection` (ie. 'src'
and 'dst' pointers) we need to deal with `struct routing_channel`.
This means we get a series of channels, from which the direction is
implied, so it's a bit more complex to decode. We add a helper
`other_node` to help with this, and since we're the only user of
`connection_to` we change that function to return the index.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Failure and pruning were the two places where a node_connection could
be freed; now they both deal with entire channels, we can remove the
NULL checks, and the destructor.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We discarded it; we should populate it. The comment is wrong, since
local_add_channel() doesn't add public channels, and we test that above.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is twice the 'update_channel_interval' we get handed.
We delete the non-existent channel_add_connection and delete_connection
declarations from the header too.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We currently give them a free pass. The simplest fix is to give them
an old timestamp on initialization.
We still skip unannounced channels, on the assumption that they're
ours. And we set the last_update_timestamp to -1 when we convert to
gossip_getchannels_entry to indicate no update.
This breaks the DEVELOPER=1 pruning test, since we hardcode the 1
week timeout. That's fixed in the next patch.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We make new_routing_channel() populate both connections
(active=false), so local_add_channel becomes simpler. We also
suppress listchannels output of active=false unannounced channels, to
avoid breaking tests (also, these are unusable, so it makes sense to
omit them)
It also seems the logic in add_channel_direction is legacy: a
channel_announce cannot replace the scid (that would be a different
channel), we don't allow duplicate announcements, and the announcement
is never NULL.
And since we disallow repeated channel_announce already, I believe
'forward' is always true, greatly simplifying the logic in
handle_pending_cannouncement.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This makes 'routing_channel' the primary object in the system; it can have
one or two 'node_connection's attached, and points to two nodes.
The nodes are freed when no more routing_channel refer to them. The
routing_channel are freed when they contain no more 'node_connection'.
This fixes#1072 which I surmise was caused by a dangling
routing_channel after pruning.
Each node contains a single array of 'routing_channel's, not one for
each direction. The 'routing_channel' itself orders nodes in key
order (conveniently the index is equal to the direction flag we use),
and 'node_connection' with source in the same order.
There are helpers to assist with common questions like "which
'node_connection' leads out of this node?".
There are now two ways to find a channel:
1. Direct scid lookup via rstate->channels map.
2. Node key lookup, followed by channel traversal.
Several FIXMEs are inserted for where we can now do things more optimally.
Fixes: #1072
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We're going to make it a first-class citizen, and pending routing_channel
are not real ones (in particular, we don't want to create pending nodes).
We had a linked list called rstate->pending_cannouncement which we didn't
actually use, so put that back for now and add a FIXME to use a faster
data structure.
We need to check that list now in handle_channel_update, but we never
have a real routing_channel and a pending, unless the routing_channel
isn't public.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Now we have them, let's use them. I missed one case deliberately, since
that causes merge conflicts when I replace it in a following patch.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I'm not completely conviced that we can't end up removing pending things,
so change asserts to simple returns.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
If we make destroy_node() remove itself from the map, then we simply
need to free it.
We can batch the frees (as we need) simply by reparenting all the pruned
nodes onto a single temporary parent, then freeing it, relying on tal's
internal datastructures.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
get_connection_by_scid() and update_to_pending() both do the same
lookup which we did in handle_channel_update().
Do the lookup once, and simplify the others.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We always hand in "NULL" (which means use tal_len on the msg), except
for two places which do that manually for no good reason.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We usually did this, but sometimes they were named after what they did,
rather than what they cleaned up.
There are still a few exceptions:
1. I didn't bother creating destroy_xxx wrappers for htable routines
which already existed.
2. Sometimes destructors really are used for side-effects (eg. to simply
mark that something was freed): these are clearer with boutique names.
3. Generally destructors are static, but they don't need to be: in some
cases we attach a destructor then remove it later, or only attach
to *some* cases. These are best with qualifiers in the destroy_<type>
name.
Suggested-by: @ZmnSCPxj
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We get intermittant failure: WIRE_UNKNOWN_NEXT_PEER (First peer not ready)
because CHANNELD_NORMAL and actually telling gossipd that the channel
is available are distinct things: we need both.
(For test_closing_different_fees, we were testing CHANNELD_NORMAL on
the peer, not on l1, too).
But we may also directly send the announcement sigs if the height is
sufficient, so the simplest is to unify the messages.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Commit a57a2dcb86 introduced a time_t
in routing.h. So also move the time.h include to the header. This
fixes the build on FreeBSD.
Signed-off-by: Wladimir J. van der Laan <laanwj@gmail.com>
We were dropping these on the floor while checking for txout. So now
we add a map that holds announcements while we are checking.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Adding channels that we are currently verifying to the map, and
skipping if we already have a channel at that position.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
We use this technique for the other tags, so use it here too.
This was drawn to my attention when I made more than 10 channels in a
block, and the string changed length:
Valgrind error file: valgrind-errors.31415
==31415== Conditional jump or move depends on uninitialised value(s)
==31415== at 0x4C35E20: bcmp (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==31415== by 0x11A624: queue_broadcast (broadcast.c:40)
==31415== by 0x118D93: handle_pending_cannouncement (routing.c:704)
==31415== by 0x1109E3: handle_txout_reply (gossip.c:1796)
==31415== by 0x111177: recv_req (gossip.c:1955)
==31415== by 0x136723: next_plan (io.c:59)
==31415== by 0x137220: do_plan (io.c:387)
==31415== by 0x13725E: io_ready (io.c:397)
==31415== by 0x138B97: io_loop (poll.c:305)
==31415== by 0x111352: main (gossip.c:2022)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We drop all but the first announcement, so any work that is done for a
channel that we already know is wasted. Pulling this up duplicates
some of the work but allows us to skip the costly txout check.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
`tal_fmt` overallocates the returned string under some circumstances,
meaning that the trailer of the formatted string is unset, but still
considered in `tal_len`. The solution then is to truncate the
formatted string to the real string length. Only necessary here, since
we mix strings and `tal_len`.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Otherwise, we otherwise end up with out-of-order updates
(ie. preceeding announcements).
I assume that is because of the locally-inserted connections.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is done it two parts, since we have to ask the main daemon to do
the lookup for us.
If this becomes a bottleneck, we can have a separate daemon, or even
an RPC pipe to bitcoind ourselves.
Fixes: #403
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Sometimes we could get into a situation in which we knew the channel
but couldn't find it via the short_channel_id. That'd result in a
replacement which triggered an assert.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
The use of status_failed() requires a stubs update, which fails
with unnamed parameters, so tweak the status.h header as well.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
If we side-load a channel, using local-add or the removed JSON-RPC
call, then we could end up in a situation in which a channel is
present, but has no associated channel_announcement. The presence of
the channel_announcement was used to identify new channels, so this
could lead to channels always being considered new. This then caused
the announcements being added to the queue always, resulting in
channel_updates preceeding the announcement.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
We should never be evicting channel_announcements because a) they were
deeply buried and should not change the short_channel_id/tag, b) they
are static.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
It's just a sha256_double, but importantly when we convert it to a
string (in type_to_string, which is used in logging) we use
bitcoin_blkid_to_hex() so it's reversed as people expect.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
If channel_announce is rebroadcast, it should replace the existing one
in-place. We currently only do this if we start from the unsigned one
and replace it with the signed one when we hit 6 confirms.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This check is expensive, so just restrict msatoshi going in, as well
as turn off channels charging more than 24x fee.
# 1M nodes:
$ /gossipd/test/run-bench-find_route 1000000 1 > /tmp/out
=> 44164 msec
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We can't get them; channel_update doesn't support it.
# 1M nodes:
$ /gossipd/test/run-bench-find_route 1000000 1 > /tmp/out
=> 47677 msec
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This will later be used to determine whether or not we should announce
ourselves as a node.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
In future it will have TOR support, so the name will be awkward.
We collect the to/fromwire functions in common/wireaddr.c, and the
parsing functions in lightningd/netaddress.c.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It makes it impossible to embed an ipaddr in another structure, since we
always try to skip over any zeroes, which may swallow a following field.
Do the skip specially for the case where we're parsing routing messages:
we never use padding for our own internal messages anyway.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
1. The code to skip over padding didn't take into account max.
2. It also didn't use symbolic names.
3. We are not supposed to fail on unknown addresses, just stop parsing.
4. We don't use the read_ip/write_ip code, so get rid of it.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I missed these when I removed the legacy daemon. We also remove the
min_blocks field which was always 0.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Use a negative timestamp as the flag for this, making the test simple.
This allows valgrind to detect that we're accessing them prematurely,
including across the wire on gossip_getchannels_entry.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I had a routing problem, and wrote a simple unit test which passed. So
I wrote one which copied the failure case (and importantly, had a non-1
fee factor), which triggerd it.
In that real example, we underflowed which resulted in us not finding
a route. Simply don't consider routes which are infinite.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Since we initialize last_timestamp to 0, we ignore any initial update
with this timestamp. Don't compare it if we don't already have an
update, and don't initialize it, so valgrind can tell us if we use
it accidentally.
b'lightning_gossipd(3368): TRACE: Received channel_update for channel 6892:2:1(0)'
b'lightning_gossipd(3368): TRACE: Ignoring outdated update.'
b'lightning_gossipd(3368): TRACE: Received channel_update for channel 6893:2:1(1)'
b'lightning_gossipd(3368): TRACE: Channel 6893:2:1(1) was updated.'
The same logic applies to node_updates, so we do the same there.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
