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.

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

chain_getBlock extrinsics encoding #1024

Merged
merged 8 commits into from
Jul 13, 2023
Merged

chain_getBlock extrinsics encoding #1024

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ebd7360
deserialize without decoding
tadeohepperle Jul 10, 2023
eeefa5c
change decoding approach
tadeohepperle Jul 10, 2023
daef05e
Merge branch 'master' into tadeo-hepperle-chain-get-block-extrinsic-d…
tadeohepperle Jul 10, 2023
04f74fd
fix tests
tadeohepperle Jul 11, 2023
1cc8f55
decode without allocating
tadeohepperle Jul 11, 2023
a343001
strip compact prefix
tadeohepperle Jul 11, 2023
d6d502b
cargo fmt
tadeohepperle Jul 11, 2023
743e73a
nit adjustment
tadeohepperle Jul 13, 2023
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
24 changes: 11 additions & 13 deletions subxt/src/blocks/extrinsic_types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ where
} else {
match ExtrinsicDetails::decode_from(
index as u32,
extrinsics[index].0.clone().into(),
&extrinsics[index].0,
client.clone(),
hash,
cached_events.clone(),
Expand Down Expand Up @@ -203,7 +203,7 @@ where
// Attempt to dynamically decode a single extrinsic from the given input.
pub(crate) fn decode_from(
index: u32,
extrinsic_bytes: Arc<[u8]>,
extrinsic_bytes: &[u8],
client: C,
block_hash: T::Hash,
cached_events: CachedEvents<T>,
Expand All @@ -215,6 +215,9 @@ where

let metadata = client.metadata();

// removing the compact encoded prefix:
let extrinsic_bytes: Vec<u8> = Decode::decode(&mut &extrinsic_bytes[..])?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here and in the .position(..) thing below, we could avoid the allocating by instead decoding the Compact<u64> off the front of the bytes and then seeing how many bytes were consumed in doing this.
(I say "avoid allocating" but this was copying the bytes into an Arc before, so either way you may allocate, but at least have the chocie of exactly where)

This "compact length" value (or in other words an "extrinsic_start_index" could be saved if useful in ExtrinsicDetails so that it's easy to get the rest of the bytes from it when needed.

I'd have to look at the code in more detail to see what the Arc was used for before and whether it's then good to keep it, or whether perhaps the Vec<ChainBlockExtrinsic> in Extrinsics could be instead something like Arc<[ChainBlockExtrinsic]> so that we don't allocate at all when we iter() over the extrinsics :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also give the ExtrinsicDetails an explicit lifetime that is bound by the lifetime of Extrinsics. This way we can degrade the bytes field of ExtrinsicDetails to bytes: &[u8] instead of bytes: Arc<[u8]> where the slice points to the i.. bytes of the Vec of the respective ChainBlockExtrinsic, where i is the compact encoded length. This also avoids allocation. I am not sure though if the Arc was necessary somewhere.


// Extrinsic are encoded in memory in the following way:
// - first byte: abbbbbbb (a = 0 for unsigned, 1 for signed, b = version)
// - signature: [unknown TBD with metadata].
Expand Down Expand Up @@ -274,7 +277,7 @@ where

Ok(ExtrinsicDetails {
index,
bytes: extrinsic_bytes,
bytes: extrinsic_bytes.into(),
is_signed,
address_start_idx,
address_end_idx,
Expand Down Expand Up @@ -717,6 +720,7 @@ mod tests {
signed: bool,
name: String,
}

impl StaticExtrinsic for TestCallExtrinsic {
const PALLET: &'static str = "Test";
const CALL: &'static str = "TestCall";
Expand Down Expand Up @@ -782,14 +786,8 @@ mod tests {
let ids = ExtrinsicPartTypeIds::new(&metadata).unwrap();

// Decode with empty bytes.
let result = ExtrinsicDetails::decode_from(
1,
vec![].into(),
client,
H256::random(),
Default::default(),
ids,
);
let result =
ExtrinsicDetails::decode_from(1, &[], client, H256::random(), Default::default(), ids);
assert_matches!(result.err(), Some(crate::Error::Codec(_)));
}

Expand All @@ -802,7 +800,7 @@ mod tests {
// Decode with invalid version.
let result = ExtrinsicDetails::decode_from(
1,
3u8.encode().into(),
&vec![3u8].encode(),
client,
H256::random(),
Default::default(),
Expand Down Expand Up @@ -841,7 +839,7 @@ mod tests {
// The length is handled deserializing `ChainBlockExtrinsic`, therefore the first byte is not needed.
niklasad1 marked this conversation as resolved.
Show resolved Hide resolved
let extrinsic = ExtrinsicDetails::decode_from(
1,
tx_encoded.encoded()[1..].into(),
tx_encoded.encoded(),
client,
H256::random(),
Default::default(),
Expand Down
16 changes: 2 additions & 14 deletions subxt/src/rpc/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,20 +110,8 @@ pub type ConsensusEngineId = [u8; 4];
pub type EncodedJustification = Vec<u8>;

/// Bytes representing an extrinsic in a [`ChainBlock`].
#[derive(Clone, Debug)]
pub struct ChainBlockExtrinsic(pub Vec<u8>);

impl<'a> ::serde::Deserialize<'a> for ChainBlockExtrinsic {
fn deserialize<D>(de: D) -> Result<Self, D::Error>
where
D: ::serde::Deserializer<'a>,
{
let r = impl_serde::serialize::deserialize(de)?;
let bytes = Decode::decode(&mut &r[..])
.map_err(|e| ::serde::de::Error::custom(format!("Decode error: {e}")))?;
Ok(ChainBlockExtrinsic(bytes))
}
}
#[derive(Clone, Debug, Deserialize)]
pub struct ChainBlockExtrinsic(#[serde(with = "impl_serde::serialize")] pub Vec<u8>);

/// Wrapper for NumberOrHex to allow custom From impls
#[derive(Serialize)]
Expand Down
12 changes: 8 additions & 4 deletions subxt/src/tx/tx_progress.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

//! Types representing extrinsics/transactions that have been submitted to a node.

use codec::{Compact, Decode, Encode};
use std::task::Poll;

use crate::{
Expand Down Expand Up @@ -80,7 +81,7 @@ where
TxStatus::InBlock(s) | TxStatus::Finalized(s) => return Ok(s),
// Error scenarios; return the error.
TxStatus::FinalityTimeout(_) => {
return Err(TransactionError::FinalityTimeout.into())
return Err(TransactionError::FinalityTimeout.into());
}
TxStatus::Invalid => return Err(TransactionError::Invalid.into()),
TxStatus::Usurped(_) => return Err(TransactionError::Usurped.into()),
Expand Down Expand Up @@ -109,7 +110,7 @@ where
TxStatus::Finalized(s) => return Ok(s),
// Error scenarios; return the error.
TxStatus::FinalityTimeout(_) => {
return Err(TransactionError::FinalityTimeout.into())
return Err(TransactionError::FinalityTimeout.into());
}
TxStatus::Invalid => return Err(TransactionError::Invalid.into()),
TxStatus::Usurped(_) => return Err(TransactionError::Usurped.into()),
Expand Down Expand Up @@ -260,7 +261,6 @@ impl<T: Config, C: Clone> Stream for TxProgress<T, C> {
/// In any of these cases the client side TxProgress stream is also closed.
/// In those cases the stream is closed however, so you currently have no way to find
/// out if they finally made it into a block or not.

#[derive(Derivative)]
#[derivative(Debug(bound = "C: std::fmt::Debug"))]
pub enum TxStatus<T: Config, C> {
Expand Down Expand Up @@ -389,7 +389,11 @@ impl<T: Config, C: OnlineClientT<T>> TxInBlock<T, C> {
.iter()
.position(|ext| {
use crate::config::Hasher;
let hash = T::Hasher::hash_of(&ext.0);
let Ok(compact) = <Compact<u32>>::decode(&mut &ext.0[..]) else {
return false;
};
let extrinsic_start_index = compact.size_hint();
let hash = T::Hasher::hash_of(&&ext.0[extrinsic_start_index..]);
hash == self.ext_hash
})
// If we successfully obtain the block hash we think contains our
Expand Down