feat(db): codec encoding/decoding #51

joshieDo · 2022-10-12T09:34:51Z

Summary:

When data is to be stored or read from the database, the type should implement table::Encoding and table::Decoding so it knows how to transform from a structured type to a list of bytes.

However, at the same time we have to care for how we encode this data. Either in a way that helps with sorting (Table::Key) or compactness (Table::Value). That's why we add external codecs such as scale for example.

Having them to be configurable through annotations allows us to contain their implementations/leakage to isolated portions of the project, and allows us to switch between them relatively easily (eg. benchmarking).

old

Added scale encoding, which allowed to remove most of the impl of some basic types.

Added Headers example.

Although, having to put #[codec(compact)] in every integer is kinda of annoying. I was kinda hoping it would do it by default. Another limitation of the lib is that inner types are not getting compacted at all (Vec, Option, etc...)

Added another one called postcard, which seemed to be good on the benchmarks. However, when I tested it with Account... it didn't perform as I expected it. I'd expect the maxsize to be 72 (u64 + 32bytes + 32bytes)... but it's encoding it to 144 bytes.. which is strange. Maybe I'm missing something.

mattsse · 2022-10-12T09:57:05Z

crates/db/src/kv/models/blocks.rs

+/// BlockNumber concatenated with BlockHash. Used as a key for multiple tables. Having the first
+/// element as BlockNumber, helps out with querying/sorting.
+#[derive(Debug)]
+#[allow(non_camel_case_types)]


why do we want non camel case here?

#29 (comment)

I wanted some representation of a concatenation of (BlockNumber, BlockHash, TxNumber), open to change it man_shrugging

imo just using camel case is sufficient

crates/db/src/kv/table.rs

mattsse · 2022-10-12T10:00:49Z

crates/db/src/kv/tables.rs

-#[allow(non_camel_case_types)]
-type BNum_BHash_TxId = Vec<u8>;
-type RlpHeader = Vec<u8>;
+type BlockNumber_BlockHash_TxNumber = Vec<u8>;


gakonst · 2022-10-12T22:14:40Z

crates/db/src/kv/codecs/scale.rs

+where
+    T: ScaleOnly + parity_scale_codec::Decode + Sync + Send + std::fmt::Debug,
+{
+    fn decode(value: bytes::Bytes) -> Result<T, KVError> {


Can we make this take Into<Bytes>?

gakonst · 2022-10-12T22:50:04Z

https://discord.com/channels/900503503162724452/969246749334065172/1029878992142090250

LMK WYT

joshieDo · 2022-10-13T11:22:12Z

crates/primitives/codecs/src/lib.rs

+                    if !path.path.segments.is_empty() {
+                        let _type = format!("{}", path.path.segments[0].ident);
+                        if compactable_types.contains(&_type.as_str()) {
+                            field.attrs.push(syn::parse_quote! {


I'm not sure if there's a better way to check the type

@mattsse may know, but seems fine to me, per L22 above

seems fine for now

crates/db/src/kv/table.rs

gakonst · 2022-10-13T23:21:45Z

crates/db/src/kv/models/blocks.rs

+impl Decode for BlockNumHash {
+    fn decode<B: Into<Bytes>>(value: B) -> Result<Self, KVError> {
+        let value = value.into();
+        Ok(BlockNumHash((u64::decode(value.clone())?, H256::decode(value.slice(8..))?)))


Can avoid the clone here by doing the u64::decode(&value) in a separate line from value.slice? else let's impl decode for refs

the decode object is often provided as mu, &mut BufMut for example.

this way you can take what you want from a buffer and advance it once done.

gakonst · 2022-10-13T23:23:50Z

crates/primitives/src/account.rs


 /// Account saved in database
+#[use_postcard]


should this be #[main_codec]`?

added it just to showcase that both can be used, but yea

gakonst · 2022-10-13T23:24:08Z

crates/primitives/codecs/src/lib.rs

+                    if !path.path.segments.is_empty() {
+                        let _type = format!("{}", path.path.segments[0].ident);
+                        if compactable_types.contains(&_type.as_str()) {
+                            field.attrs.push(syn::parse_quote! {


@mattsse may know, but seems fine to me, per L22 above

crates/db/src/kv/table.rs

gakonst · 2022-10-13T23:28:12Z

crates/db/src/kv/models/blocks.rs

@@ -0,0 +1,58 @@
+//! Block related models and types.


There's also that similar XForStorage types that Akula has, which we maybe could adopt as a pattern

gakonst · 2022-10-13T23:30:21Z

This all looks good as it sets up the infra for encoding/decoding with configurable codec.

Next steps:

Ensure we have the derive everywhere needed
Add missing types
Test ser/de (I'd strongly encourage we use proptest and fuzz-test every single ser/de function)
Benchmarks for ser, de & disk-size for every type, for each codec

Can be done in follow-up PR or any additional small changes you wanna do.

crates/db/src/kv/table.rs

mattsse · 2022-10-14T07:38:35Z

crates/primitives/codecs/src/lib.rs

@@ -0,0 +1,63 @@
+use proc_macro::{self, TokenStream};


let's move this to derive folder which is it's own proc-macro crate

mattsse · 2022-10-14T07:38:53Z

crates/primitives/codecs/src/lib.rs

+                    if !path.path.segments.is_empty() {
+                        let _type = format!("{}", path.path.segments[0].ident);
+                        if compactable_types.contains(&_type.as_str()) {
+                            field.attrs.push(syn::parse_quote! {


seems fine for now

mattsse

As I understood it, this serves as a way to easily switch between codecs which we'll make use of for benchmarks?

once we settled on a codec this will be become obsolete, right?

can we add this to the readme?

crates/codecs/derive/Cargo.toml

gakonst · 2022-10-17T07:11:41Z

crates/db/Cargo.toml

@@ -11,6 +11,12 @@ description = "Staged syncing primitives used in reth."
 # reth
 reth-primitives = { path = "../primitives" }

+# codecs


Shouldn't these be optional?

gakonst · 2022-10-17T07:12:16Z

crates/db/src/kv/codecs/mod.rs

@@ -0,0 +1,2 @@
+mod postcard;


Should these be feature gated?

gakonst · 2022-10-17T07:13:12Z

crates/primitives/Cargo.toml

 reth-rlp = { path = "../common/rlp", features = ["derive"]}
+parity-scale-codec = { version = "3.2.1", features = ["derive", "bytes"] }


Is this necessary?

yes, we're using scale, so when the macro #[main_codec] gets expanded, it expects parity-scale-codec to be reachable

mattsse

lgtm

joshieDo · 2022-10-17T08:02:05Z

couldn't find a straightforward way to include/exclude code & dependencies depending on a downstream dependency feature (primitives -> codecs!) .

the impl code should get optimized out anyway, but yeah it slows down compilation i guess.

if it's okay, i'll look into it another time

joshieDo added 20 commits October 5, 2022 10:49

wip

b77281b

add table macro

2426fbf

add simple put get test with Address

70eb292

add Env.view and Env.update

0159538

docs

14d7c06

slightly change the test

2458373

add initial table initialization and placeholders

5b9dc7c

lint & some

ef76ca2

replace String with str

c130ad3

Merge branch 'main' into joshie/kv_mdbx

a7f2f8f

add error.rs

a36cb71

add docs to encode

940cb87

add docs

3cdc13d

clamp

86c216a

add source on libmdbx_max_page_size

bd5c9b9

Merge branch 'main' into joshie/kv_mdbx

ce9c505

add BlockNumer_BlockHash

7a2bf87

add scale

837ce40

Merge branch 'main' into joshie/scale

d5b0b4d

set header filed to bytes Bytes

2534e1c

joshieDo added the A-db Related to the database label Oct 12, 2022

mattsse reviewed Oct 12, 2022

View reviewed changes

joshieDo added 2 commits October 12, 2022 10:18

remove unwrap

5167017

restrict scale to chosen types

31b06f7

gakonst reviewed Oct 12, 2022

View reviewed changes

joshieDo added 4 commits October 13, 2022 07:06

into bytes

88549df

add postcard

af9614e

Merge branch 'main' into joshie/scale

f892996

changed to BlockNumHash

c8dfdc5

joshieDo force-pushed the joshie/scale branch from 0062f57 to 5b6f783 Compare October 13, 2022 11:05

cleanup

18b4345

joshieDo force-pushed the joshie/scale branch from 5b6f783 to 18b4345 Compare October 13, 2022 11:07

joshieDo added 2 commits October 13, 2022 11:14

seal ScaleOnly

d7e23d6

remove unnecessary dependencies

c96f421

joshieDo commented Oct 13, 2022

View reviewed changes

gakonst reviewed Oct 13, 2022

View reviewed changes

joshieDo added 4 commits October 14, 2022 07:04

properly encode/decode blocknumhash

957fa17

change Account codec to scale

b37cab2

add missing feature to scale

e427d78

add codec to a couple more types

34dc2bb

mattsse requested changes Oct 14, 2022

View reviewed changes

joshieDo added 4 commits October 14, 2022 07:49

silence clippy

58ceb34

add docs about table encoding

0ad054b

move and add reth-codecs

5cb39a3

clippy

6a82be9

joshieDo mentioned this pull request Oct 14, 2022

Tracking: DB #66

Closed

6 tasks

mattsse reviewed Oct 14, 2022

View reviewed changes

crates/codecs/derive/Cargo.toml Show resolved Hide resolved

onbjerg added the C-enhancement New feature or request label Oct 14, 2022

joshieDo added 3 commits October 17, 2022 05:51

make proc-macro visible

e0e5813

add README.md

78863d9

Merge branch 'main' into joshie/scale

bc56452

joshieDo requested a review from mattsse October 17, 2022 06:05

gakonst requested changes Oct 17, 2022

View reviewed changes

mattsse approved these changes Oct 17, 2022

View reviewed changes

gakonst merged commit 063b444 into main Oct 17, 2022

gakonst deleted the joshie/scale branch October 17, 2022 08:04

mattsse mentioned this pull request Oct 17, 2022

feat: impl rlp for receipt #83

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(db): codec encoding/decoding #51

feat(db): codec encoding/decoding #51

joshieDo commented Oct 12, 2022 •

edited

Loading

mattsse Oct 12, 2022

joshieDo Oct 12, 2022 •

edited

Loading

mattsse Oct 12, 2022

mattsse Oct 12, 2022

gakonst Oct 12, 2022

gakonst commented Oct 12, 2022

joshieDo Oct 13, 2022

gakonst Oct 13, 2022

mattsse Oct 14, 2022

gakonst Oct 13, 2022

mattsse Oct 14, 2022

gakonst Oct 13, 2022

joshieDo Oct 14, 2022

gakonst Oct 13, 2022

gakonst Oct 13, 2022

gakonst commented Oct 13, 2022

mattsse Oct 14, 2022

mattsse Oct 14, 2022

mattsse left a comment

gakonst Oct 17, 2022

gakonst Oct 17, 2022

gakonst Oct 17, 2022

joshieDo Oct 17, 2022

mattsse left a comment

joshieDo commented Oct 17, 2022 •

edited

Loading

		reth-rlp = { path = "../common/rlp", features = ["derive"]}
		parity-scale-codec = { version = "3.2.1", features = ["derive", "bytes"] }

feat(db): codec encoding/decoding #51

feat(db): codec encoding/decoding #51

Conversation

joshieDo commented Oct 12, 2022 • edited Loading

Summary:

old

Choose a reason for hiding this comment

joshieDo Oct 12, 2022 • edited Loading

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

gakonst commented Oct 12, 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

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

gakonst commented Oct 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse 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

mattsse left a comment

Choose a reason for hiding this comment

joshieDo commented Oct 17, 2022 • edited Loading

joshieDo commented Oct 12, 2022 •

edited

Loading

joshieDo Oct 12, 2022 •

edited

Loading

joshieDo commented Oct 17, 2022 •

edited

Loading