Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SetAppendix(ReportError(QueryResponseInfo)) instruction stopped reporting xcm execution result #3198

Closed
2 tasks done
GopherJ opened this issue Feb 4, 2024 · 32 comments
Closed
2 tasks done
Assignees
Labels
I2-bug The node fails to follow expected behavior. I10-unconfirmed Issue might be valid, but it's not yet known. T6-XCM This PR/Issue is related to XCM.

Comments

@GopherJ
Copy link

GopherJ commented Feb 4, 2024

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • This is not a support question.

Description of bug

  1. reserve_transfer_assets may causes users to lose all funds if insufficient KSM/DOT left in wallet.
  2. SetAppendix(ReportError(QueryResponseInfo)) seems stopped reporting xcm status to parachain. We start to see this after the latest kusama upgrade

Steps to reproduce

No response

@GopherJ GopherJ added I10-unconfirmed Issue might be valid, but it's not yet known. I2-bug The node fails to follow expected behavior. labels Feb 4, 2024
@dudo50
Copy link
Contributor

dudo50 commented Feb 4, 2024

Hey @GopherJ ,
I have already reported this here: #3050
The fix is being issued in the latest 1.1.2 update and you can vote on it here: https://kusama.polkassembly.io/referenda/339

@acatangiu
Copy link
Contributor

Marking as duplicate of #3050

@GopherJ
Copy link
Author

GopherJ commented Feb 5, 2024

@acatangiu @dudo50 How about another issue?

SetAppendix(ReportError(QueryResponseInfo)) seems stopped reporting xcm status to parachain. We start to see this after the latest kusama upgrade

I think it should be re-opened

@acatangiu
Copy link
Contributor

@franciscoaguirre can you take a look please?

@GopherJ
Copy link
Author

GopherJ commented Feb 6, 2024

hi @franciscoaguirre do you have any guess? we are live on kusama as Parallel Heiko network, we had this issue since 13 days ago

You can see kusama is not re-calling our notification_received method since that time https://parallel-heiko.subscan.io/event?page=1&time_dimension=date&module=liquidstaking&event_id=notificationreceived

code ref: https://github.com/parallel-finance/parallel/blob/efa2146c42fd85743a6565b32a9bb65e9e991620/pallets/xcm-helper/src/lib.rs#L258-L280

I've checked the transact was executed successfully on kusama but without callback

@franciscoaguirre
Copy link
Contributor

I'll look into it and let you know what I find

@franciscoaguirre
Copy link
Contributor

I think I know what's the problem. @GopherJ can you provide me with the extrinsic that triggers the notification? So I can replay it locally and look at the logs

@GopherJ
Copy link
Author

GopherJ commented Feb 7, 2024

@franciscoaguirre sure

You can check https://parallel-heiko.subscan.io/event?page=1&time_dimension=date&module=liquidstaking&event_id=unbonding these are all xcm extrinsics that we sent to kusama

picking https://parallel-heiko.subscan.io/block/5346019?tab=event&event=5346019-3, it comes from the block 0x7136d6bab11772edc763e98479f851aad8e0b979166fc8231d46d21b4a78a354 which corresponds relaychian block: 21,766,456

and it finally executed in relaychain block 21,766,459 as you can see from: https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama.api.onfinality.io%2Fpublic-ws#/explorer/query/21766459

@GopherJ
Copy link
Author

GopherJ commented Feb 7, 2024

The extrinsic sent to kusama should be simple, it should be staking.unbond(31204272643408)

@GopherJ
Copy link
Author

GopherJ commented Feb 8, 2024

@franciscoaguirre hi, do you have any ideas?

@franciscoaguirre
Copy link
Contributor

I haven't managed to run it locally with the same configuration, but it's an issue with the ReportError instruction needing to send a new message and not having anything in the holding register to pay for delivery fees.

