8000 Deserialization of a node in a freed area probably produces the wrong error · Issue #831 · ava-labs/firewood · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Deserialization of a node in a freed area probably produces the wrong error #831

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
Open
rkuris opened this issue Mar 27, 2025 · 1 comment
Open

Deserialization of a node in a freed area probably produces the wrong error #831

rkuris opened this issue Mar 27, 2025 · 1 comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed rust Pull requests that update rust code

Comments

@rkuris
Copy link
Collaborator
rkuris commented Mar 27, 2025

The problem here is that we currently deserialize StoredArea<Area<Node, FreeArea>> but only when it's expted to be a freed area. If there happens to be a node on a free list, bad things are likely to happen since we'll deserialize the node using serde.

The right fix is to remove serde everywhere.

We're not using it in many places any more, and it's clearly slower. Although it's in Cargo.toml for the firewood subdirectory, it isn't referenced anywhere in that directory. Only storage is currently using it, and it's only being used to serialize some stuff related to free space management, such as this one and another for AreaIndex.

This would also remove a lot of the benchmarks related to serde serialization timings, which show serde is almost always slower.

Bonus points if you can figure out why manual serialization of a full node is a tiny bit slower than serde.

     Running benches/serializer.rs (/Users/rkuris/open-source/firewood/target/release/deps/serializer-6c232d65bd4e75a9)
leaf/serde              time:   [69.045 ns 69.287 ns 69.553 ns]
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild
leaf/manual             time:   [39.281 ns 39.360 ns 39.450 ns]
Found 9 outliers among 100 measurements (9.00%)
  9 (9.00%) high mild

has_value/serde         time:   [239.35 ns 243.83 ns 247.95 ns]
has_value/manual        time:   [80.964 ns 81.715 ns 83.037 ns]
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) high mild
  1 (1.00%) high severe

1_child/serde           time:   [153.20 ns 154.21 ns 155.28 ns]
1_child/manual          time:   [73.337 ns 73.438 ns 73.569 ns]
Found 14 outliers among 100 measurements (14.00%)
  7 (7.00%) high mild
  7 (7.00%) high severe

2_child/serde           time:   [246.32 ns 251.62 ns 256.86 ns]
2_child/manual          time:   [106.19 ns 106.60 ns 107.05 ns]
Found 5 outliers among 100 measurements (5.00%)
  5 (5.00%) high mild

16_child/serde          time:   [483.69 ns 494.48 ns 504.73 ns]
16_child/manual         time:   [502.68 ns 505.68 ns 509.86 ns]
Found 4 outliers among 100 measurements (4.00%)
  1 (1.00%) high mild
  3 (3.00%) high severe
@rkuris rkuris added bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed rust Pull requests that update rust code labels Mar 27, 2025
@rkuris rkuris mentioned this issue Mar 27, 2025
Draft
@rkuris
Copy link
Collaborator Author
rkuris commented Mar 27, 2025

git grep bincode points to all the places that require new code:

firewood/Cargo.toml:bincode = "1.3.3"
storage/Cargo.toml:bincode = "1.3.3"
storage/benches/serializer.rs:use bincode::Options;
storage/benches/serializer.rs:    let serializer = bincode::DefaultOptions::new().with_varint_encoding();
storage/benches/serializer.rs:    let serializer = bincode::DefaultOptions::new().with_varint_encoding();
storage/src/nodestore.rs:use bincode::{DefaultOptions, Options as _};
storage/src/nodestore.rs:fn serializer() -> impl bincode::Options {

git grep '\(Des\|S\)erialize' points to all the places you can remove code, as you'll no longer need to derive or implement serialization using serde for all these structures:

grpc-testtool/proto/sync/sync.proto:  SerializedPath key = 1;
grpc-testtool/proto/sync/sync.proto:message SerializedPath {
grpc-testtool/src/bin/process-server.rs:use serde::Deserialize;
grpc-testtool/src/bin/process-server.rs:#[derive(Clone, Debug, Deserialize)]
storage/src/node/branch.rs:use serde::ser::SerializeStruct as _;
storage/src/node/branch.rs:use serde::{Deserialize, Serialize};
storage/src/node/branch.rs:    use serde::{Deserialize, Serialize};
storage/src/node/branch.rs:    #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
storage/src/node/branch.rs:impl Serialize for BranchNode {
storage/src/node/branch.rs:        S: serde::Serializer,
storage/src/node/branch.rs:impl<'de> Deserialize<'de> for BranchNode {
storage/src/node/branch.rs:        D: serde::Deserializer<'de>,
storage/src/node/branch.rs:        #[derive(Deserialize)]
storage/src/node/branch.rs:        struct SerializedBranchNode {
storage/src/node/branch.rs:        let s: SerializedBranchNode = Deserialize::deserialize(deserializer)?;
storage/src/node/leaf.rs:use serde::{Deserialize, Serialize};
storage/src/node/leaf.rs:#[derive(PartialEq, Eq, Clone, Serialize, Deserialize)]
storage/src/node/mod.rs:use serde::{Deserialize, Serialize};
storage/src/node/mod.rs:#[derive(PartialEq, Eq, Clone, Debug, EnumAsInner, Serialize, Deserialize)]
storage/src/node/path.rs:use serde::{Deserialize, Serialize};
storage/src/node/path.rs:#[derive(PartialEq, Eq, Clone, Serialize, Deserialize)]
storage/src/nodestore.rs:use serde::{Deserialize, Serialize};
storage/src/nodestore.rs:#[derive(PartialEq, Eq, Clone, Debug, Deserialize, Serialize)]
storage/src/nodestore.rs:#[derive(PartialEq, Eq, Clone, Debug, Deserialize, Serialize)]
storage/src/nodestore.rs:#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize, Serialize, NoUninit, AnyBitPattern)]
storage/src/nodestore.rs:#[derive(Copy, Debug, PartialEq, Eq, Serialize, Deserialize, Clone, NoUninit, AnyBitPattern)]
storage/src/nodestore.rs:#[derive(Debug, PartialEq, Eq, Clone, Copy, Serialize, Deserialize)]
storage/src/trie_hash.rs:use serde::{Deserialize, Serialize};
storage/src/trie_hash.rs:impl Serialize for TrieHash {
storage/src/trie_hash.rs:        S: serde::Serializer,
storage/src/trie_hash.rs:impl<'de> Deserialize<'de> for TrieHash {
storage/src/trie_hash.rs:        D: serde::Deserializer<'de>,

grpc-testtool might still want or need serde, or this can be done in another ticket, although it looks like it's not used much there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed rust Pull requests that update rust code
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@rkuris @aaronbuchwald
0