Skip to content

fix: safely allow unbounded mappings in cnight observation pallet#1423

Open
ozgb wants to merge 19 commits into
mainfrom
oscar-cnight-observation-mapping-storage-split
Open

fix: safely allow unbounded mappings in cnight observation pallet#1423
ozgb wants to merge 19 commits into
mainfrom
oscar-cnight-observation-mapping-storage-split

Conversation

@ozgb
Copy link
Copy Markdown
Contributor

@ozgb ozgb commented Apr 27, 2026 •
edited
Loading

Overview

Replaces the single Mappings storage map with a new Mapping double storage map

We were storing mappings for each Cardano address in an unbounded Vec - every time we add/remove a registration this Vec is loaded into memory - if it gets too big, this could cause allocation failures and liveness issues.

🗹 TODO before merging

  • Ready

📌 Submission Checklist

  • All commits are signed off (git commit -s) for the DCO
  • Changes are backward-compatible (or flagged if breaking)
  • Pull request description explains why the change is needed
  • Self-reviewed the diff
  • I have included a change file, or skipped for this reason:
  • If the changes introduce a new feature, I have bumped the node minor version
  • Update documentation (if relevant)
  • Updated AGENTS.md if build commands, architecture, or workflows changed
  • No new todos introduced

🧪 Testing Evidence

Please describe any additional testing aside from CI:

  • Additional tests are provided (if possible)
  • Existing tests pass
  • Tests added for migration

🔱 Fork Strategy

  • Node Runtime Update
  • Node Client Update
  • Other:
  • N/A

Links

Fixes https://github.com/midnightntwrk/midnight-security/issues/116

ozgb added 2 commits April 27, 2026 09:59
@ozgb
fix: allow unbounded mappings in cnight-observation pallet
f11abb6 
Uses two new maps to replace the previous single `Mappings` storage map - avoids encoding/decoding an unbounded vec in runtime

Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
@ozgb
docs: add change file
81a0049 
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
@ozgb ozgb requested a review from a team as a code owner April 27, 2026 09:11
@ozgb
docs: add pr link to change file
17a1e34 
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
@ozgb ozgb added the ai-assisted Created or modified with AI assistance label Apr 27, 2026
Klapeyron
Klapeyron reviewed Apr 27, 2026
View reviewed changes
Comment thread pallets/cnight-observation/src/migration.rs Outdated
@ozgb
feat: remove MappingCount - check count via iterator instead
6265bd6 
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
LGLO
LGLO reviewed Apr 27, 2026
View reviewed changes
Comment thread pallets/cnight-observation/src/lib.rs Outdated
LGLO
LGLO reviewed Apr 27, 2026
View reviewed changes
Comment thread pallets/cnight-observation/src/lib.rs Outdated
ozgb added 3 commits April 27, 2026 11:07
@ozgb
feat: add pre/post runtime migration checks in cnight observation
eee721d 
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
@ozgb
feat: add fatal error log
757037a 
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
@ozgb
refactor: use sidechain_domain::UtxoId to simplify MappingEntry struc…
14c239e 
…ture

Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
@ozgb
Copy link
Copy Markdown
Contributor Author

ozgb commented Apr 27, 2026

/bot rebuild-metadata

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 27, 2026

❌ Metadata rebuild failed. Check the workflow logs for details.

justinfrevert
justinfrevert reviewed Apr 27, 2026
View reviewed changes
Comment thread pallets/cnight-observation/src/migration.rs Outdated
Klapeyron
Klapeyron reviewed Apr 28, 2026
View reviewed changes
Comment thread pallets/cnight-observation/src/migration.rs Outdated
ozgb added 7 commits April 28, 2026 10:33
@ozgb
refactor: convert cnight-observation v0->v1 migration to multi-block
9338914 
Replaces the OnRuntimeUpgrade-style migration with a SteppedMigration so
v0 -> v1 storage drains incrementally over many blocks rather than in
the upgrade block. Per-step weight is bounded by MAX_ENTRIES_PER_ADDR
(observed worst case across networks). Storage version is bumped
inline on completion since pallet_migrations does not bump it for us.

Wires the migration into pallet_migrations::Config::Migrations and
removes it from the executive Migrations tuple. Tests drive
SteppedMigration::step directly against the existing mock.

Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
@ozgb
fix: revert changes to genesis cnight format
e05b492 
These changes came from switching the pallet to use sidechain_domain::UtxoId

Changed genesis to keep the previous utxo_tx_hash and utxo_index split

Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
@ozgb
chore: cargo fmt
ba8fbc3 
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
@ozgb
test: make mock DbWeight settable and cover meter-driven migration paths
d08c03f 
The mock previously used `type DbWeight = ()`, which yields zero per-step
weight and prevents exercising the SteppedMigration paths that depend on
WeightMeter pressure. Replace with a `parameter_types! pub static` so the
default is non-zero and tests can override per-test via MockDbWeight::set.

Adds two tests now feasible against the v0->v1 migration: that an empty
meter surfaces InsufficientWeight, and that step returns a resume cursor
when the meter exhausts mid-migration.

Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
@ozgb
test: fix test data (string -> int)
f69780e 
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
@ozgb
fix: increase headroom for max_entries_per_addr
af75ee4 
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
@ozgb
test: fix step weight in test
7142d96 
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
@ozgb
Copy link
Copy Markdown
Contributor Author

ozgb commented Apr 28, 2026

/bot rebuild-metadata

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 28, 2026

✅ Metadata rebuild complete! Changes have been committed.

@ozgb ozgb closed this Apr 28, 2026
@ozgb ozgb reopened this Apr 28, 2026
ozgb added 2 commits May 1, 2026 23:20
@ozgb
fix: add gate in multi-block-migration to prevent state corruption
84fc809 
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
@ozgb
Merge branch 'main' into oscar-cnight-observation-mapping-storage-split
cbd304f 
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
@ozgb
Copy link
Copy Markdown
Contributor Author

ozgb commented May 5, 2026

/bot rebuild-metadata

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 5, 2026

✅ Metadata rebuild complete! Changes have been committed.

@ozgb ozgb closed this May 5, 2026
@ozgb ozgb reopened this May 5, 2026
@ozgb
test: fix e2e tests not compiling
3789ca8 
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
@Klapeyron
Copy link
Copy Markdown
Contributor

Klapeyron commented May 11, 2026

/bot rebuild-metadata

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 11, 2026

✅ Metadata rebuild complete. No changes detected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LGLO LGLO LGLO left review comments

@Klapeyron Klapeyron Klapeyron left review comments

@justinfrevert justinfrevert justinfrevert approved these changes

Assignees

No one assigned

Labels

ai-assisted Created or modified with AI assistance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ozgb @Klapeyron @LGLO @justinfrevert