I saw the message before the appendix does RefundSurplus and DepositAsset. Could you try removing the DepositAsset and putting it after the ReportError instruction in the appendix? That would make sure the weight surplus is still in the holding register, so it can be used for the delivery fees.

This situation definitely needs to be improved, but please let me know if that solves the issue.

@GopherJ
Copy link
Author

GopherJ commented Feb 13, 2024

@acatangiu could you reopen this issue?

@acatangiu acatangiu reopened this Feb 13, 2024
@GopherJ
Copy link
Author

GopherJ commented Feb 18, 2024

@franciscoaguirre @acatangiu shall this be fixed on kusama side instead, ideally we shouldn't see breaking change going this way. How do you think?

@GopherJ GopherJ changed the title Xcm issues SetAppendix(ReportError(QueryResponseInfo)) instruction stopped reporting xcm execution result Feb 18, 2024
@Aleksey4Crypro
Copy link

Hey guys, how much time approximately remains for you to fix the issue caused by KSM update? @franciscoaguirre @acatangiu

@franciscoaguirre
Copy link
Contributor

It is going to be fixed in the Kusama side, maybe I didn't make myself clear. What I meant is while I'm working on a fix, you can try what I told you.

Once I have the fix I'll push for getting it on Kusama as soon as possible.

@acatangiu
Copy link
Contributor

Hey guys, how much time approximately remains for you to fix the issue caused by KSM update? @franciscoaguirre @acatangiu

If you have any workaround (like the one suggested by Francisco), I suggest you use it. Even if we'd have a fix for this tomorrow, it will still take a few weeks until the next https://github.com/polkadot-fellows/runtimes/ release containing it, plus around 2 weeks until on-chain upgrade referenda passes and is enacted.

TLDR: under the normal release process it will take a few weeks at least..

@franciscoaguirre
Copy link
Contributor

There should be no problem if you don't empty the holding register (do RefundSurplus) before the ReportError instruction.

@GopherJ
Copy link
Author

GopherJ commented Feb 23, 2024

@franciscoaguirre @acatangiu why has this version been upgraded to polkadot before the fix? it's ridiculous

@franciscoaguirre
Copy link
Contributor

Hi, unfortunately getting to the fix took a while, since we want to make sure we don't introduce new errors.

Have you tried making sure you don't deposit until the very end of the program execution? In the appendix.
I know it's not ideal, but it should help you continue your operations.

The fix is being worked on here: #3450

@franciscoaguirre
Copy link
Contributor

Also, I don't see a way for you to not have to change anything and have this work sadly. In an ideal world, it would just be a simple fix that wouldn't require any "breaking" change from your side.

@acatangiu
Copy link
Contributor

@franciscoaguirre @acatangiu why has this version been upgraded to polkadot before the fix? it's ridiculous

I guess it's a mix of things:

  1. failure on our side to raise visibility of this issue in governance proposal to upgrade Polkadot
  2. failure on your side to raise visibility of this issue in governance proposal to upgrade Polkadot
  3. the fact that this Polkadot release came with a many awaited features/improvements and the community was happy to take it as is

Regarding point 1 above, a workaround was proposed to you for which you did not provide any feedback or answer on. For better alignment on priority/impact/importance, this two-way communication needs improvement.

@acatangiu acatangiu added the T6-XCM This PR/Issue is related to XCM. label Mar 1, 2024
@franciscoaguirre
Copy link
Contributor

@GopherJ We are discussing a solution to this problem in XCM RFC 53, since it needs a change to the format itself. All contributions are welcome.

@KudosDot
Copy link

KudosDot commented Mar 12, 2024

@franciscoaguirre any updates on this?

your help is much appreciated.

@acatangiu
Copy link
Contributor

acatangiu commented Mar 12, 2024

@GopherJ @KudosDot asking yet again, have you tried the following workaround: #3198 (comment) ?

Let me clarify the situation here: the existing XCMs that you were using are probably forever broken, there is no "fix" for them AFAIK.

