Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Fixed extrinsic encoding #794

Merged
merged 4 commits into from
Sep 25, 2018
Merged
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
25 changes: 25 additions & 0 deletions core/sr-primitives/src/generic/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,28 @@ pub use self::checked_extrinsic::CheckedExtrinsic;
pub use self::header::Header;
pub use self::block::{Block, SignedBlock, BlockId};
pub use self::digest::{Digest, DigestItem, DigestItemRef};

use codec::Encode;
use rstd::prelude::*;

fn encode_with_vec_prefix<T: Encode, F: Fn(&mut Vec<u8>)>(encoder: F) -> Vec<u8> {
let size = ::rstd::mem::size_of::<T>();
let reserve = match size {
0...0b00111111 => 1,
0...0b00111111_11111111 => 2,
_ => 4,
};
let mut v = Vec::with_capacity(reserve + size);
v.resize(reserve, 0);
encoder(&mut v);

// 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() - reserve, ());
length.using_encoded(|s| {
v.splice(0..reserve, s.iter().cloned());
});

v
}
38 changes: 23 additions & 15 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 @@ -123,19 +123,10 @@ where
Call: Encode,
{
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));

v
super::encode_with_vec_prefix::<Self, _>(|v| {
self.signature.encode_to(v);
self.function.encode_to(v);
})
}
}

Expand All @@ -150,3 +141,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);
}
}
49 changes: 25 additions & 24 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 All @@ -141,28 +141,19 @@ where
Call: Encode,
{
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) => {
v.push(TRANSACTION_VERSION | 0b1000_0000);
s.encode_to(&mut v);
}
None => {
v.push(TRANSACTION_VERSION & 0b0111_1111);
super::encode_with_vec_prefix::<Self, _>(|v| {
// 1 byte version id.
match self.signature.as_ref() {
Some(s) => {
v.push(TRANSACTION_VERSION | 0b1000_0000);
s.encode_to(v);
}
None => {
v.push(TRANSACTION_VERSION & 0b0111_1111);
}
}
}
self.function.encode_to(&mut v);

let length = (v.len() - 4) as u32;
length.using_encoded(|s| v[0..4].copy_from_slice(s));

v
self.function.encode_to(v);
})
}
}

Expand Down Expand Up @@ -275,4 +266,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!("e312f8e2111124c57448050aed74c625382ed222f48ab55ada41afacd0a65fa6").into(),
hex!("0f107d73773b84409de8a444dd0d39e6d90b18b3ce393b379a4f6bf34226aa0a").into(),
None,
vec![
CheckedExtrinsic {
Expand Down