331: Group database key prefixes as enums. #663

rndhouse · 2022-09-26T15:51:53Z

This commit reformulates database key prefixes such that they are grouped into enums.

Currently, key prefixes are listed as separate const values. This formulation makes it difficult to manage key prefixes (import, iterate, test, etc), particularly when it comes to dbdump. This commit simplifies key prefix management using enums and a key prefix trait. It also makes it possible to test and ensure key prefixes are locally unique.

rndhouse · 2022-09-26T16:08:00Z

This PR is currently a proof-of-concept. I've kept changes to a minimum. fedimint-api should build, other components probably wont.

justinmoon · 2022-09-26T19:15:07Z

The move to enums seems like big improvement. I'm always a little nervous when I add new database keys just because I know there's no guarantee against duplicate key. Will defer to @elsirion who has worked with the database a lot more than I have ...

dpc · 2022-09-27T18:50:52Z

fedimint-api/Cargo.toml

@@ -25,6 +25,7 @@ rand07 = { version = "0.7", package = "rand" }
 secp256k1-zkp = { version = "0.6.0", features = [ "use-serde", "bitcoin_hashes", "global-context" ] }
 serde = { version = "1.0.145", features = [ "derive" ] }
 serde_json = "1.0.85"
+strum = { version = "0.24.1", features = ["derive"] }


If I'm seeing it right, this could be a dev-depenendecny? (sorry if I missed something)

I think you are correct where this draft PR (in its current state) is concerned. But I would like to be able to iterate over enum fields in non-dev where dbdump is concerned, see issue #331.

Do we really need this dep? If it's just for iterating over enum variants we should probably just use a macro that implements this since strum seems to do a lot more.

Copying the strum::EnumIter macro from strum would mean adding at least 160 lines of code and a new dependency on syn. I haven't investigated whether it might be possible to do the same thing with less code.

Personally I would lean towards keeping things simple and using strum. I think that the tradeoff of an additional dependency is worth it.

But let me know what the maintainers want and I'll do what's necessary.

dpc · 2022-09-27T18:54:18Z

fedimint-api/src/db/mod.rs

+    const KEY_PREFIX: Self::KeyPrefixType;
+}
+
+pub trait DbKeyToBytes: Debug {


I would name it DBKey. The that it converts "to bytes" is kind of less important (interface detail). The main thing is that "it functions as a database key".

There is a trait called DatabaseKey defined below. I think that sorting out and simplifying traits should be a separate PR.

I renamed to DbKeyToBytes from DatabaseKeyPrefix in this instance because I needed to commandeer the "prefix" name/concept.

dpc · 2022-09-27T18:56:30Z

fedimint-api/src/db/mod.rs

+// This trait is implemented for database key prefix enums.
+pub trait DbKeyPrefix {
+    type KeyPrefixType: std::convert::Into<u8>;
+    const KEY_PREFIX: Self::KeyPrefixType;


Any reason this can't be just const KEY_PREFIX: u8;?

I don't think it's possible to convert from enum to u8 given that KEY_PREFIX is const. The error should be:

error[[E0015]](https://doc.rust-lang.org/stable/error-index.html#E0015): cannot call non-const fn `<Prefix as Into<u8>>::into` in constants

Here's a Rust playground showing what I mean:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=da4c4f7c283bd3bfdfa3529b68cb6006

Do you know of a way around this error?

I suppose each enum could implement its own const fn as_u8(self) -> u8.

But I think that by bounding on trait std::convert::Into<u8> we could retain the enum type for longer without collapsing into the anonymous u8 primitive. Might make debugging/serialization more informative for example.

This could be solved in a much easier way imo: Playground

#[repr(u8)] enum Foo { Bar = 0x01, Baz = 0x02, } const BAZ: u8 = Foo::Baz as u8; fn main() { println!("BAZ = {}", BAZ); }

Thank you both, that's very clear code @elsirion , I'll go with that.

dpc · 2022-09-27T19:20:34Z

BTW. I didn't have opportunity to dive into that code, but I don't understand why db code is so complex .

Seems like the prefix could be just u8 with a wrapper if that's all we plan to support:

#[derive(Copy, Clone, Debug)]
struct DBPrefix(u8);

enum KnownDBPrefix {
  Something,
  SomethingElse,
}

impl DBPrefix {
  const fn as_u8(self) -> u8 { self.0 }
  fn get_standard_prefix_range() -> (u8, u8) {
     // fedimint reserves first 128 ids for own use
    (0, 128)
  }
}

impl From<StandardDBPrefix> for DBPrefix { ... }

// If we need to support prefixes supported by exteran code (like modules).
// They can take care of uniqueness with their own enum.
// Possibly check here and panic if value conficlits with `get_standard_prefix_range`
impl From<u8> for DBPrefix { ... }

and the key just &[u8] - again if that's all we plan to support:

struct DBKey<'a>(Cow<'a, [u8]>);

Cow possibly to handle cases where it needs calculated (and allocated) in flight. Maybe it's unnecessary and we could either use always Vec or always &'a [u8].

with a final:

struct DBPrefixedKey<'a> {
  prefix: DBPrefix,
  key: DBKey<'a>,
}

Or some stuff like this. I guess I just don't see what are all these traits buying us there, while they cost a lot of complexity.

rndhouse · 2022-09-29T10:12:54Z

@dpc I agree with you that there's room to simplify the db traits/code.

I would like to be able to group prefixes so that I can complete #331
Let me know how you'd like to see my grouping method proposal (this PR) improved so that I can get to something you'd be happy to merge.

dpc · 2022-09-30T05:49:49Z

@rndhouse I think we are going to need @elsirion to tell us what he thinks. :)

