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

stateful-storage: check hash in signed extension #2137

Merged
merged 7 commits into from
Aug 30, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 5 additions & 4 deletions e2e/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 8 additions & 2 deletions e2e/stateful-pallet-storage/handleItemized.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,10 @@ describe('📗 Stateful Pallet Storage', function () {
add_actions,
0
);
await assert.rejects(itemized_add_result_1.fundAndSend(fundingSource), { name: 'StalePageState' });
await assert.rejects(itemized_add_result_1.signAndSend('current'), {
name: 'RpcError',
message: /1010: Invalid Transaction: Custom error: 9/,
});
});
});

Expand Down Expand Up @@ -285,7 +288,10 @@ describe('📗 Stateful Pallet Storage', function () {
};
const remove_actions = [remove_action];
const op = ExtrinsicHelper.applyItemActions(providerKeys, schemaId_deletable, msa_id, remove_actions, 0);
await assert.rejects(op.fundAndSend(fundingSource), { name: 'StalePageState' });
await assert.rejects(op.signAndSend('current'), {
name: 'RpcError',
message: /1010: Invalid Transaction: Custom error: 9/,
});
});
});

Expand Down
14 changes: 7 additions & 7 deletions e2e/stateful-pallet-storage/handlePaginated.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -201,9 +201,9 @@ describe('📗 Stateful Pallet Storage', function () {
const payload_1 = new Bytes(ExtrinsicHelper.api.registry, 'Hello World From Frequency');

const paginated_add_result_1 = ExtrinsicHelper.upsertPage(providerKeys, schemaId, msa_id, page_id, payload_1, 0);
await assert.rejects(paginated_add_result_1.fundAndSend(fundingSource), {
name: 'StalePageState',
section: 'statefulStorage',
await assert.rejects(paginated_add_result_1.signAndSend('current'), {
name: 'RpcError',
message: /1010: Invalid Transaction: Custom error: 9/,
});
});
});
Expand Down Expand Up @@ -242,10 +242,10 @@ describe('📗 Stateful Pallet Storage', function () {
});

it('🛑 should fail call to remove page with stale target hash', async function () {
const paginated_add_result_1 = ExtrinsicHelper.removePage(providerKeys, schemaId, msa_id, 0, 0);
await assert.rejects(paginated_add_result_1.fundAndSend(fundingSource), {
name: 'StalePageState',
section: 'statefulStorage',
const paginated_add_result_1 = ExtrinsicHelper.removePage(providerKeys, schemaId, msa_id, 0, 1000);
await assert.rejects(paginated_add_result_1.signAndSend('current'), {
name: 'RpcError',
message: /1010: Invalid Transaction: Custom error: 9/,
});
});
});
Expand Down
1 change: 1 addition & 0 deletions node/cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ frequency-service = { package = "frequency-service", path = "../service", defaul
pallet-msa = { package = "pallet-msa", path = "../../pallets/msa", default-features = false }
pallet-frequency-tx-payment = { package = "pallet-frequency-tx-payment", path = "../../pallets/frequency-tx-payment", default-features = false }
pallet-handles = { package = "pallet-handles", path = "../../pallets/handles", default-features = false }
pallet-stateful-storage = { package = "pallet-stateful-storage", path = "../../pallets/stateful-storage", default-features = false }
cli-opt = { default-features = false, path = "../cli-opt" }

# Substrate
Expand Down
11 changes: 7 additions & 4 deletions node/cli/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,10 @@ pub fn create_benchmark_extrinsic(
.unwrap_or(2) as u64;
let extra: runtime::SignedExtra = (
frame_system::CheckNonZeroSender::<runtime::Runtime>::new(),
frame_system::CheckSpecVersion::<runtime::Runtime>::new(),
frame_system::CheckTxVersion::<runtime::Runtime>::new(),
(
frame_system::CheckSpecVersion::<runtime::Runtime>::new(),
frame_system::CheckTxVersion::<runtime::Runtime>::new(),
),
frame_system::CheckGenesis::<runtime::Runtime>::new(),
frame_system::CheckEra::<runtime::Runtime>::from(sp_runtime::generic::Era::mortal(
period,
Expand All @@ -130,6 +132,7 @@ pub fn create_benchmark_extrinsic(
pallet_frequency_tx_payment::ChargeFrqTransactionPayment::<runtime::Runtime>::from(0),
pallet_msa::CheckFreeExtrinsicUse::<runtime::Runtime>::new(),
pallet_handles::handles_signed_extension::HandlesSignedExtension::<runtime::Runtime>::new(),
pallet_stateful_storage::signed_extension::StatefulStorageSignedExtension::<runtime::Runtime>::new(),
frame_metadata_hash_extension::CheckMetadataHash::<runtime::Runtime>::new(false),
cumulus_primitives_storage_weight_reclaim::StorageWeightReclaim::<runtime::Runtime>::new(),
);
Expand All @@ -139,15 +142,15 @@ pub fn create_benchmark_extrinsic(
extra.clone(),
(
(),
runtime::VERSION.spec_version,
runtime::VERSION.transaction_version,
(runtime::VERSION.spec_version, runtime::VERSION.transaction_version),
genesis_hash,
best_hash,
(),
(),
(),
(),
(),
(),
None,
(),
),
Expand Down
5 changes: 3 additions & 2 deletions pallets/stateful-storage/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ use sp_std::prelude::*;
mod stateful_child_tree;
pub mod types;

pub mod signed_extension;
pub mod weights;

use crate::{stateful_child_tree::StatefulChildTree, types::*};
Expand Down Expand Up @@ -939,7 +940,7 @@ impl<T: Config> Pallet<T> {
}

/// Gets a paginated storage for desired parameters
fn get_paginated_page_for(
pub fn get_paginated_page_for(
msa_id: MessageSourceId,
schema_id: SchemaId,
page_id: PageId,
Expand All @@ -955,7 +956,7 @@ impl<T: Config> Pallet<T> {
}

/// Gets an itemized storage for desired parameters
fn get_itemized_page_for(
pub fn get_itemized_page_for(
msa_id: MessageSourceId,
schema_id: SchemaId,
) -> Result<Option<ItemizedPage<T>>, DispatchError> {
Expand Down
200 changes: 200 additions & 0 deletions pallets/stateful-storage/src/signed_extension.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,200 @@
//! Substrate Signed Extension for validating requests to the stateful storage pallet
use crate::{Call, Config, Error, Pallet};
use common_primitives::{
msa::{MessageSourceId, MsaValidator, SchemaId},
stateful_storage::{PageHash, PageId},
};
use core::marker::PhantomData;
use frame_support::{
dispatch::DispatchInfo, ensure, pallet_prelude::ValidTransaction, traits::IsSubType,
};
use parity_scale_codec::{Decode, Encode};
use scale_info::TypeInfo;
use sp_runtime::{
traits::{DispatchInfoOf, Dispatchable, SignedExtension},
transaction_validity::{InvalidTransaction, TransactionValidity, TransactionValidityError},
DispatchError,
};

/// The SignedExtension trait is implemented on CheckFreeExtrinsicUse to validate the request. The
aramikm marked this conversation as resolved.
Show resolved Hide resolved
/// purpose of this is to ensure that the target_hash is verified in transaction pool before getting
/// into block. This is to reduce the chance that capacity consumption due to stale hash
aramikm marked this conversation as resolved.
Show resolved Hide resolved
#[derive(Encode, Decode, Clone, Eq, PartialEq, TypeInfo)]

Check warning on line 22 in pallets/stateful-storage/src/signed_extension.rs

View check run for this annotation

Codecov / codecov/patch

pallets/stateful-storage/src/signed_extension.rs#L22

Added line #L22 was not covered by tests
#[scale_info(skip_type_params(T))]
pub struct StatefulStorageSignedExtension<T: Config + Send + Sync>(PhantomData<T>);

Check warning on line 24 in pallets/stateful-storage/src/signed_extension.rs

View check run for this annotation

Codecov / codecov/patch

pallets/stateful-storage/src/signed_extension.rs#L24

Added line #L24 was not covered by tests

impl<T: Config + Send + Sync> StatefulStorageSignedExtension<T> {
/// Create new `SignedExtension`.
pub fn new() -> Self {
Self(sp_std::marker::PhantomData)
}

/// Verifies the hashes for an Itemized Stateful Storage extrinsic
fn verify_hash_itemized(
msa_id: &MessageSourceId,
schema_id: &SchemaId,
target_hash: &PageHash,
) -> TransactionValidity {
const TAG_PREFIX: &str = "StatefulStorageHashItemized";

if let Ok(Some(page)) = Pallet::<T>::get_itemized_page_for(*msa_id, *schema_id) {
let current_hash: PageHash = page.get_hash();

ensure!(
&current_hash == target_hash,
map_dispatch_error(Error::<T>::StalePageState.into())
);

return ValidTransaction::with_tag_prefix(TAG_PREFIX)
.and_provides((msa_id, schema_id))

Check warning on line 49 in pallets/stateful-storage/src/signed_extension.rs

View check run for this annotation

Codecov / codecov/patch

pallets/stateful-storage/src/signed_extension.rs#L48-L49

Added lines #L48 - L49 were not covered by tests
aramikm marked this conversation as resolved.
Show resolved Hide resolved
.build()
}
Ok(Default::default())

Check warning on line 52 in pallets/stateful-storage/src/signed_extension.rs

View check run for this annotation

Codecov / codecov/patch

pallets/stateful-storage/src/signed_extension.rs#L52

Added line #L52 was not covered by tests
}

/// Verifies the hashes for a Paginated Stateful Storage extrinsic
fn verify_hash_paginated(
msa_id: &MessageSourceId,
schema_id: &SchemaId,
page_id: &PageId,
target_hash: &PageHash,
) -> TransactionValidity {
const TAG_PREFIX: &str = "StatefulStorageHashPaginated";
if let Ok(Some(page)) = Pallet::<T>::get_paginated_page_for(*msa_id, *schema_id, *page_id) {
let current_hash: PageHash = page.get_hash();

ensure!(
&current_hash == target_hash,
map_dispatch_error(Error::<T>::StalePageState.into())
);

return ValidTransaction::with_tag_prefix(TAG_PREFIX)
.and_provides((msa_id, schema_id, page_id))

Check warning on line 72 in pallets/stateful-storage/src/signed_extension.rs

View check run for this annotation

Codecov / codecov/patch

pallets/stateful-storage/src/signed_extension.rs#L71-L72

Added lines #L71 - L72 were not covered by tests
aramikm marked this conversation as resolved.
Show resolved Hide resolved
.build()
}

Ok(Default::default())

Check warning on line 76 in pallets/stateful-storage/src/signed_extension.rs

View check run for this annotation

Codecov / codecov/patch

pallets/stateful-storage/src/signed_extension.rs#L76

Added line #L76 was not covered by tests
}
}

impl<T: Config + Send + Sync> sp_std::fmt::Debug for StatefulStorageSignedExtension<T> {
#[cfg(feature = "std")]
fn fmt(&self, f: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result {
write!(f, "StatefulStorageSignedExtension<{:?}>", self.0)
}

Check warning on line 84 in pallets/stateful-storage/src/signed_extension.rs

View check run for this annotation

Codecov / codecov/patch

pallets/stateful-storage/src/signed_extension.rs#L82-L84

Added lines #L82 - L84 were not covered by tests
#[cfg(not(feature = "std"))]
fn fmt(&self, _: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result {
Ok(())
}
}

/// Map a module DispatchError to an InvalidTransaction::Custom error
pub fn map_dispatch_error(err: DispatchError) -> InvalidTransaction {
aramikm marked this conversation as resolved.
Show resolved Hide resolved
InvalidTransaction::Custom(match err {
DispatchError::Module(module_err) =>
<u32 as Decode>::decode(&mut module_err.error.as_slice())
.unwrap_or_default()
.try_into()
.unwrap_or_default(),
_ => 255u8,
Copy link
Collaborator

@enddynayn enddynayn Aug 23, 2024

Choose a reason for hiding this comment

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

It seems we could end up with an error code that’s not entirely accurate, particularly in the case where zero is returned. Using unwrap_or_default() might not be the best approach here, as falling back to the default value could potentially introduce a bug. That said, I might be overlooking something.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, just a note: the "Custom Error" code that we get as part of the RPC "1010" error is entirely arbitrary, and does not indicate the pallet. So a code of 9 (StalePageHash) is not meaningful to the end user, because they don't know what pallet enum that comes from. I do think somewhere there is an enum of custom RPC error codes for Frequency; perhaps we should map to that instead? Sadly Polkadot/Substrate does not give us a way to propagate anything more than an integer value for custom RPC errors.

Copy link
Collaborator Author

@aramikm aramikm Aug 23, 2024

Choose a reason for hiding this comment

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

@enddynayn The first unwrap_or_default() tries to convert encoded error value (coming from the position of error in the Error enum in the pallet) to u32 which means it can not fail. The second unwrap_or_default() tries to convert u32 to u8 but that would only fail if we have more than 256 errors in the same pallet which is highly unlikely.

@JoeCap08055 Since this is coming from validate it means that after submission you would immediately get that error back. And for any individual extrinsic since the client knows what type of extrinsic they are submitting it is obvious what that error would mean. I have to look into the batch usecase since that would be trickier.

In any case since we only have 256 unique values for different errors we might be able to create a universal custom error enum which would be different between the errors in different pallets but that does not seem to scale based on the total number of pallets and errors. I'm open to suggestions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting approach but works.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably a wider issue than this PR, but the universal enum idea might not be so bad; we wouldn't need to map every error from every pallet; only the ones that can happen in a SignedExtension check. There shouldn't be that many.

Anyway, this approach works for now; maybe it's worth a backlog issue to revisit how we do this across all of our signed extensions, or maybe not.

})
}

impl<T: Config + Send + Sync> SignedExtension for StatefulStorageSignedExtension<T>
where
T::RuntimeCall: Dispatchable<Info = DispatchInfo> + IsSubType<Call<T>>,
{
type AccountId = T::AccountId;
type Call = T::RuntimeCall;
type AdditionalSigned = ();
type Pre = ();
const IDENTIFIER: &'static str = "StatefulStorageSignedExtension";

/// Additional signed
fn additional_signed(&self) -> sp_std::result::Result<(), TransactionValidityError> {
Ok(())
}

Check warning on line 116 in pallets/stateful-storage/src/signed_extension.rs

View check run for this annotation

Codecov / codecov/patch

pallets/stateful-storage/src/signed_extension.rs#L114-L116

Added lines #L114 - L116 were not covered by tests

/// Pre dispatch
fn pre_dispatch(
self,

Check warning on line 120 in pallets/stateful-storage/src/signed_extension.rs

View check run for this annotation

Codecov / codecov/patch

pallets/stateful-storage/src/signed_extension.rs#L119-L120

Added lines #L119 - L120 were not covered by tests
_who: &Self::AccountId,
_call: &Self::Call,
_info: &DispatchInfoOf<Self::Call>,
_len: usize,
) -> Result<Self::Pre, TransactionValidityError> {
// Since we already check the hash in stateful-storage-pallet extrinsics we do not need to
// check the hash before dispatching
Ok(())
}

Check warning on line 129 in pallets/stateful-storage/src/signed_extension.rs

View check run for this annotation

Codecov / codecov/patch

pallets/stateful-storage/src/signed_extension.rs#L128-L129

Added lines #L128 - L129 were not covered by tests

/// Filters stateful extrinsics and verifies the hashes to ensure we are proceeding
aramikm marked this conversation as resolved.
Show resolved Hide resolved
/// with a stale one
fn validate(
&self,
_who: &Self::AccountId,
call: &Self::Call,
_info: &DispatchInfoOf<Self::Call>,
_len: usize,
) -> TransactionValidity {
match call.is_sub_type() {
Some(Call::apply_item_actions {
state_owner_msa_id, schema_id, target_hash, ..
}) => Self::verify_hash_itemized(state_owner_msa_id, schema_id, target_hash),
Some(Call::upsert_page {
state_owner_msa_id, schema_id, target_hash, page_id, ..
}) => Self::verify_hash_paginated(state_owner_msa_id, schema_id, page_id, target_hash),
Some(Call::delete_page {
state_owner_msa_id, schema_id, target_hash, page_id, ..
}) => Self::verify_hash_paginated(state_owner_msa_id, schema_id, page_id, target_hash),
Some(Call::apply_item_actions_with_signature { payload, .. }) =>
Self::verify_hash_itemized(
&payload.msa_id,
&payload.schema_id,
&payload.target_hash,
),
Some(Call::upsert_page_with_signature { payload, .. }) => Self::verify_hash_paginated(
aramikm marked this conversation as resolved.
Show resolved Hide resolved
&payload.msa_id,
&payload.schema_id,
&payload.page_id,
&payload.target_hash,
),
Some(Call::delete_page_with_signature { payload, .. }) => Self::verify_hash_paginated(
&payload.msa_id,
&payload.schema_id,
&payload.page_id,
&payload.target_hash,
),
Some(Call::apply_item_actions_with_signature_v2 { payload, delegator_key, .. }) => {
let state_owner_msa_id = T::MsaInfoProvider::ensure_valid_msa_key(delegator_key)
.map_err(|e| map_dispatch_error(e))?;

Check warning on line 170 in pallets/stateful-storage/src/signed_extension.rs

View check run for this annotation

Codecov / codecov/patch

pallets/stateful-storage/src/signed_extension.rs#L170

Added line #L170 was not covered by tests
Self::verify_hash_itemized(
&state_owner_msa_id,
&payload.schema_id,
&payload.target_hash,
)
},
Some(Call::upsert_page_with_signature_v2 { payload, delegator_key, .. }) => {
let state_owner_msa_id = T::MsaInfoProvider::ensure_valid_msa_key(delegator_key)
.map_err(|e| map_dispatch_error(e))?;
Self::verify_hash_paginated(
&state_owner_msa_id,
&payload.schema_id,
&payload.page_id,
&payload.target_hash,
)
},
Some(Call::delete_page_with_signature_v2 { payload, delegator_key, .. }) => {
let state_owner_msa_id = T::MsaInfoProvider::ensure_valid_msa_key(delegator_key)
.map_err(|e| map_dispatch_error(e))?;

Check warning on line 189 in pallets/stateful-storage/src/signed_extension.rs

View check run for this annotation

Codecov / codecov/patch

pallets/stateful-storage/src/signed_extension.rs#L189

Added line #L189 was not covered by tests
Self::verify_hash_paginated(
&state_owner_msa_id,
&payload.schema_id,
&payload.page_id,
&payload.target_hash,
)
},
_ => Ok(Default::default()),

Check warning on line 197 in pallets/stateful-storage/src/signed_extension.rs

View check run for this annotation

Codecov / codecov/patch

pallets/stateful-storage/src/signed_extension.rs#L197

Added line #L197 was not covered by tests
}
}
aramikm marked this conversation as resolved.
Show resolved Hide resolved
}
1 change: 1 addition & 0 deletions pallets/stateful-storage/src/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ mod child_tree_tests;
mod delete_page_tests;
mod itemized_operations_tests;
mod other_tests;
mod signed_extension_tests;
mod upsert_page_tests;
Loading
Loading