Final gossip store optimization #2644
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
m-schmoock
reviewed
May 17, 2019
There was a problem hiding this comment.
Great change(set).
This is a bit large for a detailed analysis, we will have to see if it behaves as expected.
But here are my two Satoshis...
-
There is a conflict in
openingd/openingd.c(which is why jenkins wont build, right?) . -
I thought putting declarations at function start was project style, but Im fine with both.
-
On my testnet node I get the following upon startup, not sure if its related. Is there a testcase for upgrading from 4 to 5?
2019-05-17T08:50:14.521Z lightning_gossipd(19181): Upgraded gossip_store from version 4 to 5
2019-05-17T08:50:14.522Z lightning_gossipd(19181): gossip_store: Bad channel_update (0102231f797252fdb20277b5bee70f942e08cd8a0a7361ea2ee989b3dcee8a63d7470a845ebda5381bac371a1b037e33f883e9cc7531fb3e8c53ebd7315f673b5c5243497fd7f826957108f4a30fd9cec3aeba79972084e90ead01ea33090000000016b6bc00001d00005cb3e7890100002800000000000003e8000003e800000001000000000229e4e0) truncating to 605593
- after startup, my
lightning-channeldremain at 100% for an extensive periord of time, it has not stopped while writing...- Edit: now 10 minutes + 100% CPU on a decent machine...
- Retest on master branch: no such behaviour
PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND
19207 will 20 0 9264 3284 2700 R 100.0 0.0 3:22.13 lightning_chann
19211 will 20 0 9264 3196 2604 R 100.0 0.0 3:26.07 lightning_chann
19213 will 20 0 9264 3068 2472 R 100.0 0.0 3:17.24 lightning_chann
| peer_failed_connection_lost(); | ||
|
|
||
| type = fromwire_peektype(reply); | ||
| if (type == replytype) { |
There was a problem hiding this comment.
I would drop the int type; (line 825) and inline this to:
if (fromwire_peektype(reply) == replytype) {
... or put the int type; at the function start.
There was a problem hiding this comment.
We reuse type below this in the status msg though...
cli/lightning-cli.c
Outdated
| resp = tal_arr(ctx, char, 1000); | ||
| toks = tal_arr(ctx, jsmntok_t, 100); | ||
| resp_len = 1000; | ||
| resp = malloc(resp_len); |
There was a problem hiding this comment.
This is seems not being released/freed , right?
There was a problem hiding this comment.
True, though we exit. Neater to free though, will fix in separate patch.
rustyrussell
changed the title
I reproduced this here. The upgrade logic was too simplistic, but it's not worth fixing. We simply truncated the gossip_store on upgrade again :( Before 0.7.1 I'll make sure we're not hammered by every peer feeding us gossip, to mitigate.
Hmm, that's weird. If you can gdb attach to it, or even send channeld a SIGABRT and look for a backtrace in the logs, that might offer a clue? Thanks! |
rustyrussell
force-pushed
the
final-gossip-store-optimization
branch
from
8aa7ee4 to
94d7220
Compare
|
Rebased on master to resolve conflict, and added a final commit to drop upgrade code. |
SimonVrouwe
reviewed
May 18, 2019
@rustyrussell Here you go, I tried again, luckily same happened again. I SIGABRTed ( Click to expand (huge) Stacktrace logs! |
rustyrussell
force-pushed
the
final-gossip-store-optimization
branch
3 times, most recently
from
5d2d012 to
182e617
Compare
Thanks! Indeed, I reproduced it here too. It spins on a 0 timeout until we get a timestamp_filter message, which doesn't happen with modern lnd. I've fixed this up and rebased, am testing now... |
|
better, but |
|
before |
It means this was the last user of the broadcast ordering uintmap; we can now remove it. |
Indeed! Nice addition, I've reproduced here and am bisecting. Sleep soon, so it'll be done in about 13 hours I'd guess. |
rustyrussell
force-pushed
the
final-gossip-store-optimization
branch
from
182e617 to
3547623
Compare
rustyrussell
changed the title
rustyrussell
force-pushed
the
final-gossip-store-optimization
branch
from
3547623 to
9ff3554
Compare
|
@rustyrussell Testing this PR, with a mainnet gossip_store (gossip_store_fromHPLaptop2_May23_1521.zip) the command Click to expand crashlogAnother thing is that at restart, |
|
Thanks, will investigate further.... Truncation is almost always a sign of previous corruption (though compaction will usually find those issues too). At least this code is now getting more testing! |
rustyrussell
changed the title
|
Maybe this is of interest, since mid April messages like: Click to expand for grep result |
|
OK, I reproduced this problem, and enhanced our test to expose it: we weren't deleting private channels from the store when they become public. It also found something else as well: our counter of entries was not counting everything. Here's the total difference after rebase: diff --git a/gossipd/gossip_store.c b/gossipd/gossip_store.c
index 779525580..58957c3da 100644
--- a/gossipd/gossip_store.c
+++ b/gossipd/gossip_store.c
@@ -385,6 +385,8 @@ u64 gossip_store_add(struct gossip_store *gs, const u8 *gossip_msg,
}
gs->count++;
+ if (addendum)
+ gs->count++;
return off;
}
diff --git a/gossipd/routing.c b/gossipd/routing.c
index 1dc7015e5..ecef419a3 100644
--- a/gossipd/routing.c
+++ b/gossipd/routing.c
@@ -1413,8 +1413,10 @@ bool routing_add_channel_announcement(struct routing_state *rstate,
}
/* Pretend it didn't exist, for the moment. */
- if (chan)
+ if (chan) {
+ remove_channel_from_store(rstate, chan);
free_chan(rstate, chan);
+ }
uc = tal(rstate, struct unupdated_channel);
uc->channel_announce = tal_dup_arr(uc, u8, msg, tal_count(msg), 0);
diff --git a/tests/test_gossip.py b/tests/test_gossip.py
index 8e36498a1..5efb3dd65 100644
--- a/tests/test_gossip.py
+++ b/tests/test_gossip.py
@@ -1094,10 +1094,22 @@ def test_gossip_store_compact(node_factory, bitcoind):
l1, l2, l3 = node_factory.line_graph(3, fundchannel=False)
# Create channel.
- l2.fund_channel(l3, 10**6)
+ scid23 = l2.fund_channel(l3, 10**6)
+ # Replace the update
l2.rpc.setchannelfee(l3.info['id'], 20, 1000)
- l2.daemon.wait_for_log(r'Received channel_update for channel')
+ l2.daemon.wait_for_log(r'Received channel_update for channel {}'.format(scid23))
+
+ # Have that channel announced.
+ bitcoind.generate_block(5)
+ wait_for(lambda: [c['active'] for c in l2.rpc.listchannels(scid23)['channels']] == [True, True])
+
+ # Create another channel, which will stay private.
+ scid12 = l1.fund_channel(l2, 10**6)
+
+ # Replace the update
+ l1.rpc.setchannelfee(l2.info['id'], 20, 1000)
+ l2.daemon.wait_for_log(r'Received channel_update for channel {}'.format(scid12))
# Now compact store.
l2.rpc.call('dev-compact-gossip-store') |
rustyrussell
force-pushed
the
final-gossip-store-optimization
branch
2 times, most recently
from
ae55921 to
724b0cf
Compare
|
Trivial rebase to fix conflict. |
Merged
cdecker
approved these changes
Jun 3, 2019
They're really gossipd-internal, and we don't want per-peer daemons to confuse them with normal updates. I don't bump the gossip_store version; that's coming with another update anyway. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
More useful if something is wrong. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We currently don't care, but the next patch means we have to find them again. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We're about to bump version again, and the code to upgrade it was quite hairy (and buggy!). It's not worthwhile for such a poorly-tested path: I will just add code to limit how much incoming gossip we get to avoid flooding when we upgrade, however. I also use a modern gossip_store version in our test_gossip_store_load test, instead of relying on the upgrade path. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We use the high bit of the length field: this way we can still check that the checksums are valid on deleted fields. Once this is done, serially reading the gossip_store file will result in a complete, ordered, minimal gossip broadcast. Also, the horrible corner case where we might try to delete things from the store during load time is completely gone: we only load non-deleted things. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…pdate. We ask gossipd for the channel_update for the outgoing channel; any other messages it sends us get queued for later processing. But this is overzealous: we can shunt those msgs to the peer while we're waiting. This fixes a nasty case where we have to handle WIRE_GOSSIPD_NEW_STORE_FD messages by queuing the fd for later. This then means that WIRE_GOSSIPD_NEW_STORE_FD can be handled internally inside handle_gossip_msg(), since it's always dealt with the same, simplifying all callers. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Encapsulating the peer state was a win for lightningd; not surprisingly, it's even more of a win for the other daemons, especially as we want to add a little gossip information. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
They already send *us* gossip messages, so they have to be distinct anyway. Why make us both do extra work? Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
There's a corner case where otherwise a reader could see the header and not the body of a message. It could handle that in various ways, but simplest (and most efficient) is to avoid it happening. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We used to generate this in the caller, then save it in case we needed to retry. We're about to change the message we send to lightningd, so we'll need to regenerate it every time; just hand all the extra args into peer_connected() and we can generate the `connect_peer_connected` msg there. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> 1diff --git a/connectd/connectd.c b/connectd/connectd.c index 94fe50b..459c9ac 100644
…elves. Keeping the uintmap ordering all the broadcastable messages is expensive: 130MB for the million-channels project. But now we delete obsolete entries from the store, we can have the per-peer daemons simply read that sequentially and stream the gossip itself. This is the most primitive version, where all gossip is streamed; successive patches will bring back proper handling of timestamp filtering and initial_routing_sync. We add a gossip_state field to track what's happening with our gossip streaming: it's initialized in gossipd, and currently always set, but once we handle timestamps the per-peer daemon may do it when the first filter is sent. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The single caller can easily use transfer_store_msg instead. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
(We don't increment the gossip_store version, since there are only a few commits since the last time we did this). This lets the reader simply filter messages; this is especially nice since the channel_announcement timestamp is *derived*, not in the actual message. This also creates a 'struct gossip_hdr' which makes the code a bit clearer. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We need the timestamp for channel_announcement, but this is simplified because MCP always follows the channel_announcement by a channel_update. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This means we intercept the peer's gossip_timestamp_filter request in the per-peer subdaemon itself. The rest of the semantics are fairly simple however. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…unt. We didn't count some records before, so we could compare the two counters. This is much simpler, and avoids reliance on bs. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We have a problem: if we get halfway through writing the compacted store and run out of disk space, we've already changed half the indexes. This changes it so we do nothing until writing is finished: then we iterate through and update indexes. It also weans us off broadcast ordering, which we can now eliminated. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This clarifies things a fair bit: we simply add and remove from the
gossip_store directly.
Before this series: (--disable-developer, -Og)
store_load_msec:20669-20902(20822.2+/-82)
vsz_kb:439704-439712(439706+/-3.2)
listnodes_sec:0.890000-1.000000(0.92+/-0.04)
listchannels_sec:11.960000-13.380000(12.576+/-0.49)
routing_sec:3.070000-5.970000(4.814+/-1.2)
peer_write_all_sec:28.490000-30.580000(29.532+/-0.78)
After: (--disable-developer, -Og)
store_load_msec:19722-20124(19921.6+/-1.4e+02)
vsz_kb:288320
listnodes_sec:0.860000-0.980000(0.912+/-0.056)
listchannels_sec:10.790000-12.260000(11.65+/-0.5)
routing_sec:2.540000-4.950000(4.262+/-0.88)
peer_write_all_sec:17.570000-19.500000(18.048+/-0.73)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
For some reason I was reluctant to use the hc local variable; I even re-declared it! Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Triggered by a previous variant of this PR, but a goo1d idea to simply discard the store in general when we get a duplicate entry. We crash trying to delete old ones, which means writing to the store. But they should have already been deleted. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
(Or, if we crashed before we got to write out the channel_update). It's a corner case, but one reported by @darosior and reproduced on my test node (both with bad gossip_store due to previous iterations of this patchset!). Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's not required, but it means valgrind won't complain about leaks. Suggested-by: @m-schmoock Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
final-gossip-store-optimization
branch
from
724b0cf to
dcb8646
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is based on #2685 which fixes some gossip bugs. Review and merge that first :)
This moves all traversal out to the per-peer daemons. It's pretty nice, actually.
Before this series: (--disable-developer, -Og)
After: (--disable-developer, -Og)