elsirion

I like making the prefix an enum, not sure if all the added type complexity is necessary, left an alternative idea (idk if it works, only built a prototype). If not I'm fine with this version.

elsirion · 2022-10-04T09:02:50Z

fedimint-api/Cargo.toml

@@ -25,6 +25,7 @@ rand07 = { version = "0.7", package = "rand" }
 secp256k1-zkp = { version = "0.6.0", features = [ "use-serde", "bitcoin_hashes", "global-context" ] }
 serde = { version = "1.0.145", features = [ "derive" ] }
 serde_json = "1.0.85"
+strum = { version = "0.24.1", features = ["derive"] }


Do we really need this dep? If it's just for iterating over enum variants we should probably just use a macro that implements this since strum seems to do a lot more.

elsirion · 2022-10-04T09:04:18Z

fedimint-api/src/db/batch.rs

@@ -18,15 +18,15 @@ pub enum BatchItem {
    /// Insets new element, even if it already exists
    InsertElement(Element),
    /// Deletes element, errors if it doesn't exist
-    DeleteElement(Box<dyn DatabaseKeyPrefix + Send>),
+    DeleteElement(Box<dyn DbKeyToBytes + Send>),


Could you please try to separate logic changes from renaming in the future (different commits in the same PR)? It's hard to focus on the relevant parts if it's mixed.

That makes sense to me, I'll aim to do that next time.

elsirion · 2022-10-04T09:08:34Z

fedimint-api/src/db/mod.rs

+// This trait is implemented for database key prefix enums.
+pub trait DbKeyPrefix {
+    type KeyPrefixType: std::convert::Into<u8>;
+    const KEY_PREFIX: Self::KeyPrefixType;


This could be solved in a much easier way imo: Playground

#[repr(u8)] enum Foo { Bar = 0x01, Baz = 0x02, } const BAZ: u8 = Foo::Baz as u8; fn main() { println!("BAZ = {}", BAZ); }

elsirion · 2022-10-04T09:29:10Z

fedimint-api/src/db/mod.rs

+// Given a database key prefix enum iterator, this function ensures that its prefix values
+// are unique within the enum scope.
+pub fn ensure_unique_key_prefixes<T: std::iter::Iterator + Clone>(db_key_prefix_iter: T)
+where
+    <T as Iterator>::Item: std::convert::Into<u8>,
+{
+    let prefixes: std::collections::BTreeSet<u8> = db_key_prefix_iter
+        .clone()
+        .map(|prefix| prefix.into())
+        .collect();
+
+    let result = prefixes.len();
+    let expected = db_key_prefix_iter.count();
+
+    assert_eq!(result, expected);
+}
+


This approach would automatically prevent duplicate prefixes.

rndhouse · 2022-10-04T14:57:21Z

@elsirion @dpc I've force pushed a new commit which doesn't modify traits and implements enums throughout the codebase.

codecov-commenter · 2022-10-09T15:56:17Z

Codecov Report

Base: 59.38% // Head: 59.24% // Decreases project coverage by -0.13% ⚠️

Coverage data is based on head (4c8d483) compared to base (0cf81c5).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #663      +/-   ##
==========================================
- Coverage   59.38%   59.24%   -0.14%     
==========================================
  Files         101      101              
  Lines       16259    16267       +8     
==========================================
- Hits         9656     9638      -18     
- Misses       6603     6629      +26

Impacted Files	Coverage Δ
client/client-lib/src/ln/db.rs	`60.00% <0.00%> (-6.67%)`	⬇️
client/client-lib/src/mint/db.rs	`85.71% <0.00%> (-14.29%)`	⬇️
client/client-lib/src/wallet/db.rs	`33.33% <0.00%> (-16.67%)`	⬇️
fedimint-api/src/db/mod.rs	`80.85% <0.00%> (-0.29%)`	⬇️
fedimint-server/src/db.rs	`88.88% <0.00%> (-11.12%)`	⬇️
modules/fedimint-ln/src/db.rs	`91.66% <0.00%> (-8.34%)`	⬇️
modules/fedimint-mint/src/db.rs	`90.00% <0.00%> (-10.00%)`	⬇️
modules/fedimint-wallet/src/db.rs	`91.66% <0.00%> (-8.34%)`	⬇️
fedimint-server/src/lib.rs	`86.08% <0.00%> (-1.47%)`	⬇️
core/api/src/server.rs	`39.04% <0.00%> (-1.22%)`	⬇️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

rndhouse · 2022-10-09T16:02:56Z

Rebased and the checks all passed.

jkitman · 2022-10-09T17:52:57Z

It also makes it possible to test and ensure key prefixes are locally unique.

Is it possible to add that assertion somewhere, such as during server start?

rndhouse · 2022-10-09T18:22:58Z

@jkitman The description that you've quoted is outdated. The latest commit does not include additional test. elsirion's formulation does not require tests for local (within an enum) uniqueness.

Not sure about testing that each prefix is globally unique. A dbdump integration test could make that global assertion.

NicolaLS

lgtm! but I would remove the strum dependency for now and introduce it in another PR when you actually want to use it.

rndhouse · 2022-10-16T23:40:11Z

@NicolaLS I've removed strum from the commit as you've suggested.

@elsirion I think this should be ready to merge now. The failing CI test is unrelated (as far as I can tell).

This commit reformulates database key prefixes such that they are grouped into enums. Currently, key prefixes are listed as separate const values. This formulation makes it difficult to manage key prefixes (import, iterate, test, etc), which is particularly relevant for dbdump. This commit simplifies key prefix management using enums. Using enums ensures that key prefixes are locally unique.

elsirion · 2022-10-18T21:45:43Z

Awesome, the final result is a really clean PR!

rndhouse force-pushed the enum_db_key_prefixes branch from a9b7631 to 205105e Compare September 26, 2022 15:55

rndhouse mentioned this pull request Sep 26, 2022

Build database inspection tool #331

Closed

rndhouse force-pushed the enum_db_key_prefixes branch 2 times, most recently from f86c252 to 31fa71b Compare September 27, 2022 12:44

dpc reviewed Sep 27, 2022

View reviewed changes

elsirion previously approved these changes Oct 4, 2022

View reviewed changes

rndhouse dismissed elsirion’s stale review via f08f8c3 October 4, 2022 14:30

rndhouse force-pushed the enum_db_key_prefixes branch from 31fa71b to f08f8c3 Compare October 4, 2022 14:30

rndhouse requested a review from elsirion October 4, 2022 14:33

rndhouse force-pushed the enum_db_key_prefixes branch from f08f8c3 to c897034 Compare October 9, 2022 15:42

rndhouse marked this pull request as ready for review October 9, 2022 16:04

NicolaLS previously approved these changes Oct 13, 2022

View reviewed changes

rndhouse dismissed NicolaLS’s stale review via 4c8d483 October 16, 2022 23:22

rndhouse force-pushed the enum_db_key_prefixes branch from c897034 to 4c8d483 Compare October 16, 2022 23:22

justinmoon force-pushed the enum_db_key_prefixes branch from 4c8d483 to 9a7a07a Compare October 18, 2022 11:46

elsirion approved these changes Oct 18, 2022

View reviewed changes

elsirion merged commit ae91bb5 into fedimint:master Oct 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

331: Group database key prefixes as enums. #663

331: Group database key prefixes as enums. #663

rndhouse commented Sep 26, 2022 •

edited

rndhouse commented Sep 26, 2022

justinmoon commented Sep 26, 2022

dpc Sep 27, 2022

rndhouse Sep 27, 2022

elsirion Oct 4, 2022

rndhouse Oct 4, 2022

dpc Sep 27, 2022 •

edited

rndhouse Sep 27, 2022

dpc Sep 27, 2022

rndhouse Sep 27, 2022

rndhouse Sep 27, 2022 •

edited

elsirion Oct 4, 2022

rndhouse Oct 4, 2022

dpc commented Sep 27, 2022 •

edited

rndhouse commented Sep 29, 2022

dpc commented Sep 30, 2022

elsirion left a comment

elsirion Oct 4, 2022

elsirion Oct 4, 2022

rndhouse Oct 4, 2022

elsirion Oct 4, 2022

elsirion Oct 4, 2022

rndhouse commented Oct 4, 2022

codecov-commenter commented Oct 9, 2022 •

edited

rndhouse commented Oct 9, 2022

jkitman commented Oct 9, 2022

rndhouse commented Oct 9, 2022 •

edited

NicolaLS left a comment

rndhouse commented Oct 16, 2022

elsirion commented Oct 18, 2022

331: Group database key prefixes as enums. #663

331: Group database key prefixes as enums. #663

Conversation

rndhouse commented Sep 26, 2022 • edited

rndhouse commented Sep 26, 2022

justinmoon commented Sep 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpc Sep 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rndhouse Sep 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpc commented Sep 27, 2022 • edited

rndhouse commented Sep 29, 2022

dpc commented Sep 30, 2022

elsirion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rndhouse commented Oct 4, 2022

codecov-commenter commented Oct 9, 2022 • edited

Codecov Report

rndhouse commented Oct 9, 2022

jkitman commented Oct 9, 2022

rndhouse commented Oct 9, 2022 • edited

NicolaLS left a comment

Choose a reason for hiding this comment

rndhouse commented Oct 16, 2022

elsirion commented Oct 18, 2022

rndhouse commented Sep 26, 2022 •

edited

dpc Sep 27, 2022 •

edited

rndhouse Sep 27, 2022 •

edited

dpc commented Sep 27, 2022 •

edited

codecov-commenter commented Oct 9, 2022 •

edited

rndhouse commented Oct 9, 2022 •

edited