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

inclusion emulator: correctly handle UMP signals #6178

Open
wants to merge 145 commits into
base: master
Choose a base branch
from

Conversation

sandreim
Copy link
Contributor

@sandreim sandreim commented Oct 22, 2024

Changes inclusion emulator to not count the UMP signals when checking ump message constraints.

Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
sandreim and others added 23 commits September 23, 2024 13:02
Signed-off-by: Andrei Sandu <[email protected]>
…aritytech/polkadot-sdk into sandreim/node_v2_descriptors
Signed-off-by: Andrei Sandu <[email protected]>
…aritytech/polkadot-sdk into sandreim/node_v2_descriptors

Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
Signed-off-by: Andrei Sandu <[email protected]>
@sandreim sandreim added R0-silent Changes should not be mentioned in any release notes T8-polkadot This PR/Issue is related to/affects the Polkadot network. labels Oct 22, 2024
Signed-off-by: Andrei Sandu <[email protected]>
Copy link
Contributor

@alindima alindima left a comment

Choose a reason for hiding this comment

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

Let's also add a prdoc and remove the silent label

@@ -600,6 +600,18 @@ impl Fragment {
validation_code_hash: &ValidationCodeHash,
persisted_validation_data: &PersistedValidationData,
) -> Result<ConstraintModifications, FragmentValidityError> {
// Filter UMP signals and the separator.
let upward_messages = if let Some(separator_index) =
Copy link
Contributor

Choose a reason for hiding this comment

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

would be good to deduplicate this filtering in a function on CandidateCommitments. We're doing this in multiple places. we should also use the UMP_SEPARATOR

Copy link
Contributor Author

@sandreim sandreim Oct 24, 2024

Choose a reason for hiding this comment

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

Yeah, can you add such a function to your PR: #6217 ? it's easier to propagate to this PR since yours has master as base and also need it in inclusion:

		let upward_messages = upward_messages
			.iter()
			.take_while(|message| message != &&UMP_SEPARATOR)
			.collect::<Vec<_>>();

@@ -750,7 +764,7 @@ fn validate_against_constraints(
})
}

if commitments.upward_messages.len() > constraints.max_ump_num_per_candidate {
if upward_message_count > constraints.max_ump_num_per_candidate {
Copy link
Contributor

Choose a reason for hiding this comment

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

we can access the ump_messages_sent from the modifications param and spare this extra parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I just 🙈 it

Base automatically changed from sandreim/node_v2_descriptors to master October 25, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T8-polkadot This PR/Issue is related to/affects the Polkadot network.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants