Conversation
|
/cmd fmt |
|
/cmd prdoc --audience runtime_dev --bump major |
| let client = TestClientBuilder::default().build(); | ||
|
|
||
| let mut call = RuntimeCall::System(frame_system::Call::remark { remark: vec![1, 2, 3] }); | ||
| for _ in 0..MAX_EXTRINSIC_DEPTH { |
There was a problem hiding this comment.
I think it would be more comprehensive to test that a batch of depth:
MAX_EXTRINSIC_DEPTH - 1MAX_EXTRINSIC_DEPTHMAX_EXTRINSIC_DEPTH + 1
All have the same behavior in authoring than building.
There was a problem hiding this comment.
All have the same behavior in authoring than building.
What you mean by this?
+ 1 is also tested below.
There was a problem hiding this comment.
All have the same behavior in authoring than building.
I also did not understand this; did you mean
All have the same behavior in authoring and building.
?
There was a problem hiding this comment.
Yes, thanks for the correction @rockbmb! As in, if one fails, other fails, and same for if one passes.
This is basically what we had in the security issue: For MAX_EXTRINSIC_DEPTH - 1, one would fail, other would succeed.
There was a problem hiding this comment.
I think it would be more comprehensive to test that a batch of depth:
* `MAX_EXTRINSIC_DEPTH - 1` * `MAX_EXTRINSIC_DEPTH` * `MAX_EXTRINSIC_DEPTH + 1`All have the same behavior in authoring than building.
Maybe to test block.build and import after every iteration :)
Something like?
for _ in 0..MAX_EXTRINSIC_DEPTH {
call = RuntimeCall::Utility(UtilityCall::batch { calls: vec![call] });
// build ok
let ext = ExtrinsicBuilder::new(call).build();
...
assert_ok!(block_builder.build())
// import ok
block_on(client.import(BlockOrigin::Own, block)).unwrap();
}
let call_one_more = RuntimeCall::Utility(UtilityCall::batch { calls: vec![call.clone()] });
// push call_one_more fails
// import ok
block_on(client.import(BlockOrigin::Own, block)).unwrap();
There was a problem hiding this comment.
Maybe to test block.build and import after every iteration :)
If the maximum is working, why should any lower one failing? Makes no sense to me.
There was a problem hiding this comment.
Maybe to test block.build and import after every iteration :)
If the maximum is working, why should any lower one failing? Makes no sense to me.
Yes, I know :), it was just suggestion how to cover Kian's:
* `MAX_EXTRINSIC_DEPTH - 1`
* `MAX_EXTRINSIC_DEPTH`
* `MAX_EXTRINSIC_DEPTH + 1`
There was a problem hiding this comment.
This is basically what we had in the security issue: For
MAX_EXTRINSIC_DEPTH - 1, one would fail, other would succeed.
This is tested here and it was not about MAX_EXTRINSIC_DEPTH - 1. We need to check that MAX_EXTRINSIC_DEPTH works and not more. (which the test is doing)
| let client = TestClientBuilder::default().build(); | ||
|
|
||
| let mut call = RuntimeCall::System(frame_system::Call::remark { remark: vec![1, 2, 3] }); | ||
| for _ in 0..MAX_EXTRINSIC_DEPTH { |
There was a problem hiding this comment.
All have the same behavior in authoring than building.
I also did not understand this; did you mean
All have the same behavior in authoring and building.
?
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
|
/cmd fmt |
|
This pull request has been mentioned on Polkadot Forum. There might be relevant details there: https://forum.polkadot.network/t/2025-05-23-polkadot-sdk-transaction-depth-limit/13248/1 |
This moves the check of the transaction depth limit to `frame-executive` instead of having it hidden in the `sp-api` macros. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* master: (62 commits) release/build-macos-binaries: add missing FEATURES argument (#8816) Add XCM Precompile to `pallet-xcm` (#8693) [Release|CI/CD] Exclude test runtimes from the runtimes build (#8820) Add freebsd sysinfo for telemetry (#7985) release-reusable-rc-build: add optional `features` input that can be considered for nodes building (#8755) [Staking] Cleanups and some improvements (#8701) Fix typos in 3 files in Implementers Guide (#8799) Update `RemoteExporter` docs to reflect removal of `forward_id_for` (#8795) Snowbridge: enforce fee when registering Polkadot native asset (#8725) Bump the ci_dependencies group across 1 directory with 7 updates (#8788) Docker hub 'master' image short sha (#8790) [Release|CI/CD] Combine branch-off and RC automation flows (#8754) Move Transaction depth limit checks (#8750) Add genesis presets for remaining runtimes in polkadot-parachain-bin (#8426) Do not make pallet-identity benchmarks signature-dependent (#8179) Introduction of Approval Slashes [Disabling Strategy Stage 4] (#6827) [AHM] Prepare For Westend Cleanup (#8715) Actually use RP offset in YAP parachain (#8745) [AHM] Relax the requirement for RC-Client to receive +1 session reports (#8702) Don't read storage items in logging (#8749) ...
This moves the check of the transaction depth limit to `frame-executive` instead of having it hidden in the `sp-api` macros. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-project-automation
bot
moved this from Todo
to Done
in [preview] release tracker
6 participants
This moves the check of the transaction depth limit to
frame-executiveinstead of having it hidden in thesp-apimacros.