-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
channeldb: specify freelist bbolt options by default #3278
channeldb: specify freelist bbolt options by default #3278
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
. But the only other large database I had on my machine was wallet.db which is the btcwallet repo. Perhaps watchtowers might use a lot of space, or there could be some design parameters that change in the future where the other databases start using more space (macaroons.db etc).
Indeed btcwallet and towers would also be good candidates for these settings! For towers we can do this on the client and tower side dbs.
Updated bbolt dependency to v1.3.3 per
Perfect, yeah i hit this panic in testing with the current version
@@ -10,7 +10,7 @@ require ( | |||
github.com/btcsuite/btcutil v0.0.0-20190425235716-9e5f4b9a998d | |||
github.com/btcsuite/btcwallet v0.0.0-20190628225330-4a9774585e57 | |||
github.com/btcsuite/fastsha256 v0.0.0-20160815193821-637e65642941 | |||
github.com/coreos/bbolt v1.3.2 | |||
github.com/coreos/bbolt v1.3.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit: swap the order of the commits so bbolt is updated first
This commit updates the bbolt dependency to v1.3.3 so that we can use the NoFreelistSync option without triggering a panic in case a rollback occurs.
This commit specifies two bbolt options when opening the underlying channel and watchtower databases so that there is reduced heap pressure in case the bbolt database has a lot of free pages in the B+ tree.
863db38
to
66d15c8
Compare
@cfromknecht Addressed comment and added the options to watchtower client + server. Can make a PR for btcwallet next. For testing, I'm wondering if the options should also be specified in unit/itests? If so, in order to reduce code duplication, I suppose I could add the options to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For testing, I'm wondering if the options should also be specified in unit/itests? If so, in order to reduce code duplication, I suppose I could add the options to channeldb/options.go similar to the way the cache sizes are setup?
I'd say yes in order to exercise what would be used in production. Adding the options wouldn't remove the duplication in btcwallet
, but seems fine to do for the other databases within lnd
.
@Crypt-iQ since the options are set inside the |
Hey you're right, I meant the other databases (sphinxreplay, macaroons) and some of the temporary test databases. I didn't look at the results of my search very closely, mb |
okay cool then i think we're good. sphinxreplay and macaroon dbs are less of a priority as they're much smaller, but not opposed to adding them as well |
The freelist on my 8MB sphinxreplay db has 5 free pages on it... so I don't think I'll include it here. And my macaroons.db is tiny. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀will start running this version to ensure there aren't any other regressions
@Crypt-iQ decided we should push this off to 0.8, since it's a pretty big change for a point release and want to leave ample time for testing |
@cfromknecht no problem, I'm looking into the InitialMmapSize option as well, so might be good to hold off until I can determine if it's worth adding |
@Crypt-iQ sounds good, yeah we haven't done much exploration wrt bbolt options, would be interested to hear what you find! |
Looked at It is also the case that long-running read transactions can block certain writes from happening. The read txns hold a lock on the mmap and writes that need to remap also need the lock, thus they are blocked. See: boltdb/bolt#472 The creator of boltdb suggested an mmap size of 1TB (boltdb/bolt#489 (comment)) but I think we could make do with much less given the size of the channel graph (my channel.db is only ~2G). It is virtual memory after all so we could set the Edit: Just remembered 32-bit systems exist and can't have memory-mapped files > 2G. Could just disable for 32-bit systems? On another note, I see no performance difference with a mmap size of 1TB and my regular mmap size of ~500MB. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🌋
We should get this in early on in the road to 0.8 so that we can get adequate testing in master. Been running on my node for the past week or so without issue.
I'm kind of indifferent on the Just a note. Looking at the boltdb code and issues, it seems that the mmap size cannot be greater than 2GB for 32-bit machines which makes sense. If |
I don't think we need to set this value in this PR, and can defer tuning it to a later PR.
Once this goes in, we can follow up with the change on the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🐝
Fixes #3241.
These options could really be specified anywhere that bbolt is opened, so if that's of interest I can also do that. But the only other large database I had on my machine was
wallet.db
which is thebtcwallet
repo. Perhaps watchtowers might use a lot of space, or there could be some design parameters that change in the future where the other databases start using more space (macaroons.db etc). Should I add these options to all the tests so they reflect the new behavior?Updated bbolt dependency to v1.3.3 per etcd-io/bbolt#153
The only downside is that if the db txn has to be rolled back in case of failure, the freelist must be built again temporarily which can be heavy with big databases. I think this is an acceptable tradeoff since this shouldn't be happening too often.