feat(runtime): add the beefy session key and combine the v1-to-v2 and session-key migrations - #1953
feat(runtime): add the beefy session key and combine the v1-to-v2 and session-key migrations#1953MicroProofs wants to merge 3 commits into
Conversation
MicroProofs
changed the title
| // Initializes the QueuedCommittee storage added in v2 | ||
| pallet_session_validator_management::migrations::v2::V1ToV2Migration<Runtime>, | ||
| // See migrations::authority_keys when opaque::SessionKeys changes shape. | ||
| // v0->v1/v1->v2 migrations were dropped: all networks are at storage version >= 2, and |
There was a problem hiding this comment.
preview, preprod, and mainnet are not at PartnerChains v2. Are they?
Unfortunately custom migration is required: one that would be able to go from V1 to V2 together with session keys migration. pallet_session_validator_management::migrations::v2::V1ToV2Migration<Runtime> was not able to do it. Careful combination of both migrations is required here.
| grandpa: old.grandpa, | ||
| // No cross-chain id in scope here, so pallet_session key entries get a | ||
| // placeholder; they are rewritten from the committee at the next rotation. | ||
| beefy: sp_core::ecdsa::Public::from_raw([0u8; 33]).into(), |
There was a problem hiding this comment.
Would beefy recover from a period of time when node cannon find a matching private key and sign blocks?
There was a problem hiding this comment.
Pushing up some changes in a moment
There was a problem hiding this comment.
Yes as long as beefy eventually gets 2/3 vote on a stuck round then it continues until it catches up
MicroProofs
changed the title
84e45fb to
685ea36
Compare
| origin: OriginFor<T>, | ||
| updates: BoundedVec<(T::AuthorityId, Option<BeefyKeyBytes>), T::MaxUpdatesPerBlock>, | ||
| ) -> DispatchResult { | ||
| ensure_none(origin)?; |
There was a problem hiding this comment.
I've seen recommendations to have a storage (killed in in on_finalize) to prevent malicious nodes include multiple calls of None origin and fill up a block.
If this pallet will be removed before opening mainnet for registered candidates, I think it is okay to not add this complexity.
EDIT: I see we are looking for registered candidates keys as well, so this perhaps will stay and should be protected from multiple inclusion.
| The `start-node` wizard now warns about a missing `beef` key instead of refusing | ||
| to start. The node also logs a warning when it starts as an authority without | ||
| this key because it cannot participate in BEEFY voting. BEEFY keys are never | ||
| derived from the cross chain key, and the `AuraToBabeMigrationKeystore` |
There was a problem hiding this comment.
Local-environment setup (res/local/permissioned-candidates-config.json and keystores in local-environment itself) will require update.
This comment has been minimized.
This comment has been minimized.
MicroProofs
changed the title
MicroProofs
added
the
bot:ai-assisted
14bc529 to
5fd5369
Compare
5fd5369 to
90662e9
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90662e949d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Wire the cutover migration from `crate::migrations::authority_keys` here. | ||
| type SingleBlockMigrations = (); |
There was a problem hiding this comment.
For any existing chain whose on-chain pallet storage version is still 1, installing this runtime now leaves QueuedCommittee absent because no migration runs. The queue-based code then reads its ValueQuery default (epoch 0 and an empty committee), uses that to select epoch 1 in create_inherent, and can promote the empty value during rotation. Deferring the combined migration until a later session-key upgrade is only safe if this intermediate runtime can never be enacted; otherwise the seed-only migration must remain wired or the combined migration must run in this upgrade.
Useful? React with 👍 / 👎.
90662e9 to
0a8e027
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a8e027923
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .map(|member| { | ||
| let member = <OldCommitteeMember as UpgradeCommitteeMember<T>>::upgrade(member); | ||
| (member.authority_id(), member.authority_keys()) |
There was a problem hiding this comment.
When a runtime's UpgradeCommitteeMember implementation accidentally changes an authority ID or other non-key member data, this fingerprint applies the same faulty upgrade to the saved pre-upgrade value that the migration applied to storage. The two fingerprints therefore still match and try-runtime reports that committee membership was preserved even though it was corrupted. Construct the expected fingerprint from the old member's original authority_id() and convert only its authority keys, as the previous implementation did.
Useful? React with 👍 / 👎.
| #runtime #migration #committee-selection | ||
| # Combine committee v1-to-v2 and session-key migrations |
There was a problem hiding this comment.
This commit removes the public reusable AuthorityKeysMigration API and the migrations::v2 module from the Partner Chains toolkit, but records the change only under changes/runtime; partner-chains/changelog.md still advertises AuthorityKeysMigration as an unreleased reusable API. Toolkit consumers therefore receive contradictory release notes and no component change entry warning them that their imports and migration wiring must change. Add the corresponding toolkit change file and update the unreleased Partner Chains changelog.
AGENTS.md reference: AGENTS.md:L197-L201
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Codex is stubborn and thinks #toolkit marker is for PC toolkit, but it is not.
7f99140 to
087b41f
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
b9d8fe4 to
5b2319f
Compare
MicroProofs
changed the title
💡 Codex Reviewmidnight-node/node/src/consensus_engine_dispatch.rs Lines 102 to 103 in 5b2319f Avoid resolving engines from unimported batch parents
When sync submits a contiguous multi-block batch, only the first block's parent is normally present in the client state; the parents of later blocks are earlier entries in the same batch. These runtime API queries therefore fail and default those later blocks to AURA, so post-flip BABE blocks are sent through the AURA verifier and rejected. Dispatch must classify blocks without requiring their in-batch parents to have already been imported, or process the batch sequentially across the engine boundary. midnight-node/node/src/babe_authoring.rs Line 100 in 5b2319f Subscribe before checking whether the flip occurred
If the flip block becomes best after the initial ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
MicroProofs
changed the title
693a1ba to
241f19c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 241f19c8cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut beefy_raw = [0u8; 33]; | ||
| beefy_raw[1..].copy_from_slice(&aura_raw); | ||
| SessionKeys { | ||
| babe: sp_core::sr25519::Public::from_raw(aura_raw).into(), | ||
| // Invalid SEC1 tag keeps the placeholder distinct from any real key. | ||
| beefy: sp_core::ecdsa::Public::from_raw(beefy_raw).into(), |
There was a problem hiding this comment.
On a v1 chain, this migration installs a BEEFY public key with the invalid SEC1 tag 0x00 for every validator. At the first session rotation those migrated QueuedKeys become the pallet_beefy authority set, but no private key can correspond to these values, so BEEFY cannot reach a signing quorum; even registrations already updated with real BEEFY keys do not become active until subsequent rotations, while registrations without BEEFY retain the same invalid fallback indefinitely. Preserve a signable legacy key (the existing cross-chain/BEEFY identity) or delay enabling the BEEFY session handler until real keys are active.
AGENTS.md reference: AGENTS.md:L69-L69
Useful? React with 👍 / 👎.
| pub aura: Aura, | ||
| pub grandpa: Grandpa, | ||
| pub babe: Babe, | ||
| // todo: add the beefy | ||
| // pub beefy: Beefy, | ||
| pub beefy: Beefy, |
There was a problem hiding this comment.
Adding this field changes the SCALE layout exposed through committee storage and the session-validator-management runtime API, but this commit leaves every file under metadata/static/ untouched. Earthfile:1101-1102 explicitly packages and compares midnight_metadata.scale, so downstream tooling using the checked-in artifact continues to see the schema without beefy and can misdecode four-key committee values; regenerate the repository metadata with this runtime change.
AGENTS.md reference: AGENTS.md:L146-L149
Useful? React with 👍 / 👎.
| db.reads(1) | ||
| } else { | ||
| let weight = <Inner as UncheckedOnRuntimeUpgrade>::on_runtime_upgrade(); | ||
| let weight = <Inner as OnRuntimeUpgrade>::on_runtime_upgrade(); |
There was a problem hiding this comment.
When try-runtime executes the configured AddBabeToSessionKeysMigration, this direct on_runtime_upgrade call runs only the inner migration body; the outer type has no pre_upgrade forwarding and its post_upgrade checks only the guard. Consequently all validation in v2.rs—including committee preservation, queue seeding, session-key conversion, and storage-version checks—is skipped for the actual Midnight runtime, so a corrupted migration can pass try-runtime as long as the guard is written. Delegate the inner pre/post state through this wrapper or wire the versioned migration directly.
Useful? React with 👍 / 👎.
Signed-off-by: microproofs <kwhitemsg@gmail.com>
241f19c to
58f2ff7
Compare
|
/bot rebuild-metadata |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58f2ff7c6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| grandpa: keys.grandpa_pubkey.into(), | ||
| // Fall back to the AURA key when no BABE key is configured (both sr25519). | ||
| babe: keys.babe_pubkey.unwrap_or(keys.aura_pubkey).into(), | ||
| beefy: keys.beefy_pubkey.into(), |
There was a problem hiding this comment.
When the node release notes are assembled, the new chain-spec behavior and the node crate's 2.1.1 version bump will have no node-component entry because this commit adds only runtime and toolkit change files. Add a corresponding changes/node/added/ or changes/node/changed/ entry so node consumers are notified that generated genesis session keys now include BEEFY.
AGENTS.md reference: AGENTS.md:L197-L201
Useful? React with 👍 / 👎.
Signed-off-by: microproofs <kwhitemsg@gmail.com>
…rsion Signed-off-by: microproofs <kwhitemsg@gmail.com>
58f2ff7 to
b5ba330
Compare
2 participants
Overview
Stacked on
feat-aura-to-babe-migration; two commits.Commit 1 — combine the migrations (pallet refactor, per prior review feedback: one migration must handle both the
QueuedCommitteeaddition and aSessionKeysshape change, and the pallet storage version must never go to 3).Replaces the pallet's pair of migrations (the queue-seeding
v2::V1ToV2Migrationand theFROM/TO-genericAuthorityKeysMigration) with a single combinedVersionedMigration<1, 2>inmigrations::v2that translatesCurrentCommittee/NextCommittee, seedsQueuedCommitteefrom the translated current committee, and upgradespallet_sessionkey storage in one gated step, ending at storage version 2.This fixes the base branch's wiring, which ran the seed before the key translation: the seed's typed read decodes old-shaped committee bytes with the current type, fails silently, and seeds an empty
QueuedCommittee— the next rotation would then handpallet_sessionan empty validator set. The runtime keeps Tomasz's structure and names:AddBabeToSessionKeysMigrationstays, still guard-gated onAddBabeSessionKeysMigrated(genesis-safe; the node-side committee decoders read it); only its inner migration is now the combined versioned step, and the separate seed leavesSingleBlockMigrations.The pallet migration also hardens the placeholder contract: keys of newly added types must be pairwise distinct per validator (a shared placeholder collapses
KeyOwneronto one validator; the next rotation'sset_keysfails withDuplicatedKeyfor everyone else, and those failures are currently swallowed, #1895). The try-runtimepost_upgraderound-tripsKeyOwnerover every key type of the new shape, and a rotation-count test pins activation timing: migrated placeholders serve the first two post-upgrade sessions; registered keys become active at the third rotation, no validator dropped.Commit 2 — add the BEEFY session key.
opaque::SessionKeysgainsbeefy, translated by the same cutover migration. Placeholder = the validator's aura bytes behind the invalid SEC1 tag0x00(distinct per validator, collides with no real key). Candidate registrations without a BEEFY key fall back to the same derivation, mirroring the base branch's BABE-from-aura fallback. Genesis takes the configuredbeefy_pubkey; dev seeds derive a properBeefyId.All live networks (devnet, qanet, preview, preprod, mainnet) were verified at on-chain pallet storage version 1 on 2026-08-13 via
state_getStorageof the storage-version key. Fresh-genesis chains start at version 2 and set the guard at genesis, so the migration no-ops there.Also includes
cargo fmtfixes for three node files on the base branch (whitespace only) so the fmt check passes.🗹 TODO before merging
📌 Submission Checklist
git commit -s) for the DCOchanges/runtime/changed/combined-session-migration.md,changes/runtime/changed/add-beefy-session-key.md)todo: add the beefymarker)🧪 Testing Evidence
pallet-session-validator-managementtests: 27 passed (v1 start state, seed assertion, re-run no-op, fresh-genesis no-op,KeyOwnerround-trip over added key types, rotation-count activation timing)midnight-node-runtimelib tests: 18 passed (includes the base branch's committee-decode tests)midnight-nodelib tests: 151 passedcargo check --workspaceclean;--features try-runtimecleancargo fmt --checkclean for all files in this diff🔱 Fork Strategy
SessionKeysshape change plus the cutover migration; enacted by the upgrade built from the base branch. Metadata rebuild needed after merge.Links
Issue: #1742
https://claude.ai/code/session_01SxL4B36LFRxeRX8HRyeSTy