Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Fix Westend Teleport Weight #4143

Closed
wants to merge 3 commits into from

Conversation

apopiak
Copy link
Contributor

@apopiak apopiak commented Oct 25, 2021

Something went wrong with the XCM benchmarks for Westend which resulted in the default weight of 2 trillion being used for ReceiveTeleportedAsset.
This moves that weight back to the previous default of 1 billion so teleport XCMs can be processed.

Also bumps the spec version so the fix can be deployed.

🙏 to @NachoPal for the find.

@apopiak apopiak added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Oct 25, 2021
@shawntabrizi
Copy link
Member

I'm doing further investigation into the benchmark with this output.

Copy link
Contributor

@KiChjang KiChjang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any other weights that got defaulted to 2 trillion? If not, this looks good to me

@apopiak
Copy link
Contributor Author

apopiak commented Oct 25, 2021

Are there any other weights that got defaulted to 2 trillion? If not, this looks good to me

There, in fact, are, e.g.
https://github.com/paritytech/polkadot/blame/03fcdc4215c5a1b5207bb71ceaf764bd3f6f4d98/runtime/kusama/src/weights/runtime_parachains_configuration.rs#L80-L82
Coming from:
#3862

@apopiak
Copy link
Contributor Author

apopiak commented Oct 25, 2021

6 results - 6 files

runtime/kusama/src/weights/pallet_elections_phragmen.rs:
  96  	fn remove_member_without_replacement() -> Weight {
  97: 		(2_000_000_000_000 as Weight)
  98  	}

runtime/kusama/src/weights/runtime_parachains_configuration.rs:
  80  	fn set_hrmp_open_request_ttl() -> Weight {
  81: 		(2_000_000_000_000 as Weight)
  82  	}

runtime/polkadot/src/weights/pallet_elections_phragmen.rs:
  125  	fn remove_member_without_replacement() -> Weight {
  126: 		(2_000_000_000_000 as Weight)
  127  	}

runtime/polkadot/src/weights/runtime_parachains_configuration.rs:
  80  	fn set_hrmp_open_request_ttl() -> Weight {
  81: 		(2_000_000_000_000 as Weight)
  82  	}

runtime/rococo/src/weights/runtime_parachains_configuration.rs:
  80  	fn set_hrmp_open_request_ttl() -> Weight {
  81: 		(2_000_000_000_000 as Weight)
  82  	}

runtime/westend/src/weights/runtime_parachains_configuration.rs:
  76  	fn set_hrmp_open_request_ttl() -> Weight {
  77: 		(2_000_000_000_000 as Weight)
  78  	}

@apopiak
Copy link
Contributor Author

apopiak commented Oct 25, 2021

@kianenigma Can you comment on the Phragmen weight?
Not sure who to ask for the runtime parachains config ones

@shawntabrizi
Copy link
Member

@apopiak remove_member_without_replacement is correct.

The other XCMs listed are not implemented in the XCM executor, so are also correct, as these XCMs should be invalid. (Maybe there is a better way to denote this).

@apopiak
Copy link
Contributor Author

apopiak commented Oct 25, 2021

Maybe we could pipe some message through the weights generator that ends up as a comment over the weight?

@apopiak
Copy link
Contributor Author

apopiak commented Oct 25, 2021

Both the failed checks should not apply. Can a team lead merge this? @shawntabrizi (Not sure how I'd fix the Cumulus build)

@shawntabrizi
Copy link
Member

This PR can probably be superseded by: #4146

@joepetrowski
Copy link
Contributor

This PR can probably be superseded by: #4146

As long as #4146 gets backported into this branch, then yes.

@apopiak apopiak closed this Oct 27, 2021
@apopiak apopiak deleted the apopiak/fix-westend-teleport-weight branch November 17, 2021 14:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants