Skip to content

Commit

Permalink
CheckWeight: account for extrinsic len as proof size (paritytech#4765)
Browse files Browse the repository at this point in the history 
Fix paritytech#4743 which allows
us to remove the defensive limit on pov size in Cumulus after relay
chain gets upgraded with these changes. Also add unit test to ensure
`CheckWeight` - `StorageWeightReclaim` integration works.

TODO:
- [x] PRDoc
- [x] Add a len to all the other tests in storage weight reclaim and
call `CheckWeight::pre_dispatch`

---------

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
  • Loading branch information
@sandreim @TarekkMA
sandreim authored and TarekkMA committed Aug 2, 2024
1 parent d22a564 commit 7f0c6be
Show file tree
Hide file tree
Showing 4 changed files with 205 additions and 175 deletions.
115 changes: 103 additions & 12 deletions cumulus/primitives/storage-weight-reclaim/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ mod tests {
use super::*;
use frame_support::{
assert_ok,
dispatch::DispatchClass,
dispatch::{DispatchClass, PerDispatchClass},
weights::{Weight, WeightMeter},
};
use frame_system::{BlockWeight, CheckWeight};
Expand All @@ -215,7 +215,7 @@ mod tests {
pages: 0u64,
});
const ALICE: AccountId32 = AccountId32::new([1u8; 32]);
const LEN: usize = 0;
const LEN: usize = 150;

pub fn new_test_ext() -> sp_io::TestExternalities {
let ext: sp_io::TestExternalities = cumulus_test_runtime::RuntimeGenesisConfig::default()
Expand Down Expand Up @@ -256,6 +256,10 @@ mod tests {
});
}

fn get_storage_weight() -> PerDispatchClass<Weight> {
BlockWeight::<Test>::get()
}

#[test]
fn basic_refund() {
// The real cost will be 100 bytes of storage size
Expand All @@ -268,6 +272,9 @@ mod tests {
let info = DispatchInfo { weight: Weight::from_parts(0, 500), ..Default::default() };
let post_info = PostDispatchInfo::default();

// Should add 500 + 150 (len) to weight.
assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));

let pre = StorageWeightReclaim::<Test>(PhantomData)
.pre_dispatch(&ALICE, CALL, &info, LEN)
.unwrap();
Expand All @@ -283,7 +290,7 @@ mod tests {
&Ok(())
));

assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 600);
assert_eq!(get_storage_weight().total().proof_size(), 1250);
})
}

Expand All @@ -299,6 +306,9 @@ mod tests {
let info = DispatchInfo { weight: Weight::from_parts(0, 500), ..Default::default() };
let post_info = PostDispatchInfo::default();

// Adds 500 + 150 (len) weight
assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));

let pre = StorageWeightReclaim::<Test>(PhantomData)
.pre_dispatch(&ALICE, CALL, &info, LEN)
.unwrap();
Expand All @@ -313,7 +323,7 @@ mod tests {
&Ok(())
));

assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 1000);
assert_eq!(get_storage_weight().total().proof_size(), 1650);
})
}

Expand All @@ -327,6 +337,9 @@ mod tests {
let info = DispatchInfo { weight: Weight::from_parts(0, 100), ..Default::default() };
let post_info = PostDispatchInfo::default();

// Weight added should be 100 + 150 (len)
assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));

let pre = StorageWeightReclaim::<Test>(PhantomData)
.pre_dispatch(&ALICE, CALL, &info, LEN)
.unwrap();
Expand All @@ -342,7 +355,10 @@ mod tests {
&Ok(())
));

assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 1100);
assert_eq!(
get_storage_weight().total().proof_size(),
1100 + LEN as u64 + info.weight.proof_size()
);
})
}

Expand All @@ -354,6 +370,8 @@ mod tests {
let info = DispatchInfo { weight: Weight::from_parts(0, 500), ..Default::default() };
let post_info = PostDispatchInfo::default();

assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));

let pre = StorageWeightReclaim::<Test>(PhantomData)
.pre_dispatch(&ALICE, CALL, &info, LEN)
.unwrap();
Expand All @@ -368,7 +386,8 @@ mod tests {
&Ok(())
));

assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 0);
// Proof size should be exactly equal to extrinsic length
assert_eq!(get_storage_weight().total().proof_size(), LEN as u64);
});
}

Expand All @@ -382,12 +401,17 @@ mod tests {
let info = DispatchInfo { weight: Weight::from_parts(0, 500), ..Default::default() };
let post_info = PostDispatchInfo::default();

// Adds 500 + 150 (len) weight, total weight is 1950
assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));

let pre = StorageWeightReclaim::<Test>(PhantomData)
.pre_dispatch(&ALICE, CALL, &info, LEN)
.unwrap();
assert_eq!(pre, Some(300));

// Refund 500 unspent weight according to `post_info`, total weight is now 1650
assert_ok!(CheckWeight::<Test>::post_dispatch(None, &info, &post_info, 0, &Ok(())));
// Recorded proof size is negative -200, total weight is now 1450
assert_ok!(StorageWeightReclaim::<Test>::post_dispatch(
Some(pre),
&info,
Expand All @@ -396,7 +420,7 @@ mod tests {
&Ok(())
));

assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 800);
assert_eq!(get_storage_weight().total().proof_size(), 1450);
});
}

Expand All @@ -416,6 +440,9 @@ mod tests {
pays_fee: Default::default(),
};

// Should add 300 + 150 (len) of weight
assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));

let pre = StorageWeightReclaim::<Test>(PhantomData)
.pre_dispatch(&ALICE, CALL, &info, LEN)
.unwrap();
Expand All @@ -432,7 +459,8 @@ mod tests {
&Ok(())
));

assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 900);
// Reclaimed 100
assert_eq!(get_storage_weight().total().proof_size(), 1350);
})
}

Expand All @@ -451,14 +479,66 @@ mod tests {
pays_fee: Default::default(),
};

// Adds 50 + 150 (len) weight, total weight 1200
assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));

let pre = StorageWeightReclaim::<Test>(PhantomData)
.pre_dispatch(&ALICE, CALL, &info, LEN)
.unwrap();
assert_eq!(pre, Some(100));

// The `CheckWeight` extension will refund `actual_weight` from `PostDispatchInfo`
// we always need to call `post_dispatch` to verify that they interoperate correctly.

// Refunds unspent 25 weight according to `post_info`, 1175
assert_ok!(CheckWeight::<Test>::post_dispatch(None, &info, &post_info, 0, &Ok(())));
// Adds 200 - 25 (unspent) == 175 weight, total weight 1350
assert_ok!(StorageWeightReclaim::<Test>::post_dispatch(
Some(pre),
&info,
&post_info,
LEN,
&Ok(())
));

assert_eq!(get_storage_weight().total().proof_size(), 1350);
})
}

#[test]
fn test_nothing_relcaimed() {
let mut test_ext = setup_test_externalities(&[100, 200]);

test_ext.execute_with(|| {
set_current_storage_weight(0);
// Benchmarked storage weight: 100
let info = DispatchInfo { weight: Weight::from_parts(100, 100), ..Default::default() };

// Actual proof size is 100
let post_info = PostDispatchInfo {
actual_weight: Some(Weight::from_parts(50, 100)),
pays_fee: Default::default(),
};

// Adds benchmarked weight 100 + 150 (len), total weight is now 250
assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));

// Weight should go up by 150 len + 100 proof size weight, total weight 250
assert_eq!(get_storage_weight().total().proof_size(), 250);

let pre = StorageWeightReclaim::<Test>(PhantomData)
.pre_dispatch(&ALICE, CALL, &info, LEN)
.unwrap();
// Should return `setup_test_externalities` proof recorder value: 100.
assert_eq!(pre, Some(100));

// The `CheckWeight` extension will refund `actual_weight` from `PostDispatchInfo`
// we always need to call `post_dispatch` to verify that they interoperate correctly.
// Nothing to refund, unspent is 0, total weight 250
assert_ok!(CheckWeight::<Test>::post_dispatch(None, &info, &post_info, LEN, &Ok(())));
// `setup_test_externalities` proof recorder value: 200, so this means the extrinsic
// actually used 100 proof size.
// Nothing to refund or add, weight matches proof recorder
assert_ok!(StorageWeightReclaim::<Test>::post_dispatch(
Some(pre),
&info,
Expand All @@ -467,7 +547,9 @@ mod tests {
&Ok(())
));

assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 1150);
// Check block len weight was not reclaimed:
// 100 weight + 150 extrinsic len == 250 proof size
assert_eq!(get_storage_weight().total().proof_size(), 250);
})
}

Expand All @@ -487,11 +569,15 @@ mod tests {
pays_fee: Default::default(),
};

// Adds 300 + 150 (len) weight, total weight 1450
assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));

let pre = StorageWeightReclaim::<Test>(PhantomData)
.pre_dispatch(&ALICE, CALL, &info, LEN)
.unwrap();
assert_eq!(pre, Some(100));

// This refunds 100 - 50(unspent), total weight is now 1400
assert_ok!(StorageWeightReclaim::<Test>::post_dispatch(
Some(pre),
&info,
Expand All @@ -504,7 +590,8 @@ mod tests {
// we always need to call `post_dispatch` to verify that they interoperate correctly.
assert_ok!(CheckWeight::<Test>::post_dispatch(None, &info, &post_info, 0, &Ok(())));

assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 900);
// Above call refunds 50 (unspent), total weight is 1350 now
assert_eq!(get_storage_weight().total().proof_size(), 1350);
})
}

Expand All @@ -523,11 +610,15 @@ mod tests {
pays_fee: Default::default(),
};

// Adds 50 + 150 (len) weight, total weight is 1200
assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&info, LEN));

let pre = StorageWeightReclaim::<Test>(PhantomData)
.pre_dispatch(&ALICE, CALL, &info, LEN)
.unwrap();
assert_eq!(pre, Some(100));

// Adds additional 150 weight recorded
assert_ok!(StorageWeightReclaim::<Test>::post_dispatch(
Some(pre),
&info,
Expand All @@ -540,7 +631,7 @@ mod tests {
// we always need to call `post_dispatch` to verify that they interoperate correctly.
assert_ok!(CheckWeight::<Test>::post_dispatch(None, &info, &post_info, 0, &Ok(())));

assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 1150);
assert_eq!(get_storage_weight().total().proof_size(), 1350);
})
}

Expand Down Expand Up @@ -644,7 +735,7 @@ mod tests {

// We reclaimed 3 bytes of storage size!
assert_eq!(reclaimed, Some(Weight::from_parts(0, 3)));
assert_eq!(BlockWeight::<Test>::get().total().proof_size(), 10);
assert_eq!(get_storage_weight().total().proof_size(), 10);
assert_eq!(remaining_weight_meter.remaining(), Weight::from_parts(10, 8));
}
}
Expand Down
18 changes: 18 additions & 0 deletions prdoc/pr_4765.prdoc
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
title: CheckWeight - account for extrinsic len as proof size

doc:
- audience: Runtime Dev
description: |
This changes how CheckWeight extension works. It will now account for the extrinsic length
as proof size. When `on_idle` is called, the remaining weight parameter reflects this.

crates:
- name: frame-system
bump: patch
- name: frame-executive
bump: none
- name: cumulus-primitives-storage-weight-reclaim
bump: none



7 changes: 4 additions & 3 deletions substrate/frame/executive/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -649,8 +649,8 @@ fn block_weight_limit_enforced() {
assert!(res.is_ok());
assert_eq!(
<frame_system::Pallet<Runtime>>::block_weight().total(),
//--------------------- on_initialize + block_execution + extrinsic_base weight
Weight::from_parts((encoded_len + 5) * (nonce + 1), 0) + base_block_weight,
//--------------------- on_initialize + block_execution + extrinsic_base weight + extrinsic len
Weight::from_parts((encoded_len + 5) * (nonce + 1), (nonce + 1)* encoded_len) + base_block_weight,
);
assert_eq!(
<frame_system::Pallet<Runtime>>::extrinsic_index(),
Expand Down Expand Up @@ -698,9 +698,10 @@ fn block_weight_and_size_is_stored_per_tx() {
<Runtime as frame_system::Config>::BlockWeights::get()
.get(DispatchClass::Normal)
.base_extrinsic;
// Check we account for all extrinsic weight and their len.
assert_eq!(
<frame_system::Pallet<Runtime>>::block_weight().total(),
base_block_weight + 3u64 * extrinsic_weight,
base_block_weight + 3u64 * extrinsic_weight + 3u64 * Weight::from_parts(0, len as u64),
);
assert_eq!(<frame_system::Pallet<Runtime>>::all_extrinsics_len(), 3 * len);

Expand Down
Loading

0 comments on commit 7f0c6be

Please sign in to comment.