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

westend: SignedPhase is a constant #3646

Merged
merged 6 commits into from
Mar 26, 2024
Merged

westend: SignedPhase is a constant #3646

merged 6 commits into from
Mar 26, 2024

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Mar 11, 2024

In preparation for the merkleized metadata, we need to ensure that constants are actually constant. If we want to test the unsigned phase we could for example just disable signed voter. Or we add some extra mechanism to the pallet to disable the signed phase from time to time.

In preparation for the merkleized metadata, we need to ensure that
constants are actually constant. If we want to test the unsigned phase
we could for example just disable signed voter. Or we add some extra
mechanism to the pallet to disable the signed phase from time to time.
@bkchr bkchr added the R0-silent Changes should not be mentioned in any release notes label Mar 11, 2024
@bkchr bkchr requested review from gpestana and Ank4n March 11, 2024 14:12
Copy link
Contributor

@Ank4n Ank4n left a comment

Choose a reason for hiding this comment

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

All good. We will figure out another way to achieve this.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Given that we don't have a workaround in this PR yet,
and given that I think this value is actually kinda useless in the metadata,
and given that I like the testing property of this constant being such, ensuring our OCW code path is being regularly,

I suggest removing this from the metadata instead of this.

@niklasad1 if the staking-miner doesn't break, then we are good. I think it is literally the only application that might read this from the metadata.

@kianenigma
Copy link
Contributor

We will figure out another way to achieve this.

If not resolved within the context of this PR, please make an issue about it. If a good workaround exists, it might be a good one for mentoring.

@bkchr bkchr added this pull request to the merge queue Mar 24, 2024
@kianenigma kianenigma removed this pull request from the merge queue due to a manual request Mar 24, 2024
@kianenigma
Copy link
Contributor

My guess about this not being used in the staking miner was correct, so I suggest removing it from the metadata instead, as per the last commit.

@niklasad1
Copy link
Member

The staking miner is only using the signed_phase constant from the metadata and AFAIU this only removes unsigned_phase constant, correct?

This won't break the staking miner and I tested it right now as well.

@bkchr bkchr enabled auto-merge March 26, 2024 17:34
@bkchr bkchr added this pull request to the merge queue Mar 26, 2024
Merged via the queue into master with commit 0c15d88 Mar 26, 2024
126 of 132 checks passed
@bkchr bkchr deleted the bkchr-westend-signed-phase branch March 26, 2024 18:21
@niklasad1
Copy link
Member

@bkchr why did you remove the signed_phase? It was still present when I had a look at this PR IIRC but after this commit it broke the staking miner :)

I don't think it should be that hard to workaround though....

@bkchr
Copy link
Member Author

bkchr commented Apr 2, 2024

Because the fix that @kianenigma had done was incorrect :D Westend overwrites the SignedPhase, so only making UnsignedPhase not a constant doesn't solves my problem. I had really understood it like staking miner doesn't require any of them, but this was my misunderstanding. Sorry!

dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this pull request Apr 9, 2024
In preparation for the merkleized metadata, we need to ensure that
constants are actually constant. If we want to test the unsigned phase
we could for example just disable signed voter. Or we add some extra
mechanism to the pallet to disable the signed phase from time to time.

---------

Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants