feat: ICP-ledger: FI-1440: Implement V4 for ICP ledger - migrate balances to stable structures #3314
Conversation
| Err(e) => eprintln!("Could not write ledger_stable_memory.txt: {}", e), | ||
| }; | ||
|
|
||
| let ledger: ledger_canister::Ledger = match serde_cbor::from_reader(File::open(cbor).unwrap()) { |
There was a problem hiding this comment.
I guess this tool is not used very often, as this would fail since https://dashboard.internetcomputer.org/proposal/133946 was executed. The ledger stable memory now contains the stable structures memory manager and cannot be directly deserialized to the Ledger struct. Also, the balances are no longer stored in the Ledger struct directly but in BALANCES_MEMORY instead. The reason I am removing it only now is that the balances().store below no longer provides an iter(). I suggest we remove this code now and reimplement it later if needed.
There was a problem hiding this comment.
Thanks a lot for this, @maciejdfinity ! I made a first pass and it looks very good! I have the following comments that weren't related to any changed code, so I'm leaving them here:
- Should we consider slightly bumping the
MAX_INSTRUCTIONS_PER_UPGRADEvalue, in case that would allow us to perform the whole migration as part of the post_upgrade? - Could we update the
rs/ledger_suite/icp/tests/golden_nns_state.rstest in this PR also as in the previous one for the ICRC ledger? When run from this PR branch it's currently failing: https://dash.idx.dfinity.network/invocation/9562aced-5ecc-44d4-a4f3-6d262efb51f6 - We should potentially wait with upgrading the ledger until NNS1-3526 has been addressed (see this Slack thread)
Co-authored-by: Mathias Björkqvist <mathias.bjorkqvist@dfinity.org>
Thank you for checking this!
I bumped it to 250B. When running the golden state test I noticed that we need ~225B, but I think we can add some margin.
Done!
Good point, let's wait! |
There was a problem hiding this comment.
If this pull request affects the behavior of any canister owned by
the Governance team, remember to update the corresponding
unreleased_changes.md file(s).
To acknowldge this reminder (and unblock the PR), dismiss this
code review by going to the bottom of the pull request page, and
supply one of the following reasons:
-
Done.
-
No canister behavior changes.
The fix was apparently already part of this PR for NNS1-3496, so we should be good to go! |
|
No canister behavior changes. |
See https://dfinity.slack.com/archives/C0175CHJ0P6/p1737022510708759 |
No canister behavior changes.
This version of the ledger migrates the balances stored by the ledger from heap to stable memory stable structures. This allows the ledger to store more data (heap memory is limited to 4GB) and reduces the number of instructions used during the upgrades - there is no need to serialize and deserialize the balances stored in heap memory. Since we are no longer limited by the heap size, the logic for trimming balances and related init/upgrade arguments (
maximum_number_of_accounts,accounts_overflow_trim_quantity) were removed. The migration to stable structures is performed during the upgrade and, if 250B instruction limit is reached, using timer invocations after the upgrade. If timer invocations are used by the migration, the ledger is unavailable for transfers and approvals until migration is finished. The migration status can be checked by calling theis_ledger_readyendpoint. After migration is finished, it is no longer possible to downgrade the ledger to the previous version .