paritytech / xcm-docs

Documentation for XCM
12 stars 4 forks source link

Example #4: can't make it fail #41

Closed kirillt closed 10 months ago

kirillt commented 10 months ago

Hey guys, thanks for the great docs. So far, it seems to be the most clear introduction into XCM.

I've especially liked that examples folder provides straightforward sequence of unit tests to play with XCM.

However, I've stumbled upon 4_origins example:

let message = Xcm(vec![
    WithdrawAsset((Here, AMOUNT).into()),
    BuyExecution { fees: (Here, AMOUNT).into(), weight_limit: WeightLimit::Unlimited },
    // Set the instructions that are executed when ExpectOrigin does not pass.
    // In this case, reporting back an error to the Parachain.
    SetErrorHandler(Xcm(vec![ReportError(QueryResponseInfo {
        destination: Parachain(1).into(),
        query_id: QUERY_ID,
        max_weight: Weight::from_all(0),
    })])),
    DescendOrigin((PalletInstance(1)).into()),
    // Checks if the XcmContext origin descended to `Parachain(1)/PalletInstance(1)`.
    ExpectOrigin(Some((Parachain(1), PalletInstance(1)).into())),
]);

This test looks logic and passes, of course. But how to make it fail?

What I tried:

But the test is still passing! This makes me feel not confident in xcm-simulator... Is it a bug somewhere or am I doing something wrong?

kirillt commented 10 months ago

Alright, so tracing this test case revealed that it fails silently:

TRACE xcm::execute: !!! ERROR: TooExpensive    
TRACE xcm::execute_xcm_in_credit: result: Err(ExecutorError { index: 1, xcm_error: TooExpensive, weight: Weight { ref_time: 4000, proof_size: 4000 } })    
TRACE xcm::weight: FixedRateOfFungible::refund_weight weight: Weight { ref_time: 4000, proof_size: 4000 }    
TRACE xcm::execute_xcm_in_credit: Execution errored at 1: TooExpensive (original_origin: MultiLocation { parents: 0, interior: X1(Parachain(1)) })

Is there a method to assert that result of remote XCM execution was OK?

We can do this: assert_ok!(ParachainPalletXcm::execute(...)). And we can do this: assert_ok!(ParachainPalletXcm::send_xcm(...))

But how to verify that execution of the message succeeded on the receiver side?

KiChjang commented 10 months ago

Is there a method to assert that result of remote XCM execution was OK?

This is impossible, as you've read from the XCM docs that XCM is built to be asynchronous. The sender is never going to know whether the recipient has executed the XCM successfully -- that's the "fire-and-forget" paradigm. Once an XCM reaches its destination, the recipient executes and interprets the message all according to its preferences.

kirillt commented 10 months ago

@KiChjang Yes, that's inherent from XCM design. But probably we could provide such a method in the testing setup?

Can we introspect the state of XCM Executor? I'm not very knowledgeable of XCM internals yet, probably we can assert on content of the Error register? Something like this should be perfect:

ParaA::execute_with(|| {
    assert_eq!(parachain::XcmState::error_register(), None)
});

Otherwise, we can't rely on the unit tests. For instance, this example 4_origins shows green but it's not correct right now — something changed around weights and const AMOUNT: u128 = 10 is not enough now. I made it 1_000_000 and observing this output now:

TRACE xcm::execute_xcm_in_credit: result: Ok(())    
TRACE xcm::weight: FixedRateOfFungible::refund_weight weight: Weight { ref_time: 1000, proof_size: 1000 }    
TRACE xcm::execute_xcm_in_credit: Trapping assets in holding register: Assets { fungible: {Concrete(MultiLocation { parents: 0, interior: Here }): 990000}, non_fungible: {} }, context: XcmContext { origin: Some(MultiLocation { parents: 0, interior: X2(Parachain(1), PalletInstance(1)) }), message_hash: [223, 3, 22, 10, 94, 199, 201, 35, 68, 16, 17, 145, 140, 1, 98, 202, 237, 151, 238, 219, 240, 200, 202, 228, 56, 71, 111, 57, 38, 63, 2, 133], topic: None } (original_origin: MultiLocation { parents: 0, interior: X1(Parachain(1)) })
kirillt commented 10 months ago