The reason of the breakage is a security fix which enforces that all XCM message deliveries have to be paid. Aka free message delivery is a thing of the past.
This ^ was absolutely required to close an attack surface where the whole network was potentially DOSable.

The side-effects of this fix were a number of unexpected and unintended breakages of existing XCM programs out there.

Unfortunately, some of them can't be fixed without changing them - e.g. if an XCM program relies on free delivery, it will never work anymore, the XCM program has to change, not the system underneath it.

Please take the above into consideration, reassess your issue accordingly, and let us know if:

  1. you believe your XCM program as is should work and it is an underlying system problem/bug (also please provide reasoning)
    or,
  2. you can adapt your XCM program to work under the new system conditions

@Aleksey4Crypro
Copy link

Hey guys, i have a few questions:

  1. How much time remains to fix issues with the XCM? This issue is already for more than 3 weeks.
  2. Why CMs of Parity team saying that Polkadot doesn't have any issues and this issue from Parallel Side? Can you ask your CMs to provide to the users the correct information, since it is not the issue from Parallel side and we can't influence on it. Right now, a lot of users are writing that we don't want to fix this issue because your CMs are providing the wrong information.

Thanks in advance for your replies.

@franciscoaguirre @acatangiu @dudo50

@pr0gress0r
Copy link

Hi everyone,

I am an investor in Parallel Finance, and due to this issue, we have been unable to unstake our DOT/KSM for a month now. All we get when asking when the issue will be resolved is "Parity is working on the fix," but there is no information on timelines. Reading into this matter #3198 (comment), I understand that Parity will not be making the fix and the Parallel team should address the issue. If that's the case, please help them do so, as they believe you should solve this problem on your own.
We have been unable to access our funds for a month, so any options that could help us would be appreciated.
Thank you in advance.

@acatangiu @franciscoaguirre @dudo50 @ggwpez

@dudo50
Copy link
Contributor

dudo50 commented Mar 16, 2024

Unsure why I am getting tagged guys, I don't work at Parity. I was reporting issue regarding asset loss during XCM.

Thanks for your understanding.

With kind regards,
Dudo

@franciscoaguirre
Copy link
Contributor

Hello @pr0gress0r, I'm very sorry to hear that.

There's a short-term and a long-term fix for this issue.

The short-term fix requires some work on their end. We've already said what works needs to be done. We are happy to give as many pointers as needed. We've asked if they've tried the changes and encountered any issue, but haven't gotten a response.

The long-term fix is being worked on. The reason there's no timeline update is that our main priority is to make sure nothing is broken again, as this was very unfortunate, and we don't want to make things more difficult for other teams as well.

@GopherJ I'd be happy to work alongside you to implement the fix we proposed.

@pr0gress0r
Copy link

Hi everyone,

@Aleksey4Crypro @GopherJ any updates? Please reply to @franciscoaguirre if you have problems implementing the fix, he will help resolve the issue.

@oilreg
Copy link

oilreg commented Mar 21, 2024

Parallel is kinda in denial regarding this .. same as @pr0gress0r my funds are locked in DOT / KSM liquid staking and also the redeeming of the crowdloans doesnt work, atm i am also worried with the staking rewards if we might get slashed, cause on their website the amount of sDOT / sKSM stays the same

.. posting a link to this discussion on their discord autodeletes the post with the link
.. so it seems the only happy ending to this dilemma is a fix from parity

@oilreg
Copy link

oilreg commented Mar 25, 2024

wanted to give a little update on my end... as of today .. unstaking sKSM with the 7 days waiting period seemed to work.. in 7 days i know more

@acatangiu
Copy link
Contributor

IIUC we can close this now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I2-bug The node fails to follow expected behavior. I10-unconfirmed Issue might be valid, but it's not yet known. T6-XCM This PR/Issue is related to XCM.
Projects
Status: Backlog
Development

No branches or pull requests

8 participants