starknet_committer,starknet_patricia: layout-dependent db-key separator

[ArielElp]

No description provided.

[reviewable-StarkWare]

This change is 

[nimrod-starkware]

@nimrod-starkware reviewed 6 files and all commit messages, and made 3 comments.
Reviewable status: 6 of 7 files reviewed, 3 unresolved discussions (waiting on @ArielElp and @yoavGrs).

---

crates/starknet_patricia_storage/src/storage_trait.rs line 209 at r2 (raw file):

/// Returns a `DbKey` from a prefix and a suffix, with a ":" separator.
/// This is used in the facts layout.

I don't think it should be here, the storage should not be aware of layouts.

---

crates/starknet_patricia_storage/src/storage_trait.rs line 210 at r2 (raw file):

/// Returns a `DbKey` from a prefix and a suffix, with a ":" separator.
/// This is used in the facts layout.
pub fn create_db_key(prefix: DbKeyPrefix, suffix: &[u8]) -> DbKey {

non-blocking. If you think it's better please rename in a separate PR

Suggestion:

create_db_key_with_separator

---

crates/starknet_patricia_storage/src/storage_trait.rs line 215 at r2 (raw file):

/// Returns a `DbKey` from a prefix and a suffix, without a separator.
/// This is used in the index layout.

same as above

[yoavGrs]

@yoavGrs reviewed 7 files and all commit messages, and made 5 comments.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ArielElp and @nimrod-starkware).

---

crates/starknet_patricia_storage/src/storage_trait.rs line 210 at r2 (raw file):

Previously, nimrod-starkware wrote…

non-blocking. If you think it's better please rename in a separate PR

+1

---

a discussion (no related file):
Please remove the default implementation of get_db_key for DBObject trait.

---

a discussion (no related file):
Change ForestMetadata for MockForestStorage to use create_db_key_no_separator.

---

crates/starknet_patricia_storage/src/storage_trait.rs line 211 at r2 (raw file):

/// This is used in the facts layout.
pub fn create_db_key(prefix: DbKeyPrefix, suffix: &[u8]) -> DbKey {
    DbKey([prefix.to_bytes().to_vec(), b":".to_vec(), suffix.to_vec()].concat())

Does it work?

Suggestion:

create_db_key_no_separator(prefix, &[b":", suffix].concat())

---

crates/starknet_patricia_storage/src/storage_trait.rs line 217 at r2 (raw file):

/// This is used in the index layout.
pub fn create_db_key_no_separator(prefix: DbKeyPrefix, suffix: &[u8]) -> DbKey {
    DbKey([prefix.to_bytes().to_vec(), suffix.to_vec()].concat())

Does it work?

Suggestion:

DbKey([prefix.to_bytes(), suffix].concat().to_vec())

[ArielElp]

@ArielElp made 7 comments.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs).

---

a discussion (no related file):

Previously, yoavGrs wrote…

Please remove the default implementation of get_db_key for DBObject trait.

Trying it a different way: one utility function that expects separator, defaultl behavior of get_db_key is empty separator, overridden in facts layout.

---

a discussion (no related file):

Previously, yoavGrs wrote…

Change ForestMetadata for MockForestStorage to use create_db_key_no_separator.

Done.

---

crates/starknet_patricia_storage/src/storage_trait.rs line 209 at r2 (raw file):

Previously, nimrod-starkware wrote…

I don't think it should be here, the storage should not be aware of layouts.

Done (see top comment to Yoav, this function expects separator now, no layout mentioned)

---

crates/starknet_patricia_storage/src/storage_trait.rs line 210 at r2 (raw file):

Previously, yoavGrs wrote…

+1

Only one function now

---

crates/starknet_patricia_storage/src/storage_trait.rs line 211 at r2 (raw file):

Previously, yoavGrs wrote…

Does it work?

Done.

---

crates/starknet_patricia_storage/src/storage_trait.rs line 215 at r2 (raw file):

Previously, nimrod-starkware wrote…

same as above

Done.

---

crates/starknet_patricia_storage/src/storage_trait.rs line 217 at r2 (raw file):

Previously, yoavGrs wrote…

Does it work?

Function removed

[nimrod-starkware]

@nimrod-starkware reviewed 6 files, made 1 comment, and resolved 3 discussions.
Reviewable status: 9 of 12 files reviewed, 4 unresolved discussions (waiting on @yoavGrs).

[yoavGrs]

@yoavGrs reviewed 6 files and all commit messages, made 3 comments, and resolved 4 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArielElp).

