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

Mandatory commitments only #1138

Closed
wants to merge 7 commits into from

Conversation

alistair-singh
Copy link
Contributor

@alistair-singh alistair-singh commented Jan 31, 2024

Adds the IsMandatory flag to SafeScanCommitmentResult. This is just appended to logging in this PR.

But I used this flag to skip signed commitments that are not mandatory in order to test parity paritytech/polkadot-sdk#3108 .

	if !result.IsMandatory {
		logEntry.Info("Skipped due to not mandatory commitment.")
		continue
	}

We do not need code changes to support the fix. Leaving the IsMandatory field in here because I think its useful for logging. In the future we could use it to save costs by limiting submissions to the light client to only mandatory commitments.

Resolves: SNO-842

Copy link
Contributor

@yrong yrong left a comment

Choose a reason for hiding this comment

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

+1

@@ -129,6 +131,21 @@ func scanCommitments(ctx context.Context, api *gsrpc.SubstrateAPI, startBlock ui
return
}

isMandatory := false
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rather we go all the way and add a documented MandatoryCommitmentsOnly config setting. Otherwise this code is going to be forgotten about it.

Copy link
Contributor

@yrong yrong Feb 6, 2024

Choose a reason for hiding this comment

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

We may not really need another config as it can be already achieved by setting

with a value big enough(i.e. more than blocks in an epoch).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in a791b2a.

We can use the update-period but this is more explicit and is good for testing the mmr fix as well. Currently turning this setting on will fail untill we update from upstream polkadot-sdk. I tested this change with cherry-picking .

$ git cherry-pick e5bb11b008ca6c1b5a07cab94a2327538f3bf15c

I think this setting also works well with incentivizing beefy relayers. For instance, our relayer can run in a mode where it only sync's mandatory commitments (i.e. 1 commitment per epoch). And if there are pending messages and an incentive is provided by a user to speed up the transaction then it can sync the next commitment it sees.

@alistair-singh alistair-singh force-pushed the alistair/mandatory-commitments-only branch from 89326d0 to d18ebc2 Compare February 6, 2024 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants