-
Notifications
You must be signed in to change notification settings - Fork 154
[Encointer] fix treasury asset payment #944
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
Merged
fellowship-merge-bot
merged 6 commits into
polkadot-fellows:main
from
encointer:cl/encointer-minor-patches
Oct 6, 2025
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
8f6590f
[Encointer] bump encointer primitives/democracy/treasuries to fix spe…
clangenb 9a88d98
reproduce Chopsticks XCM error
clangenb 747d89d
Fix XCM-Execution
clangenb 646b2bb
fmt & clippy
clangenb d9d3b6a
update changelog
clangenb 6956e4e
fix encointer unit tests
clangenb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, I thought that this is just a
max_weightissue, so I changed this to max. However, I still got aNotHoldingFeeserror, although I surely withdraw enough as shown in the logs.I have fixed the problem for now, but I guess that this might even be an XCM bug such that
ReportErrordoesn't the appendix together withDescendOriginyet? What do the XCM overlords think @franciscoaguirre, @bkontur, @acatangiu?XCM Execution Logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iiuc, are we talking about this XCM: https://github.com/polkadot-fellows/runtimes/blob/main/system-parachains/encointer/src/treasuries_xcm_payout.rs#L267-L283? Maybe just for completeness, could you add the full XCM from the logs here?
Do you have a block/transaction, that we can replay and investigate more?
From the logs above, I don't see any errored instruction after
SetAppendix, which is justTransferAsset, so it passed.But the
SetAppendix(ReportError(..)is trying to sendself.errorregister, even if the error register isNone.There are couple of issues or things to think about:
ReportErrorsends/reports error register regardless if Some/None, which means, for every successful XCM processing, it send one everytimeXcm(ReportError(None), which is kind of "questionable":if let Some(error) = self.error {...}here: https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/xcm-executor/src/lib.rs#L1141-L1151, but it is not aligned with actual ReportError description:Immediately report the contents of the Error Register to the given destination via XCM.https://github.com/paritytech/polkadot-sdk/blob/d36d48a25c80502c577df3b1a88eadb3b3d3b6be/polkadot/xcm/src/v5/mod.rs#L588-L597 (sure, we could potentially change the description and add the IF)NotHoldingFeesis correct error, because we can seeWithdrawAsset(remote_fee),PayFees(remote_fee)and here I am little bit confused, becausePayFeestakes the wholeremote_feefrom theself.holding, soself.holdingis now empty, and theunspentfrom trader should be return back toself.fees, but looks like it is not at this pointHolding doesn't hold enough for feesis from the wrong branch: https://github.com/paritytech/polkadot-sdk/blob/d36d48a25c80502c577df3b1a88eadb3b3d3b6be/polkadot/xcm/xcm-executor/src/lib.rs#L612-L614, I would expect here thatself.feescontains at leastself.fees.subsume_assets(unspent);, but it is empty, this is what I don't understand now, needs (clean morning mind) more investigationrefund_weightwhich adds something back to the holding is executed after all this stuff fromvm.post_processcc: @franciscoaguirre @raymondkfcheung
@clangenb Hmm, I still had it in my head that somewhere during PR reviews I came across
ReportError, and I think it was exactly this XCM :D #679 (comment)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other thing is if
DescendOrigin(as effective origin) should pay forReportErrorordestinationsovereign account (which would be Encointer parachain), but this needs more thinking, not to open possible free spamming with ReportError or something, so something like:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @bkontur that
AliasOrigin+ reorderingReportErrorlooks like the right fix here. We just need to make sure it can't be abused for freeReportErrorspam.For tracing, I'll follow up with targeted logs around the appendix path. I need a bit more time to place these in the right spots so they're useful without being noisy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, I am also not sure if
AliasOriginwould work here, because ofAliasers:target does not start with origin (
AliasChildLocation):For:
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks for looking into this.
You are right @bkontur, this is the xcm we are talking about.
In this setup at least, I believe that it could not be abused for free error spams because whatever is triggering the alias back into the Encointer location would need to pay XCM execution fees on AH. So they just get the spam for free, but not without paying for the enclosing XCM first, which I guess is a reasonable spam prevention mechanism.
Regardless, in order to have encointer fixed soon, I suggest that I open another issue because I think that fixing this properly might even need some design thought in XCM first. We would really like to have a working patch released.
I do not have a block to replay, but I could give you a chopsticks setup that could trigger the XCM from encointer to AH with one single extrinsic. Would this work @bkontur?
Haha, you are on point ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding your second point @bkontur, I have opened an issue for further investigation, as this might be a general XCM bug: paritytech/polkadot-sdk#10078.