Incorrect message for xcm::v4::Assets decoding error

[Dinonard]

Summary

I've been trying to run report_holding benchmarks for xcm-generic, and have been getting a very much misleading error message.

Please check the trace here 👇

``` [2024-08-08T09:26:54Z ERROR xcm::validate_xcm_nesting] Decode error: Error for xcm: V4( Xcm([QueryResponse { query_id: 0, response: Assets(Assets([ Asset { id: AssetId(Location { parents: 0, interior: X1([Parachain(100)]) }), fun: Fungible(1000000000000000000000000) }, Asset { id: AssetId(Location { parents: 0, interior: X2([Parachain(100), GeneralIndex(0)]) }), fun: Fungible(0) }, Asset { id: AssetId(Location { parents: 0, interior: X2([Parachain(100), GeneralIndex(1)]) }), fun: Fungible(1000000000000000000000000) }, Asset { id: AssetId(Location { parents: 0, interior: X2([Parachain(100), GeneralIndex(2)]) }), fun: Fungible(2000000000000000000000000) }, Asset { id: AssetId(Location { parents: 0, interior: X2([Parachain(100), GeneralIndex(3)]) }), fun: Fungible(3000000000000000000000000) }, Asset { id: AssetId(Location { parents: 0, interior: X2([Parachain(100), GeneralIndex(4)]) }), fun: Fungible(4000000000000000000000000) }, Asset { id: AssetId(Location { parents: 0, interior: X2([Parachain(100), GeneralIndex(5)]) }), fun: Fungible(5000000000000000000000000) }, Asset { id: AssetId(Location { parents: 0, interior: X2([Parachain(100), GeneralIndex(6)]) }), fun: Fungible(6000000000000000000000000) }, Asset { id: AssetId(Location { parents: 0, interior: X2([Parachain(100), GeneralIndex(7)]) }), fun: Fungible(7000000000000000000000000) }, Asset { id: AssetId(Location { parents: 0, interior: X2([Parachain(100), GeneralIndex(8)]) }), fun: Fungible(8000000000000000000000000) }, Asset { id: AssetId(Location { parents: 0, interior: X2([Parachain(100), GeneralIndex(9)]) }), fun: Fungible(9000000000000000000000000) }, Asset { id: AssetId(Location { parents: 0, interior: X2([Parachain(100), GeneralIndex(10)]) }), fun: Fungible(10000000000000000000000000) }, Asset { id: AssetId(Location { parents: 0, interior: X2([Parachain(100), GeneralIndex(11)]) }), fun: Fungible(11000000000000000000000000) }, Asset { id: AssetId(Location { parents: 0, interior: X2([Parachain(100), GeneralIndex(12)]) }), fun: Fungible(12000000000000000000000000) }, Asset { id: AssetId(Location { parents: 0, interior: X2([Parachain(100), GeneralIndex(13)]) }), fun: Fungible(13000000000000000000000000) }, Asset { id: AssetId(Location { parents: 0, interior: X2([Parachain(100), GeneralIndex(14)]) }), fun: Fungible(14000000000000000000000000) }, Asset { id: AssetId(Location { parents: 0, interior: X2([Parachain(100), GeneralIndex(15)]) }), fun: Fungible(15000000000000000000000000) }, Asset { id: AssetId(Location { parents: 0, interior: X2([Parachain(100), GeneralIndex(16)]) }), fun: Fungible(16000000000000000000000000) }, Asset { id: AssetId(Location { parents: 0, interior: X2([Parachain(100), GeneralIndex(17)]) }), fun: Fungible(17000000000000000000000000) }, Asset { id: AssetId(Location { parents: 0, interior: X2([Parachain(100), GeneralIndex(18)]) }), fun: Fungible(18000000000000000000000000) }])), max_weight: Weight { ref_time: 18446744073709551615, proof_size: 18446744073709551615 }, querier: Some(Location { parents: 0, interior: X2([Parachain(100), AccountId32 { network: None, id: [15, 52, 233, 32, 143, 235, 70, 176, 225, 195, 109, 117, 158, 189, 8, 134, 172, 115, 186, 128, 84, 20, 122, 150, 24, 165, 38, 245, 135, 43, 131, 177] }]) }) }]))! [2024-08-08T09:26:54Z ERROR staging_xcm_executor] XCM ERROR >> Index: 0, Error: ExceedsMaxMessageSize, Weight: Weight { ref_time: 0, proof_size: 0 } [2024-08-08T09:26:54Z ERROR frame_benchmarking_cli::pallet::command] Error executing and verifying runtime benchmark: xcm executor error: see error logs The following 1 benchmarks failed: - xcm_benchmarks_generic::report_holding Error: Input("1 benchmarks failed") ```

The important thing to note is that error is reported via this line:

[2024-08-08T09:26:54Z ERROR staging_xcm_executor] XCM ERROR >> Index: 0, Error: ExceedsMaxMessageSize, Weight: Weight { ref_time: 0, proof_size: 0 }

However, it's completely misleading 🙂. There are exactly 20 different Asset instances in the Assets instance, which is the limit. The real issue is that the 2nd asset's Fungiblity in the list is 0.

The code handling decoding seems to prohibit 0 as fungibility. I've also noticed some debug asserts that check against 0 as fungibility.

impl Decode for Fungibility {
  fn decode<I: codec::Input>(input: &mut I) -> Result<Self, codec::Error> {
    match UncheckedFungibility::decode(input)? {
      UncheckedFungibility::Fungible(a) if a != 0 => Ok(Self::Fungible(a)),
      UncheckedFungibility::NonFungible(i) => Ok(Self::NonFungible(i)),
      UncheckedFungibility::Fungible(_) => Err("Fungible asset of zero amount is not allowed".into()),
    }
  }
}

Question

I'm not sure what the correct fix is here since I'm not sure why this limitation exists.

1. Do we need to keep the limitation for 0 fungiblity or can it be removed?
2. The proper fix, I assume, would be to properly propagate errors inside the parity-scale-codec since it seems to get lost somewhere.

[bkontur]

@Dinonard Can you please show me your fn worst_case_holding setup? In this PR we added to the ParentAsUmp - yes, that ExceedsMaxMessageSize is coming from that code and we don't have a better error code there:

            versioned_xcm
                .validate_xcm_nesting()
                .map_err(|()| SendError::ExceedsMaxMessageSize)?;

and I also had to fix benchmarks, because they produced invalid XCMs, e.g. 0 value for fungible: https://github.com/paritytech/polkadot-sdk/pull/4236/files#diff-a3b656e8095fe815cf8611c8319aff7b2f32482cdbe7c24ac37a2ce45235f32dR1528

So I would suggest to change/fix your fn worst_case_holding not to use 0 for fungible.

we could improve at least that log, where {e:?} is just Error:

log::error!(target: "xcm::validate_xcm_nesting", "Decode error: {e:?} for xcm: {self:?}!");