-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[AHM] Staking async fixes for XCM and election planning #8422
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
Conversation
|
\cmd prdoc --bump minor |
substrate/frame/staking-async/runtimes/parachain/src/staking.rs
Outdated
Show resolved
Hide resolved
substrate/frame/staking-async/runtimes/parachain/src/staking.rs
Outdated
Show resolved
Hide resolved
substrate/frame/staking-async/runtimes/parachain/src/staking.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Adrian Catangiu <[email protected]>
Co-authored-by: Adrian Catangiu <[email protected]>
Co-authored-by: Adrian Catangiu <[email protected]>
…staking-async-xcm-weight
|
/cmd prdoc --bump minor |
…staking-async-xcm-weight
…d now allow page count to be in storage
substrate/frame/staking-async/runtimes/parachain/src/staking.rs
Outdated
Show resolved
Hide resolved
#8585) Backports a part of #8422 to master so it can be included in ongoing releases sooner. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Dónal Murray <[email protected]>
| } | ||
|
|
||
| /// Common utility to send XCM messages that can use [`SplittableMessage`]. | ||
| pub struct XCMSender<Sender, Destination, Message, ToXcm>( |
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.
@franciscoaguirre @acatangiu I tried to be smart here and refactor the XCM sending facility in both RC and AH into one type, but I now realize that this has a downside that it is binding both to use the same xcm types, aka v5. This will be an issue right? or do we update XCM versions across system chains in sync?
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.
@franciscoaguirre @acatangiu I tried to be smart here and refactor the XCM sending facility in both RC and AH into one type, but I now realize that this has a downside that it is binding both to use the same xcm types, aka v5. This will be an issue right? or do we update XCM versions across system chains in sync?
@kianenigma all the xcm/xcm-builder stuff/tooling is using latest XCM version. Are you using any specific instruction from XCMv5? If not then, the different XCM version on different chains shouldn't be an issue, because at the end you are sending the XCM with Sender: SendXcm implementation which I guess it would be either struct ParentAsUmp or struct ChildParachainRouter, and they both are doing wrap_version to VersionedXcm according to the known XCM versions of receiving chain.
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.
The only thing, if you are using xcm::v5::Transact and just to be sure, you need to set fallback_max_weight: Some(...), - in case it will go to the chain with just XCMv4
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.
and also with 1.5.1 release all the system+relay chains got XCMv5, so there shouldn't be any issue with XCM vesrions between relaychain and AHs
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.
Thanks for the info @bkontur!
We are indeed using Transact, and atm I am setting fallback_max_weight to None.
This type is only intended to be used between RC <> AH, so version mismatch will not be an issue.
I conclude that:
- within this PR, we are good
- what I need to do to ace this is to have emulator tests in the fellowship runtimes that ensures these XCMs can always be sent between RC <> AH of polkadot and kusama.
…staking-async-xcm-weight
…polkadot-sdk into kiz-staking-async-xcm-weight
| { | ||
| Ok((_ticket, price)) => { | ||
| log::debug!(target: "runtime::staking-async::xcm", "📨 validated, price: {:?}", price); | ||
| return Ok(current_messages.into_iter().map(ToXcm::convert).collect::<Vec<_>>()); |
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'm not sure if I understand this correctly. At first, you're trying to validate whether it's okay to send just one message using first_xcm = ToXcm::convert(current_messages.first()). If the validation succeeds, you then return all the current_messages, not just the one that was validated?
I think this isn't fully compliant with the SendXcm trait's description. According to it, the ticket returned by SendXcm::validate should be passed to SendXcm::deliver.
Or is this kind of "best effort" strategy? Validate at least first one and then try to send more and just log errors when they fail?
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.
Great catch! this goes back to
// the first message is the heaviest, the last one might be smaller.
In that our only concern here is the size. And the first message is always the largest. All the rest are equal or smaller, so if the first one is validated, we should be good.
I think this isn't fully compliant with the SendXcm trait's description. According to it, the ticket returned by SendXcm::validate should be passed to SendXcm::deliver.
I am sort of skipping this, as I am dropping ticket in this scope. Laster on, when I call send, it will re-do validate internally and pass on the ticket.
- Deprecated `asset-hub-next`, replaced by `staking-async-parachain` and `staking-async-rc` in paritytech/polkadot-sdk#8422 - Add support for statemint, statemine, westmint AH chains, without dropping the polkadot / kusama / westend RC variants until we deprecate the legacy `monitor` path and rely only on EPMB For the time being, static miner's config for StakingAsync is a copy-paste of Westend one but will evolve independently. As a separate topic (see #994), a partial / complete removal of the static configuration per chain will be tackled.
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
* static config: Add support for AH and staking-async chains - Deprecated `asset-hub-next`, replaced by `staking-async-parachain` and `staking-async-rc` in paritytech/polkadot-sdk#8422 - Add support for statemint, statemine, westmint AH chains, without dropping the polkadot / kusama / westend RC variants until we deprecate the legacy `monitor` path and rely only on EPMB For the time being, static miner's config for StakingAsync is a copy-paste of Westend one but will evolve independently. As a separate topic (see #994), a partial / complete removal of the static configuration per chain will be tackled. * Remove staking-async support from legacy This change removes staking-async runtime support from the legacy monitor code path, replacing it with appropriate error messages for unsupported cases.
* master: omni-node: fix `benchmark pallet` to work with `--runtime` (#8594) Handle and suppress "New unknown `FromSwarm` libp2p event" warning (#8731) Implement detailed logging for XCM failures (#8724) [pallet-revive] contract's nonce starts at 1 (#8734) sync/fix: Clear gap sync on known imported blocks (#8445) [PoP] Add personhood tracking pallets (#8164) client/net: Use litep2p as the default network backend (#8461) Unflake `returns_status_for_pruned_blocks` (#8709) [AHM] Report the weights of epmb pallet to expose kusama and polkadot weights (#8704) Remove all XCM dependencies from `pallet-revive` (#8584) Docker master image tag fix (#8711) Record ed as part of the storage deposit (#8718) [pallet-revive] update dry-run logic (#8662) feat: add collator peer ID to ParachainInherentData (#8708) Nest errors in pallet-xcm (#7730) pallet-assets ERC20 precompile (#8554) Broker: Introduce min price + adjust renewals to lower market. (#8630) [AHM] Staking async fixes for XCM and election planning (#8422) Staking (EPMB): Add defensive error handling to voter snapshot creation and solution verification (#8687)
This PR brings a few small fixes related to the XCM messages of staking-async, among other small fixes: * [x] Allows `xcm::validate` to check the message size, and we actually now act upon it in the `staking-async-rc/parachain-runtime`s. The code is a bit duplicate now, and there is a TOOD about how to better refactor it later. * [x] Part of this work is backported separately as #8409 * [x] It brings a default `EraElectionPlannerOf` which should be the right tool to use to ensure elections always happen in time, with an educated guess based on `ElectionProvider::duration` rather than a random number. * [x] It adds a few unit tests about the above * [x] It silences some logs that were needlessly `INFO`, and makes the printing of some types a bit more CLI friendly. * [x] Fix the issue with duplicate voters in solution type: see #8585 * [x] Move `PagedRawSolution` to be unbounded, and therefore decode-able without PoV * [x] Undo a rename from `ClaimedRewards` to `ErasClaimedRewards` * [ ] Needs fixing in westend --------- Co-authored-by: Adrian Catangiu <[email protected]> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Paolo La Camera <[email protected]>
#8585) Backports a part of #8422 to master so it can be included in ongoing releases sooner. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Dónal Murray <[email protected]>
This PR brings a few small fixes related to the XCM messages of staking-async, among other small fixes: * [x] Allows `xcm::validate` to check the message size, and we actually now act upon it in the `staking-async-rc/parachain-runtime`s. The code is a bit duplicate now, and there is a TOOD about how to better refactor it later. * [x] Part of this work is backported separately as #8409 * [x] It brings a default `EraElectionPlannerOf` which should be the right tool to use to ensure elections always happen in time, with an educated guess based on `ElectionProvider::duration` rather than a random number. * [x] It adds a few unit tests about the above * [x] It silences some logs that were needlessly `INFO`, and makes the printing of some types a bit more CLI friendly. * [x] Fix the issue with duplicate voters in solution type: see #8585 * [x] Move `PagedRawSolution` to be unbounded, and therefore decode-able without PoV * [x] Undo a rename from `ClaimedRewards` to `ErasClaimedRewards` * [ ] Needs fixing in westend --------- Co-authored-by: Adrian Catangiu <[email protected]> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Paolo La Camera <[email protected]>
This PR brings a few small fixes related to the XCM messages of staking-async, among other small fixes:
xcm::validateto check the message size, and we actually now act upon it in thestaking-async-rc/parachain-runtimes. The code is a bit duplicate now, and there is a TOOD about how to better refactor it later.EraElectionPlannerOfwhich should be the right tool to use to ensure elections always happen in time, with an educated guess based onElectionProvider::durationrather than a random number.INFO, and makes the printing of some types a bit more CLI friendly.PagedRawSolutionto be unbounded, and therefore decode-able without PoVClaimedRewardstoErasClaimedRewards