With increased fee I can fail the test case pretty easily:

thread 'origins::tests::descend_origin' panicked at 'assertion failed: `(left == right)`
  left: `[Xcm([QueryResponse { query_id: 1234, response: ExecutionResult(Some((4, ExpectationFalse))), max_weight: Weight { ref_time: 0, proof_size: 0 }, querier: Some(MultiLocation { parents: 0, interior: X1(PalletInstance(1)) }) }])]`,
 right: `[]`', src/4_origins/mod.rs:38:13

So is it only BuyExecution instruction that can silently fail? When there is not enough weight, parachain::MsgQueue::received_dmp() is empty. We need another assertion to support this kind of failures.

KiChjang commented 10 months ago

Let's be clear what the problem is -- the test itself only has 2 assert statements: one for send_xcm and the other for received_dmp. You are seeing the test passing because the only thing that would make send_xcm fail is if the transport layer fails to send the XCM; it does not check whether or not the execution of the XCM succeeded.

However, in the XCM program, there is a ReportError instruction, which allows the recipient to send an error response back whenever the XCM execution on the recipient throws an XCM error. This is what the 2nd assertion does -- check whether ParaA received any reports of errors, and is also what you're seeing on your assertion failure message.

When you don't have enough fees for BuyExecution, the AllowTopLevelPaidExecution barrier should have rejected the execution of your XCM program, and so it was not able to even get to the stage where it processes ReportError. Given that's the case, let's first check whether this scenario is true; i.e. do you see a TRACE message that starts with something like TRACE xcm::execute_xcm_in_credit: Barrier blocked execution!?

kirillt commented 10 months ago

@KiChjang I don't see this message in the log (commit df4f3b603eeed9c72a37adcdcc11d499654324f2, no changes from me).

Here is complete trace output, lines not containing xcm word are filtered out.

DEBUG xcm::send_xcm: dest: MultiLocation { parents: 1, interior: Here }, message: Xcm([WithdrawAsset(MultiAssets([MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: Here }), fun: Fungible(10) }])), BuyExecution { fees: MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: Here }), fun: Fungible(10) }, weight_limit: Unlimited }, SetErrorHandler(Xcm([ReportError(QueryResponseInfo { destination: MultiLocation { parents: 0, interior: X1(Parachain(1)) }, query_id: 1234, max_weight: Weight { ref_time: 0, proof_size: 0 } })])), DescendOrigin(X1(PalletInstance(1))), ExpectOrigin(Some(MultiLocation { parents: 0, interior: X2(Parachain(1), PalletInstance(1)) }))])    
TRACE get_version_1: state: method="Get" ext_id=97f3 key=5b6a986bc2690faba863ba2edcffd23404a74d81251e398fd8a0a4d55023bb3f result=Some(01000000) result_encoded=0101000000
DEBUG xcm::execute_xcm: origin: MultiLocation { parents: 0, interior: X1(Parachain(1)) }, message: Xcm([WithdrawAsset(MultiAssets([MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: Here }), fun: Fungible(10) }])), BuyExecution { fees: MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: Here }), fun: Fungible(10) }, weight_limit: Unlimited }, SetErrorHandler(Xcm([ReportError(QueryResponseInfo { destination: MultiLocation { parents: 0, interior: X1(Parachain(1)) }, query_id: 1234, max_weight: Weight { ref_time: 0, proof_size: 0 } })])), DescendOrigin(X1(PalletInstance(1))), ExpectOrigin(Some(MultiLocation { parents: 0, interior: X2(Parachain(1), PalletInstance(1)) }))]), weight_limit: Weight { ref_time: 18446744073709551615, proof_size: 18446744073709551615 }    
TRACE xcm::weight: FixedWeightBounds message: Xcm([WithdrawAsset(MultiAssets([MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: Here }), fun: Fungible(10) }])), BuyExecution { fees: MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: Here }), fun: Fungible(10) }, weight_limit: Unlimited }, SetErrorHandler(Xcm([ReportError(QueryResponseInfo { destination: MultiLocation { parents: 0, interior: X1(Parachain(1)) }, query_id: 1234, max_weight: Weight { ref_time: 0, proof_size: 0 } })])), DescendOrigin(X1(PalletInstance(1))), ExpectOrigin(Some(MultiLocation { parents: 0, interior: X2(Parachain(1), PalletInstance(1)) }))])    
TRACE xcm::execute_xcm_in_credit: origin: MultiLocation { parents: 0, interior: X1(Parachain(1)) }, message: Xcm([WithdrawAsset(MultiAssets([MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: Here }), fun: Fungible(10) }])), BuyExecution { fees: MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: Here }), fun: Fungible(10) }, weight_limit: Unlimited }, SetErrorHandler(Xcm([ReportError(QueryResponseInfo { destination: MultiLocation { parents: 0, interior: X1(Parachain(1)) }, query_id: 1234, max_weight: Weight { ref_time: 0, proof_size: 0 } })])), DescendOrigin(X1(PalletInstance(1))), ExpectOrigin(Some(MultiLocation { parents: 0, interior: X2(Parachain(1), PalletInstance(1)) }))]), weight_credit: Weight { ref_time: 0, proof_size: 0 }    
TRACE xcm::barriers: WithComputedOrigin origin: MultiLocation { parents: 0, interior: X1(Parachain(1)) }, instructions: [WithdrawAsset(MultiAssets([MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: Here }), fun: Fungible(10) }])), BuyExecution { fees: MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: Here }), fun: Fungible(10) }, weight_limit: Unlimited }, SetErrorHandler(Xcm([ReportError(QueryResponseInfo { destination: MultiLocation { parents: 0, interior: X1(Parachain(1)) }, query_id: 1234, max_weight: Weight { ref_time: 0, proof_size: 0 } })])), DescendOrigin(X1(PalletInstance(1))), ExpectOrigin(Some(MultiLocation { parents: 0, interior: X2(Parachain(1), PalletInstance(1)) }))], max_weight: Weight { ref_time: 6000, proof_size: 6000 }, weight_credit: Weight { ref_time: 0, proof_size: 0 }    
TRACE xcm::barriers: AllowExplicitUnpaidExecutionFrom origin: MultiLocation { parents: 0, interior: X1(Parachain(1)) }, instructions: [WithdrawAsset(MultiAssets([MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: Here }), fun: Fungible(10) }])), BuyExecution { fees: MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: Here }), fun: Fungible(10) }, weight_limit: Unlimited }, SetErrorHandler(Xcm([ReportError(QueryResponseInfo { destination: MultiLocation { parents: 0, interior: X1(Parachain(1)) }, query_id: 1234, max_weight: Weight { ref_time: 0, proof_size: 0 } })])), DescendOrigin(X1(PalletInstance(1))), ExpectOrigin(Some(MultiLocation { parents: 0, interior: X2(Parachain(1), PalletInstance(1)) }))], max_weight: Weight { ref_time: 6000, proof_size: 6000 }, weight_credit: Weight { ref_time: 0, proof_size: 0 }    
TRACE xcm::barriers: AllowTopLevelPaidExecutionFrom origin: MultiLocation { parents: 0, interior: X1(Parachain(1)) }, instructions: [WithdrawAsset(MultiAssets([MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: Here }), fun: Fungible(10) }])), BuyExecution { fees: MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: Here }), fun: Fungible(10) }, weight_limit: Unlimited }, SetErrorHandler(Xcm([ReportError(QueryResponseInfo { destination: MultiLocation { parents: 0, interior: X1(Parachain(1)) }, query_id: 1234, max_weight: Weight { ref_time: 0, proof_size: 0 } })])), DescendOrigin(X1(PalletInstance(1))), ExpectOrigin(Some(MultiLocation { parents: 0, interior: X2(Parachain(1), PalletInstance(1)) }))], max_weight: Weight { ref_time: 6000, proof_size: 6000 }, weight_credit: Weight { ref_time: 0, proof_size: 0 }    
TRACE xcm::process: origin: Some(MultiLocation { parents: 0, interior: X1(Parachain(1)) }), total_surplus/refunded: Weight { ref_time: 0, proof_size: 0 }/Weight { ref_time: 0, proof_size: 0 }, error_handler_weight: Weight { ref_time: 0, proof_size: 0 }    
TRACE xcm::process_instruction: === WithdrawAsset(MultiAssets([MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: Here }), fun: Fungible(10) }]))    
TRACE xcm::currency_adapter: withdraw_asset what: MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: Here }), fun: Fungible(10) }, who: MultiLocation { parents: 0, interior: X1(Parachain(1)) }    

TRACE xcm::process_instruction: === BuyExecution { fees: MultiAsset { id: Concrete(MultiLocation { parents: 0, interior: Here }), fun: Fungible(10) }, weight_limit: Limited(Weight { ref_time: 6000, proof_size: 6000 }) }    
TRACE xcm::weight: FixedRateOfFungible::buy_weight weight: Weight { ref_time: 6000, proof_size: 6000 }, payment: Assets { fungible: {Concrete(MultiLocation { parents: 0, interior: Here }): 10}, non_fungible: {} }    
TRACE xcm::execute: !!! ERROR: TooExpensive    
TRACE xcm::execute_xcm_in_credit: result: Err(ExecutorError { index: 1, xcm_error: TooExpensive, weight: Weight { ref_time: 4000, proof_size: 4000 } })    
TRACE xcm::weight: FixedRateOfFungible::refund_weight weight: Weight { ref_time: 4000, proof_size: 4000 }    
TRACE xcm::execute_xcm_in_credit: Execution errored at 1: TooExpensive (original_origin: MultiLocation { parents: 0, interior: X1(Parachain(1)) })
KiChjang commented 10 months ago

Ah ok, I see what's going on here -- BuyExecution is set with a weight_limit of Unlimited, and it doesn't go deep inside of the fees parameter to check and see if the sender has sent enough assets to cover the weight_limit.

AFAICT this is expected, since converting assets into equivalent weight units is not a trivial operation, so the AllowTopPaidExecutionFrom barrier is not able to reject it as we need barriers to be lightweight, therefore the executor is the only place that can check whether the given fees is enough to pay for the given weight. It then throws a TooExpensive error since there isn't enough fees.

But what is unexpected is that the ReportError instruction should still have been executed, as it's inside of a SetErrorHandler instruction, which can only be executed in case of error, i.e. the 2nd assertion should have returned false, as it would have received a response reporting the TooExpensive error. Did this not happen?

kirillt commented 10 months ago

It looks to me that SetErrorHandler is never executed since we fail earlier, at BuyExecution.

vstam1 commented 10 months ago

Hi Kirillt,

I have taken a look and it seems that this example is incorrect. If you take a look at the other examples you can see that we estimate the fees using the estimate_message_fee function. Somehow this example slipped through the cracks, so I will update it accordingly.

You are right that the SetErrorHandler is never executed as the BuyExecution fails. The BuyExecution instruction should however always be used before execution other instructions. If we would allow other instructions before BuyExecution (for example Transact) we would allow users to freely execute some instructions, which could open up potential DOS attacks. This is what the AllowTopPaidExecutionFrom barrier checks. You can try it out by moving the SetErrorHandler instruction before the WithdrawAsset instruction. You should see some Barrier error message in the trace.

Now for your concern about silently failing execution in the simulator. We have two functions for printing out events. The print_para_events and print_relay_events. In this case you could manually check if the test fails using the following piece of code:

Relay::execute_with(|| {
    print_relay_events();
});

You would probably see some kind of UMP event that says that the execution was Incomplete and then the XCM error. If you take a look at how we print out the events you could also write an function that matches on the events and checks if the execution was successful. It might not be the cleanest way but it is definitely possible.

Hope this answers your question and I will take a look at updating the example!

kirillt commented 10 months ago

Hello @vstam1,

Thank you so much for taking a look and composing very comprehensive answer!

Indeed, with Relay::execute_with(|| print_relay_events()); debugging is easier. And yes, there is Incomplete(..., TooExpensive)).

I have no other questions. Will continue learning XCM with xcm-simulator. Thanks for help!

KiChjang commented 10 months ago

Fixed by #43.