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.

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

v2.0: Supports deserializing accounts lt hash in snapshots (backport of #2994) #3014

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions runtime/src/serde_snapshot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,12 @@ use {
thread::Builder,
},
storage::SerializableStorage,
types::SerdeAccountsLtHash,
};

mod storage;
mod tests;
mod types;
mod utils;

pub(crate) use {
Expand Down Expand Up @@ -419,6 +421,10 @@ where
.map(|(epoch, versioned_epoch_stakes)| (epoch, versioned_epoch_stakes.into())),
);

// This field is only deserialized (and ignored) in this version.
let _accounts_lt_hash: Option<SerdeAccountsLtHash> =
ignore_eof_error(deserialize_from(&mut stream))?;

Ok((bank_fields, accounts_db_fields))
}

Expand Down
48 changes: 48 additions & 0 deletions runtime/src/serde_snapshot/types.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
use {
serde::{de, Deserialize, Deserializer},
std::{fmt, marker::PhantomData},
};

/// Snapshot serde-safe AccountsLtHash
#[cfg_attr(feature = "frozen-abi", derive(AbiExample))]
#[derive(Debug)]
pub struct SerdeAccountsLtHash(
// serde only has array support up to 32 elements; anything larger needs to be handled manually
// see https://github.com/serde-rs/serde/issues/1937 for more information
#[allow(dead_code)] pub [u16; 1024],
);

impl<'de> Deserialize<'de> for SerdeAccountsLtHash {

Choose a reason for hiding this comment

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

yuck. I do not like to review this. @t-nelson thoughts?

Copy link

Choose a reason for hiding this comment

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

if we aren't willing to review logic, we have no business using a dependency that to hide it from us. especially the case for a backport

Choose a reason for hiding this comment

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

in this case the dependency was already used by runtime, just not by accounts-db, as I understand it. So, yes, we can review it, but it sure feels nicer reviewing the simple thing than this. But, you certainly have a point. The fact we're adding a dependency feels more like a code modularization byproduct than it does a new risk.

Copy link

@HaoranYi HaoranYi Oct 1, 2024

Choose a reason for hiding this comment

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

From what I review, "serde_as" procedure macro is already being used for packet class, which is u8[1232]. And here we are just using it again in a similar way for u16[1024]. Therefore, I think it should be fine.

#[serde_as]

Also, looking at the source repo for "serde_as", there is already sufficient tests coverage of "serde_as" for the fixed size array case that we used here.
https://github.com/jonasbb/serde_with/blob/f1b79f27cac7545513c4f14f7c295db2b843cc4f/serde_with/tests/serde_as/lib.rs#L321

Here, instead of using "serde_as", we basically implement just deserialize specially for the u16[1024] by hand, which I have reviewed and believe it is correct. And I trust that books has already tested it manually. But as far as the test for this manually written the code goes, the coverage may not be as good as those tests already in the "serde_as" crate. Beside, with "serde_as" we also get the 'serialize' for free.

Therefore, IMHO, I prefer use "serde_as" in this case (less code). But I am fine with the hand implementation too. They don't make much difference.

fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
struct ArrayVisitor {
element: PhantomData<u16>,
}
impl<'de> de::Visitor<'de> for ArrayVisitor {
type Value = [u16; 1024];
fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
formatter.write_str(concat!("a u16 array of length 1024"))
}
fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
where
A: de::SeqAccess<'de>,
{
let mut arr = [0u16; 1024];
for (i, elem) in arr.iter_mut().enumerate() {
*elem = seq
.next_element()?
.ok_or_else(|| de::Error::invalid_length(i, &self))?;
}
Ok(arr)
}
}

let visitor = ArrayVisitor {
element: PhantomData,
};
let array = deserializer.deserialize_tuple(1024, visitor)?;
Ok(Self(array))
}
}
Loading