Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Optimise slasher DB layout and switch to MDBX #2776

Closed
wants to merge 18 commits into from
Closed

[Merged by Bors] - Optimise slasher DB layout and switch to MDBX #2776

wants to merge 18 commits into from

Conversation

michaelsproul
Copy link
Member

@michaelsproul michaelsproul commented Nov 4, 2021 •
edited
Loading

Issue Addressed

Closes #2286
Closes #2538
Closes #2342

Proposed Changes

Part II of major slasher optimisations after #2767

These changes will be backwards-incompatible due to the move to MDBX (and the schema change) 😱

  • Shrink attester keys from 16 bytes to 7 bytes.
  • Shrink attester records from 64 bytes to 6 bytes.
  • Separate DiskConfig from regular Config.
  • Add configuration for the LRU cache size.
  • Add a "migration" that deletes any legacy LMDB database.

@michaelsproul michaelsproul added work-in-progress PR is a work-in-progress slasher backwards-incompat Backwards-incompatible API change labels Nov 4, 2021
@michaelsproul
Copy link
Member Author

I've just pushed a commit that shrinks the attester records even further to a mere 6 bytes! This means that our attesters table is now 13 bytes per entry rather than 80! 🎉

There seems to be a slight hit to batch processing time, as we now have to load indexed attestations to check for double votes in the case where we process two distinct indexed attestations for the same validator. This shouldn't occur too often anymore because of de-duplication, and I've added the SLASHER_NUM_INDEXED_ATTESTATION_LOADS metric to track it.

I'm going to observe the performance and storage growth over a longer period before marking this ready for review

@michaelsproul michaelsproul force-pushed the slasher-shrink-attesters branch from f94079f to 9eed024 Compare November 5, 2021 05:44
@michaelsproul
Copy link
Member Author

michaelsproul commented Nov 5, 2021 •
edited
Loading

Threw an LRU cache in there to reduce the number of unnecessary reads. It hasn't impacted total running time as much as I thought, but it's still looking pretty low, and the slasher seems to settle into its happy path sooner after start up.

@michaelsproul michaelsproul force-pushed the slasher-shrink-attesters branch from aee885e to 73c34e7 Compare November 8, 2021 05:09
@michaelsproul michaelsproul force-pushed the slasher-shrink-attesters branch 2 times, most recently from 53cb4db to b59bf47 Compare November 23, 2021 23:34
@michaelsproul michaelsproul force-pushed the slasher-shrink-attesters branch from b59bf47 to 87ab60b Compare November 23, 2021 23:41
@michaelsproul michaelsproul marked this pull request as ready for review November 24, 2021 04:09
@michaelsproul michaelsproul added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Nov 24, 2021
@michaelsproul michaelsproul changed the title Optimise slasher database schema Optimise slasher DB layout and switch to MDBX Nov 24, 2021
michaelsproul
michaelsproul commented Nov 24, 2021
View reviewed changes
slasher/Cargo.toml Outdated Show resolved Hide resolved
@michaelsproul michaelsproul mentioned this pull request Nov 24, 2021
Open
@michaelsproul
Copy link
Member Author

This is ready for review and merge (although I understand we may want to prioritise merging Kintsugi)

@paulhauner paulhauner self-requested a review December 14, 2021 01:26
paulhauner
paulhauner approved these changes Dec 21, 2021
View reviewed changes
Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, I couldn't fault it! 🚀

@paulhauner paulhauner added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Dec 21, 2021
@michaelsproul
Copy link
Member Author

Glorious! Thank you!!

bors r+

bors bot pushed a commit that referenced this pull request Dec 21, 2021
@michaelsproul
Optimise slasher DB layout and switch to MDBX (#2776)
27783b5 
## Issue Addressed

Closes #2286
Closes #2538
Closes #2342

## Proposed Changes

Part II of major slasher optimisations after #2767

These changes will be backwards-incompatible due to the move to MDBX (and the schema change) 😱 

* [x] Shrink attester keys from 16 bytes to 7 bytes.
* [x] Shrink attester records from 64 bytes to 6 bytes.
* [x] Separate `DiskConfig` from regular `Config`.
* [x] Add configuration for the LRU cache size.
* [x] Add a "migration" that deletes any legacy LMDB database.
@bors
Copy link

bors bot commented Dec 21, 2021

Build failed (retrying...):

@michaelsproul
Copy link
Member Author

Fixing up some Rust 1.57.0 lints

@bors
Copy link

bors bot commented Dec 21, 2021

Canceled.

@michaelsproul
Copy link
Member Author

Let's try again

bors r+

bors bot pushed a commit that referenced this pull request Dec 21, 2021
@michaelsproul
Optimise slasher DB layout and switch to MDBX (#2776)
3b61ac9 
## Issue Addressed

Closes #2286
Closes #2538
Closes #2342

## Proposed Changes

Part II of major slasher optimisations after #2767

These changes will be backwards-incompatible due to the move to MDBX (and the schema change) 😱 

* [x] Shrink attester keys from 16 bytes to 7 bytes.
* [x] Shrink attester records from 64 bytes to 6 bytes.
* [x] Separate `DiskConfig` from regular `Config`.
* [x] Add configuration for the LRU cache size.
* [x] Add a "migration" that deletes any legacy LMDB database.
@bors
Copy link

bors bot commented Dec 21, 2021

Pull request successfully merged into unstable.

Build succeeded:

@bors bors bot changed the title Optimise slasher DB layout and switch to MDBX [Merged by Bors] - Optimise slasher DB layout and switch to MDBX Dec 21, 2021
@bors bors bot closed this Dec 21, 2021
@michaelsproul michaelsproul deleted the slasher-shrink-attesters branch December 21, 2021 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulhauner paulhauner paulhauner approved these changes

Assignees
No one assigned
Labels
backwards-incompat Backwards-incompatible API change ready-for-merge This PR is ready to merge. slasher
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@michaelsproul @paulhauner