Add versioned migration to backfill commitment_nonce in pallet-storage-provider#270
Conversation
x3c41a
left a comment
x3c41a
left a comment
There was a problem hiding this comment.
Confirmed, independently traced the same root cause and verified the fix.
Root cause matches: #125 added commitment_nonce: u64 to BucketSnapshot with no storage version or migration. Since the snapshot is nested in Bucket.snapshot, every Buckets entry carrying a snapshot changed encoding, so pre-#125 entries (previewnet) became undecodable and broke try_decode_entire_state.
Verification:
- SCALE-decoded the exact
Bucketsvalue from the failing CI log: it fails against the current layout and decodes to zero bytes remaining against the pre-#125 layout (nocommitment_nonce), so the only delta is the missing field. - Ran the two migration tests on this branch locally, both pass (backfill on a snapshot-bearing bucket, and no-op preservation of a snapshot-less bucket).
commitment_nonce: 0is the right default: the field is only read byextend_checkpointto verify a late signature against the original signers' payload, and a snapshot predating the field was never eligible for that flow.- Gating as
VersionedMigration<0, 1>is correct: no-ops on fresh chains (genesis at v1), runs only where the pallet is still at v0.
LGTM.
|
Might be wrong but don't we also need to bump |
|
@RafalMirowski1 this PR only contains fixes on top of #125. in #125, it already bumped |
| // Backfill the `commitment_nonce` field added to `BucketSnapshot` by the | ||
| // challenge flow overhaul (#125). A real data transform, so it stays a | ||
| // `VersionedMigration` gated on the pallet's storage version. | ||
| pallet_storage_provider::migrations::v1::MigrateV0ToV1<Runtime>, |
There was a problem hiding this comment.
@danielbui12 for now this is ok, but for the future, if we have lots of Buckets, we need to do this by stepped multiblock migration
There was a problem hiding this comment.
yep, this migration only runs once, ever, there's no risk of it re-running later; we has yet to deploy it to mainnet/testnet. In previewnet, Bucket storage is still small now.
But do you want to apply multi-block to this?
There was a problem hiding this comment.
yep, this migration only runs once, ever, there's no risk of it re-running later; we has yet to deploy it to mainnet/testnet. In previewnet, Bucket storage is still small now.
But do you want to apply multi-block to this?
no need to, probably, before going live, we will set it to 1 and remove all the migrations :)
4 participants
Primary Changes
#[pallet::storage_version(STORAGE_VERSION)](v1) topallet-storage-provider, which previously had no storage version tracking at allpallet/src/migrations.rswith av1::MigrateV0ToV1migration that translates everyBucketslayout to the current one (after Challenge flow overhaul #125), defaulting the newcommitment_noncefield to0on existingBucketSnapshots nested inBucket.snapshotpallet_storage_provider::migrations::v1::MigrateV0ToV1<Runtime>into theMigrationstuple inruntimes/web3-storage-paseo/src/migrations.rs, following the same pattern already used forpallet-drive-registry's prior migrationcommitment_nonce: u64field toBucketSnapshotwithout a migration or storage version bump, leaving pre-existing on-chainBucketsentries on paseo undecodable by the current runtime (confirmed viatry-runtime on-runtime-upgradeagainst a previewnet); causing feat: migrate storage-client and provider-node to typed subxt bindings via storage-subxt #239 runtime migration check failed