Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
32 changes: 24 additions & 8 deletions core/sr-primitives/src/generic/unchecked_extrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,9 @@ where
fn decode<I: Input>(input: &mut I) -> Option<Self> {
// This is a little more complicated than usual since the binary format must be compatible
// with substrate's generic `Vec<u8>` type. Basically this just means accepting that there
// will be a prefix of u32, which has the total number of bytes following (we don't need
// will be a prefix of vector length (we don't need
// to use this).
let _length_do_not_remove_me_see_above: u32 = Decode::decode(input)?;
let _length_do_not_remove_me_see_above: Vec<()> = Decode::decode(input)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

We are avoiding Compact<u32> here, right?

If so, the problem with Vec<()> is that the generated code is less optimal. It will actually try to check for capacity overflow, increase length and other things to keep semantics of Vec. Consider changing it to Compact.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not matter as long as it does not allocate. I'd prefer not to rely on internal details of how Vec is encoded.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, it shouldn't allocate, but still will iterate n times.

I'd prefer not to rely on internal details of how Vec is encoded.

Fair enough. The fact that it was different has lead to this issue in the first place. However, maybe the test that tests equivalence for some values would be enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've checked that Vec<()> does not do any capacity calculations. Capacity is always reported as usize::max_value() and resize function is just 6 ASM instructions in debug build, 1 inlined instruction in release build.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, interesting. That's actually makes sense, since it's ZST.

My observation was based on checking the wasm build, I wonder why it is generated this way.


Some(UncheckedExtrinsic {
signature: Decode::decode(input)?,
Expand All @@ -125,15 +125,14 @@ where
fn encode(&self) -> Vec<u8> {
let mut v = Vec::new();

// need to prefix with the total length as u32 to ensure it's binary comptible with
// Vec<u8>. we'll make room for it here, then overwrite once we know the length.
v.extend(&[0u8; 4]);

self.signature.encode_to(&mut v);
self.function.encode_to(&mut v);

let length = (v.len() - 4) as u32;
length.using_encoded(|s| v[0..4].copy_from_slice(s));
// need to prefix with the total length to ensure it's binary comptible with
// Vec<u8>.
let mut length: Vec<()> = Vec::new();
length.resize(v.len(), ());
length.using_encoded(|s| { v.splice(0..0, s.iter().cloned()); });
Copy link
Member

Choose a reason for hiding this comment

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

will always need to do a memmove. we could skip this in most cases by a well chosen space reservation as before.


v
}
Expand All @@ -150,3 +149,20 @@ impl<Address, Index, Call, Signature> fmt::Debug for UncheckedExtrinsic<Address,
write!(f, "UncheckedExtrinsic({:?}, {:?})", self.signature.as_ref().map(|x| (&x.0, &x.2)), self.function)
}
}

#[cfg(test)]
mod test {
use codec::{Decode, Encode};
use super::UncheckedExtrinsic;

#[test]
fn encoding_matches_vec() {
type Extrinsic = UncheckedExtrinsic<u32, u32, u32, u32>;
let ex = Extrinsic::new_unsigned(42);
let encoded = ex.encode();
let decoded = Extrinsic::decode(&mut encoded.as_slice()).unwrap();
assert_eq!(decoded, ex);
let as_vec: Vec<u8> = Decode::decode(&mut encoded.as_slice()).unwrap();
assert_eq!(as_vec.encode(), encoded);
}
}
27 changes: 18 additions & 9 deletions core/sr-primitives/src/generic/unchecked_mortal_extrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,9 @@ where
fn decode<I: Input>(input: &mut I) -> Option<Self> {
// This is a little more complicated than usual since the binary format must be compatible
// with substrate's generic `Vec<u8>` type. Basically this just means accepting that there
// will be a prefix of u32, which has the total number of bytes following (we don't need
// will be a prefix of vector length (we don't need
// to use this).
let _length_do_not_remove_me_see_above: u32 = Decode::decode(input)?;
let _length_do_not_remove_me_see_above: Vec<()> = Decode::decode(input)?;

let version = input.read_byte()?;

Expand Down Expand Up @@ -143,10 +143,6 @@ where
fn encode(&self) -> Vec<u8> {
let mut v = Vec::new();

// need to prefix with the total length as u32 to ensure it's binary comptible with
// Vec<u8>. we'll make room for it here, then overwrite once we know the length.
v.extend(&[0u8; 4]);

// 1 byte version id.
match self.signature.as_ref() {
Some(s) => {
Expand All @@ -159,8 +155,11 @@ where
}
self.function.encode_to(&mut v);

let length = (v.len() - 4) as u32;
length.using_encoded(|s| v[0..4].copy_from_slice(s));
// need to prefix with the total length to ensure it's binary comptible with
// Vec<u8>.
let mut length: Vec<()> = Vec::new();
length.resize(v.len(), ());
length.using_encoded(|s| { v.splice(0..0, s.iter().cloned()); });

v
}
Expand Down Expand Up @@ -275,4 +274,14 @@ mod tests {
assert!(ux.is_signed());
assert_eq!(<Ex as Checkable<TestContext>>::check(ux, &TestContext), Err("bad signature in extrinsic"));
}
}

#[test]
fn encoding_matches_vec() {
let ex = Ex::new_unsigned(DUMMY_FUNCTION);
let encoded = ex.encode();
let decoded = Ex::decode(&mut encoded.as_slice()).unwrap();
assert_eq!(decoded, ex);
let as_vec: Vec<u8> = Decode::decode(&mut encoded.as_slice()).unwrap();
assert_eq!(as_vec.encode(), encoded);
}
}
2 changes: 1 addition & 1 deletion node/executor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ mod tests {
construct_block(
2,
block1(false).1,
hex!("703efd5294ae5677c3c975593c64796cce27b471f27936c0cf31fda0a55ede19").into(),
hex!("022372996f312fc57b4649a8e3427a989803652d6d08735f4e4c8510f6b89ff8").into(),
None,
vec![
CheckedExtrinsic {
Expand Down