Skip to content

Commit

Permalink
Follow up work on TransactionExtension - fix weights and clean up `…
Browse files Browse the repository at this point in the history 
…UncheckedExtrinsic` (#6418)

Follow up to #3685
Partially fixes #6403

The main PR introduced bare support for the new extension version byte
as well as extension weights and benchmarking.

This PR:
- Removes the redundant extension version byte from the signed v4
extrinsic, previously unused and defaulted to 0.
- Adds the extension version byte to the inherited implication passed to
`General` transactions.
- Whitelists the `pallet_authorship::Author`, `frame_system::Digest` and
`pallet_transaction_payment::NextFeeMultiplier` storage items as they
are read multiple times by extensions for each transaction, but are hot
in memory and currently overestimate the weight.
- Whitelists the benchmark caller for `CheckEra` and `CheckGenesis` as
the reads are performed for every transaction and overestimate the
weight.
- Updates the umbrella frame weight template to work with the system
extension changes.
- Plans on re-running the benchmarks at least for the `frame_system`
extensions.

---------

Signed-off-by: georgepisaltu <[email protected]>
Co-authored-by: command-bot <>
Co-authored-by: gui <[email protected]>
  • Loading branch information
@georgepisaltu @gui1117
georgepisaltu and gui1117 committed Nov 15, 2024
1 parent f2fd080 commit 8e7bb27
Show file tree
Hide file tree
Showing 102 changed files with 5,593 additions and 5,072 deletions.
12 changes: 8 additions & 4 deletions bridges/bin/runtime-common/src/extensions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -615,6 +615,7 @@ mod tests {
&(),
0,
External,
0,
),
InvalidTransaction::Custom(1)
);
Expand All @@ -623,7 +624,8 @@ mod tests {
42u64.into(),
&MockCall { data: 1 },
&(),
0
0,
0,
),
InvalidTransaction::Custom(1)
);
Expand All @@ -635,6 +637,7 @@ mod tests {
&(),
0,
External,
0,
),
InvalidTransaction::Custom(2)
);
Expand All @@ -643,21 +646,22 @@ mod tests {
42u64.into(),
&MockCall { data: 2 },
&(),
0
0,
0,
),
InvalidTransaction::Custom(2)
);

