Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Use DecodeLimit for decoding XCM messages #605

Merged
merged 6 commits into from
Sep 19, 2021
Merged

Conversation

KiChjang
Copy link
Contributor

Fixes paritytech/srlabs_findings#117.

This PR upgrades parity-scale-codec to 2.3.0 (to pick up decode_and_advance_with_depth_limit) and adds an upper limit on the length of the XCM messages during decoding.

@@ -112,8 +115,11 @@ impl<T: Config> DmpMessageHandler for UnlimitedDmpExecution<T> {
let mut used = 0;
for (_sent_at, data) in iter {
let id = sp_io::hashing::twox_64(&data[..]);
let msg = VersionedXcm::<T::Call>::decode(&mut &data[..])
.map(Xcm::<T::Call>::try_from);
let msg = VersionedXcm::<T::Call>::decode_and_advance_with_depth_limit(
Copy link
Member

Choose a reason for hiding this comment

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

Here we don't really want to advance the data? We more want to decode all the data, otherwise it should be an error?

Copy link
Contributor Author

@KiChjang KiChjang Sep 12, 2021

Choose a reason for hiding this comment

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

Ah ok, so decode's default behaviour is to advance the cursor, albeit without any upper limit to the input data length, but if we wanna change the behaviour to "decode everything otherwise error", we can change that too.

pallets/xcm/src/lib.rs Outdated Show resolved Hide resolved
pallets/xcmp-queue/src/lib.rs Outdated Show resolved Hide resolved
pallets/dmp-queue/src/lib.rs Outdated Show resolved Hide resolved
@KiChjang
Copy link
Contributor Author

KiChjang commented Sep 15, 2021

"/tmp/.tmpFzXtH6/chains/local_testnet/db/full" removed.
"/tmp/.tmpFzXtH6/polkadot/chains/westend2/db/full" did not exist.
test purge_chain_works ... FAILED
failures:
---- purge_chain_works stdout ----
thread 'purge_chain_works' panicked at 'assertion failed: !base_path.path().join(\"chains/local_testnet/db\").exists()', polkadot-parachains/tests/purge_chain_works.rs:74:9
stack backtrace:
   0: rust_begin_unwind
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/panicking.rs:493:5
   1: core::panicking::panic_fmt
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/panicking.rs:92:14
   2: core::panicking::panic
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/panicking.rs:50:5
   3: core::ops::function::FnOnce::call_once
   4: core::ops::function::FnOnce::call_once
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
failures:
    purge_chain_works
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 31.05s
error: test failed, to rerun pass '-p polkadot-collator --test purge_chain_works'

Unsure what went wrong with CI here? Retried a couple of times, but the problem still persists.

@bkchr bkchr merged commit 1aab334 into master Sep 19, 2021
@bkchr bkchr deleted the kckyeung/xcm-decode-limit branch September 19, 2021 12:29
@bkchr
Copy link
Member

bkchr commented Sep 19, 2021

The error is known and needs to be fixed. However, it only appears on ci

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants