refactor and harden check_core_index#6217
Conversation
alindima
added
the
T8-polkadot
ordian
left a comment
ordian
left a comment
There was a problem hiding this comment.
I see that we harden the checks in sanitize_backed_candidate_v2, but remove them in inclusion for upward_messages.
I wonder if it makes sense to introduce some type safety around it, so that we make sure the messages we got in inclusion were already checked. But this could be a part of further refactoring.
|
|
||
| if signals_iter.next().is_some() { | ||
| let Some(core_selector_message) = signals_iter.next() else { return Ok(None) }; | ||
| // We should have exactly one signal beyond the separator |
There was a problem hiding this comment.
unrelated q: right now we have a strict check of at most one signal
IIUC this is being check on both node and runtime sides
if we are to add more signals in the future, how are we going to coordinate the upgrade? via node_features?
There was a problem hiding this comment.
I discussed this with @sandreim a while ago. Yes, we'll probably use a new node feature
There was a problem hiding this comment.
Yes, we'll need a feature anyway if we want to introduce a new signal.
| } else { | ||
| upward_messages | ||
| }; | ||
| let upward_messages = skip_ump_signals(upward_messages.iter()).collect::<Vec<_>>(); |
There was a problem hiding this comment.
this has a downside of allocation in comparison to the previous version, but it copies only references to the messages, so should not be bad
This would be a good idea for further refactoring. |
This is a very good idea in general: for any thing we check in the runtime we need to have separate types (checked/unchecked). |
|
|
||
| // Use first commitment | ||
| let message = self.upward_messages.get(separator_pos + 1)?; | ||
| /// Utility function for skipping the ump signals. |
There was a problem hiding this comment.
Any good reason not to put it inside CandidateCommitments ?
There was a problem hiding this comment.
yes, in the runtime we use this function after taking the upward messages from the commitments struct
* master: (129 commits) pallet-revive: Use `RUSTUP_TOOLCHAIN` if set (#6365) [eth-rpc] proxy /health (#6360) [Release|CI/CD] adjust release pipelines (#6366) Bump the known_good_semver group across 1 directory with 3 updates (#6339) Run check semver in MQ (#6287) [Deprecation] deprecate treasury `spend_local` call and related items (#6169) refactor and harden check_core_index (#6217) litep2p: Update litep2p to v0.8.0 (#6353) [pallet-staking] Additional check for virtual stakers (#5985) migrate pallet-remarks to v2 bench syntax (#6291) Remove leftover references of Wococo (#6361) snowbridge: allow account conversion for Ethereum accounts (#6221) authority-discovery: Populate DHT records with public listen addresses (#6298) Bounty Pallet: add `approve_bounty_with_curator` call to `bounties` pallet (#5961) Silent annoying log (#6351) [pallet-revive] rework balance transfers (#6187) `statement-distribution`: RFC103 implementation (#5883) Disable flaky tests reported in #6343 / #6345 (#6346) migrate pallet-recovery to benchmark V2 syntax (#6299) inclusion emulator: correctly handle UMP signals (#6178) ...
github-project-automation
bot
moved this from Todo
to Done
in [preview] release tracker
5 participants
Resolves #6179