assert_eq!(
BridgeRejectObsoleteHeadersAndMessages
.validate_only(42u64.into(), &MockCall { data: 3 }, &(), 0, External)
.validate_only(42u64.into(), &MockCall { data: 3 }, &(), 0, External, 0)
.unwrap()
.0,
ValidTransaction { priority: 3, ..Default::default() },
);
assert_eq!(
BridgeRejectObsoleteHeadersAndMessages
.validate_and_prepare(42u64.into(), &MockCall { data: 3 }, &(), 0)
.validate_and_prepare(42u64.into(), &MockCall { data: 3 }, &(), 0, 0)
.unwrap()
.0
.unwrap(),
Expand Down
6 changes: 6 additions & 0 deletions bridges/modules/relayers/src/extension/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1081,6 +1081,7 @@ mod tests {
&DispatchInfo::default(),
0,
External,
0,
)
.map(|t| t.0)
}
Expand All @@ -1094,6 +1095,7 @@ mod tests {
&DispatchInfo::default(),
0,
External,
0,
)
.map(|t| t.0)
}
Expand All @@ -1107,6 +1109,7 @@ mod tests {
&DispatchInfo::default(),
0,
External,
0,
)
.map(|t| t.0)
}
Expand All @@ -1132,6 +1135,7 @@ mod tests {
&call,
&DispatchInfo::default(),
0,
0,
)
.map(|(pre, _)| pre)
}
Expand All @@ -1149,6 +1153,7 @@ mod tests {
&call,
&DispatchInfo::default(),
0,
0,
)
.map(|(pre, _)| pre)
}
Expand All @@ -1166,6 +1171,7 @@ mod tests {
&call,
&DispatchInfo::default(),
0,
0,
)
.map(|(pre, _)| pre)
}
Expand Down
26 changes: 13 additions & 13 deletions cumulus/primitives/storage-weight-reclaim/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ fn basic_refund() {
assert_ok!(CheckWeight::<Test>::do_prepare(&info, LEN, next_len));

let (pre, _) = StorageWeightReclaim::<Test>(PhantomData)
.validate_and_prepare(Some(ALICE.clone()).into(), CALL, &info, LEN)
.validate_and_prepare(Some(ALICE.clone()).into(), CALL, &info, LEN, 0)
.unwrap();
assert_eq!(pre, Some(0));

Expand Down Expand Up @@ -130,7 +130,7 @@ fn underestimating_refund() {
assert_ok!(CheckWeight::<Test>::do_prepare(&info, LEN, next_len));

let (pre, _) = StorageWeightReclaim::<Test>(PhantomData)
.validate_and_prepare(Some(ALICE.clone()).into(), CALL, &info, LEN)
.validate_and_prepare(Some(ALICE.clone()).into(), CALL, &info, LEN, 0)
.unwrap();
assert_eq!(pre, Some(0));

Expand Down Expand Up @@ -168,7 +168,7 @@ fn sets_to_node_storage_proof_if_higher() {
assert_ok!(CheckWeight::<Test>::do_prepare(&info, LEN, next_len));

let (pre, _) = StorageWeightReclaim::<Test>(PhantomData)
.validate_and_prepare(Some(ALICE.clone()).into(), CALL, &info, LEN)
.validate_and_prepare(Some(ALICE.clone()).into(), CALL, &info, LEN, 0)
.unwrap();
assert_eq!(pre, Some(1000));

Expand Down Expand Up @@ -211,7 +211,7 @@ fn sets_to_node_storage_proof_if_higher() {
assert_ok!(CheckWeight::<Test>::do_prepare(&info, LEN, next_len));

let (pre, _) = StorageWeightReclaim::<Test>(PhantomData)
.validate_and_prepare(Some(ALICE.clone()).into(), CALL, &info, LEN)
.validate_and_prepare(Some(ALICE.clone()).into(), CALL, &info, LEN, 0)
.unwrap();
assert_eq!(pre, Some(175));

Expand Down Expand Up @@ -256,7 +256,7 @@ fn does_nothing_without_extension() {
assert_ok!(CheckWeight::<Test>::do_prepare(&info, LEN, next_len));

let (pre, _) = StorageWeightReclaim::<Test>(PhantomData)
.validate_and_prepare(Some(ALICE.clone()).into(), CALL, &info, LEN)
.validate_and_prepare(Some(ALICE.clone()).into(), CALL, &info, LEN, 0)
.unwrap();
assert_eq!(pre, None);

Expand Down Expand Up @@ -288,7 +288,7 @@ fn negative_refund_is_added_to_weight() {
assert_ok!(CheckWeight::<Test>::do_prepare(&info, LEN, next_len));

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

Expand Down Expand Up @@ -321,7 +321,7 @@ fn test_zero_proof_size() {
assert_ok!(CheckWeight::<Test>::do_prepare(&info, LEN, next_len));

let (pre, _) = StorageWeightReclaim::<Test>(PhantomData)
.validate_and_prepare(Some(ALICE.clone()).into(), CALL, &info, LEN)
.validate_and_prepare(Some(ALICE.clone()).into(), CALL, &info, LEN, 0)
.unwrap();
assert_eq!(pre, Some(0));

Expand Down Expand Up @@ -354,7 +354,7 @@ fn test_larger_pre_dispatch_proof_size() {
assert_ok!(CheckWeight::<Test>::do_prepare(&info, LEN, next_len));

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

Expand Down Expand Up @@ -394,7 +394,7 @@ fn test_incorporates_check_weight_unspent_weight() {
assert_ok!(CheckWeight::<Test>::do_prepare(&info, LEN, next_len));

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

Expand Down Expand Up @@ -434,7 +434,7 @@ fn test_incorporates_check_weight_unspent_weight_on_negative() {
assert_ok!(CheckWeight::<Test>::do_prepare(&info, LEN, next_len));

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

Expand Down Expand Up @@ -478,7 +478,7 @@ fn test_nothing_relcaimed() {
assert_eq!(get_storage_weight().total().proof_size(), 250);

let (pre, _) = StorageWeightReclaim::<Test>(PhantomData)
.validate_and_prepare(Some(ALICE.clone()).into(), CALL, &info, LEN)
.validate_and_prepare(Some(ALICE.clone()).into(), CALL, &info, LEN, 0)
.unwrap();
// Should return `setup_test_externalities` proof recorder value: 100.
assert_eq!(pre, Some(0));
Expand Down Expand Up @@ -525,7 +525,7 @@ fn test_incorporates_check_weight_unspent_weight_reverse_order() {
assert_ok!(CheckWeight::<Test>::do_prepare(&info, LEN, next_len));

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

Expand Down Expand Up @@ -567,7 +567,7 @@ fn test_incorporates_check_weight_unspent_weight_on_negative_reverse_order() {
assert_ok!(CheckWeight::<Test>::do_prepare(&info, LEN, next_len));

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

Expand Down
8 changes: 4 additions & 4 deletions polkadot/runtime/common/src/claims.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1078,7 +1078,7 @@ mod tests {
});
let di = c.get_dispatch_info();
assert_eq!(di.pays_fee, Pays::No);
let r = p.validate_only(Some(42).into(), &c, &di, 20, External);
let r = p.validate_only(Some(42).into(), &c, &di, 20, External, 0);
assert_eq!(r.unwrap().0, ValidTransaction::default());
});
}
Expand All @@ -1091,13 +1091,13 @@ mod tests {
statement: StatementKind::Regular.to_text().to_vec(),
});
let di = c.get_dispatch_info();
let r = p.validate_only(Some(42).into(), &c, &di, 20, External);
let r = p.validate_only(Some(42).into(), &c, &di, 20, External, 0);
assert!(r.is_err());
let c = RuntimeCall::Claims(ClaimsCall::attest {
statement: StatementKind::Saft.to_text().to_vec(),
});
let di = c.get_dispatch_info();
let r = p.validate_only(Some(69).into(), &c, &di, 20, External);
let r = p.validate_only(Some(69).into(), &c, &di, 20, External, 0);
assert!(r.is_err());
});
}
Expand Down Expand Up @@ -1739,7 +1739,7 @@ mod benchmarking {
#[block]
{
assert!(ext
.test_run(RawOrigin::Signed(account).into(), &call, &info, 0, |_| {
.test_run(RawOrigin::Signed(account).into(), &call, &info, 0, 0, |_| {
Ok(Default::default())
})
.unwrap()
Expand Down
Loading

0 comments on commit 8e7bb27

Please sign in to comment.