---

crates/starknet_committer/src/db/facts_db/types.rs line 56 at r3 (raw file):

        };
        let suffix = self.root_hash.0.to_bytes_be();
        create_db_key(prefix, b":", &suffix)

Define a constant FactsDbSeparator = b":".

Code quote:

b":"

---

crates/starknet_patricia_storage/src/db_object.rs line 52 at r3 (raw file):

    /// Returns a [DbKey] from a prefix and a suffix.
    fn get_db_key(&self, key_context: &Self::KeyContext, suffix: &[u8]) -> DbKey {
        create_db_key(self.get_prefix(key_context), &[], suffix)

Define a constant NoDbSeparator = b"".

Code quote:

&[],

---

crates/starknet_committer/src/patricia_merkle_tree/leaf/leaf_serde.rs line 55 at r3 (raw file):

    fn get_db_key(&self, key_context: &Self::KeyContext, suffix: &[u8]) -> DbKey {
        create_db_key(self.get_prefix(key_context), b":", suffix)
    }

Index layer wraps StarknetStorageValue and ignores this method.

Maybe the right thing was to define

pub struct FactLayoutStarknetStorageValue(pub StarknetStorageValue);

If you agree, please add TODO for it.

Code quote:

    fn get_db_key(&self, key_context: &Self::KeyContext, suffix: &[u8]) -> DbKey {
        create_db_key(self.get_prefix(key_context), b":", suffix)
    }

[yoavGrs]

@yoavGrs made 1 comment.
Reviewable status: 11 of 12 files reviewed, 3 unresolved discussions (waiting on @ArielElp and @nimrod-starkware).

---

crates/starknet_committer/src/db/facts_db/types.rs line 56 at r3 (raw file):

Previously, yoavGrs wrote…

Define a constant FactsDbSeparator = b":".

And maybe fn create_facts_db_key as well.

[ArielElp]

@ArielElp made 3 comments.
Reviewable status: 7 of 12 files reviewed, 3 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs).

---

crates/starknet_committer/src/db/facts_db/types.rs line 56 at r3 (raw file):

Previously, yoavGrs wrote…

And maybe fn create_facts_db_key as well.

Done. (the constants, not separate function)

---

crates/starknet_committer/src/patricia_merkle_tree/leaf/leaf_serde.rs line 55 at r3 (raw file):

Previously, yoavGrs wrote…

Index layer wraps StarknetStorageValue and ignores this method.

Maybe the right thing was to define

pub struct FactLayoutStarknetStorageValue(pub StarknetStorageValue);

If you agree, please add TODO for it.

I don't follow, we want this method ignored, we have a separate DBObject implementation.

Unrelated to this PR, it may have made things slightly more clear if we separate the "base" leaf types from the facts layout db types (who happen to be the same), but not worth it IMO.

---

crates/starknet_patricia_storage/src/db_object.rs line 52 at r3 (raw file):

Previously, yoavGrs wrote…

Define a constant NoDbSeparator = b"".

Done. (different name)

[yoavGrs]

@yoavGrs reviewed 5 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp).

---

crates/starknet_committer/src/patricia_merkle_tree/leaf/leaf_serde.rs line 55 at r3 (raw file):

Previously, ArielElp wrote…

I don't follow, we want this method ignored, we have a separate DBObject implementation.

Unrelated to this PR, it may have made things slightly more clear if we separate the "base" leaf types from the facts layout db types (who happen to be the same), but not worth it IMO.

I suggested creating a base leaf.
Anyway, that's not going to happen anytime soon.

[dorimedini-starkware]

@dorimedini-starkware reviewed 12 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArielElp and @yoavGrs).

---

crates/starknet_committer/src/patricia_merkle_tree/leaf/leaf_serde.rs line 55 at r3 (raw file):

Previously, yoavGrs wrote…

I suggested creating a base leaf.
Anyway, that's not going to happen anytime soon.

index layout wraps leaves, but facts layout uses the "base" leaves?
i.e. index layout wraps fact-y leaves and changes behavior?
a bit confusing, no?
suggestion (not in this PR!):

1. the DBObject get_db_key trait function definition should include a separator argument
2. seems like it can have a default implementation after (1), right? no need to impl 3 times here
3. separator should be defined per layout (const method? associated const, if such a thing exists?)

---

crates/starknet_patricia_storage/src/db_object.rs line 52 at r3 (raw file):

Previously, ArielElp wrote…

Done. (different name)

why do you have a specific implementation (implementation that doesn't fit all trait implementors) on the trait itself?
I would remove the default impl here, and reinstate it once the separator is a trait-associated-value (or something equivalent) of DBObject trait

---

crates/starknet_patricia/src/patricia_merkle_tree/traversal_test.rs line 52 at r5 (raw file):

    ) -> DbKey {
        // Dummy key for testing purposes (we only need a key when interacting with storage).
        DbKey(TEST_PREFIX.to_vec())

no longer equivalent, right? previously the DBKey had an extra ":"
doesn't matter though, better this way

Code quote:

DbKey(TEST_PREFIX.to_vec())

---

crates/starknet_committer/src/db/index_db/types.rs line 184 at r5 (raw file):

        let prefix = L::get_static_prefix(key_context);
        let suffix = self.root_index.0.to_be_bytes();
        create_db_key(prefix, &[], &suffix)

1. make it explicit
2. we may want to change this in the future, so no hard-coded literals

const INDEX_DB_KEY_SEPARATOR: &[u8] = b"";

Suggestion:

create_db_key(prefix, INDEX_DB_KEY_SEPARATOR, &suffix)

[ArielElp]

@ArielElp made 3 comments.
Reviewable status: 1 of 12 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware, @nimrod-starkware, and @yoavGrs).

---

crates/starknet_committer/src/db/index_db/types.rs line 184 at r5 (raw file):

Previously, dorimedini-starkware wrote…

1. make it explicit
2. we may want to change this in the future, so no hard-coded literals

const INDEX_DB_KEY_SEPARATOR: &[u8] = b"";

Done (there's already EMPTY_DB_KEY_SEPARATOR, made it pub)

---

crates/starknet_committer/src/patricia_merkle_tree/leaf/leaf_serde.rs line 55 at r3 (raw file):

Previously, dorimedini-starkware wrote…

index layout wraps leaves, but facts layout uses the "base" leaves?
i.e. index layout wraps fact-y leaves and changes behavior?
a bit confusing, no?
suggestion (not in this PR!):

1. the DBObject get_db_key trait function definition should include a separator argument
2. seems like it can have a default implementation after (1), right? no need to impl 3 times here
3. separator should be defined per layout (const method? associated const, if such a thing exists?)

Agree on it being a bit confusing. We can have separate wrappers at the end of the stack, though it's unrelated to the question ATM.

Re 1-3, an associated const on DBObject itself seems like the way to go. DBObject impl is clearly layout-dependent, and that way we don't worry about propogating it from e.g. Layout all the way to create_db_key.

TLDR of the changes now:

1. Still two arguments
2. We do have a default impl now that fits everyone
3. Associated const on DBObject

---

crates/starknet_patricia_storage/src/db_object.rs line 52 at r3 (raw file):

Previously, dorimedini-starkware wrote…

why do you have a specific implementation (implementation that doesn't fit all trait implementors) on the trait itself?
I would remove the default impl here, and reinstate it once the separator is a trait-associated-value (or something equivalent) of DBObject trait

Irrelevant given the discussion above.