Ledger9 with no migration#1604
Conversation
Signed-off-by: Tomasz Bartos <tomasz.bartos@shielded.io>
Signed-off-by: Lech Głowiak <lech.glowiak@shielded.io>
Signed-off-by: Lech Głowiak <lech.glowiak@shielded.io>
![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: 36decc6087
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| assert!( | ||
| l9_blocks.is_empty() || (l7_blocks.is_empty() && l8_blocks.is_empty()), | ||
| "chain has Ledger9 blocks and eariler version blocks. This is not supported yet!" |
There was a problem hiding this comment.
For a normal runtime upgrade, fetched history will contain Ledger8 blocks followed by spec 001_001_000 blocks mapped to Ledger9; build_fork_aware_context_raw starts from the first block's version and this replay path then has both l8_blocks and l9_blocks populated. This assertion makes toolkit commands that replay across the 1.1.0 upgrade (for example dust balance, transaction generation, or cache rebuilds) abort instead of using the added 8→9 fork conversion and replaying the Ledger9 tail.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Autoformatted arrays of bytes is one change, but the relevant one is that I've updated system_tx by deserializing one present in this file, mapping it to Ledger 9 transaction and serializing again.
Signed-off-by: Lech Głowiak <lech.glowiak@shielded.io>
Signed-off-by: Lech Głowiak <lech.glowiak@shielded.io>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78219b6fe3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // runtime in substitute for the on-chain Wasm runtime unless all of `spec_name`, | ||
| // `spec_version`, and `authoring_version` are the same between Wasm and native. | ||
| spec_version: 001_000_000, | ||
| spec_version: 001_001_000, |
There was a problem hiding this comment.
With this spec_version bump, existing chains will execute the new runtime on top of a StateKey that was serialized as a Ledger8 TypedArenaKey, but the active runtime bridge now deserializes and loads it as Ledger9 (Bridge::get_ledger in ledger/src/versions/common/mod.rs). Since no runtime migration rewrites pallet_midnight::StateKey or converts the persisted ledger state before on_finalize calls post_block_update, normal upgrades from 1.0.0 will fail to load the ledger state and stop producing blocks; this needs a migration or the spec bump should be withheld from upgradeable chains.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a8f6bbab7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let prover: Arc< | ||
| dyn midnight_node_ledger_helpers::ledger_8::ProofProvider< | ||
| midnight_node_ledger_helpers::ledger_8::DefaultDB, | ||
| >, | ||
| > = Arc::new(midnight_node_ledger_helpers::ledger_8::LocalProofServer::new()); |
There was a problem hiding this comment.
When fork_ctx.version() is Ledger8 and the user provided --proof-server, this branch ignores prover_config and always constructs a LocalProofServer; before this change the Ledger8 path used the configured prover via make_prover. Toolkit transaction generation against pre-1.1/Ledger8 sources will silently do local proving instead of using the requested remote proof server, which can make runs fail or become unexpectedly expensive on machines that rely on remote proving.
Useful? React with 👍 / 👎.
Signed-off-by: Lech Głowiak <lech.glowiak@shielded.io>
Signed-off-by: Lech Głowiak <lech.glowiak@shielded.io>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7476dabe0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ForkAwareLedgerContext::Ledger8(_) => { | ||
| log::debug!("Skipping cache save: context is still on ledger 8"); | ||
| return; |
There was a problem hiding this comment.
When the fetched source has not reached spec 1.1.0 yet, build_fork_aware_context_cached still replays into a Ledger8 context and then calls this function at the real head, but this branch returns before writing either the ledger snapshot or wallet states. That means toolkit commands against pre-upgrade Ledger8 chains with cache storage configured will do a full replay every run and never refresh the per-wallet cache that existed before this change, which is a significant regression for long histories even though Ledger8 builders are still supported elsewhere in this PR.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
genesis files for networks we're not resetting should not change (i.e. all networks other than devnet)
There was a problem hiding this comment.
We have tests that check that current code is able to create chain-specs for some 5 or 6 networks
| ARG NETWORK | ||
| ARG GENERATE_TEST_TXS=false | ||
| # ledger-9 migration WIP: toolkit-js still emits intent[v6] (ledger-8). | ||
| # Disabled by default until util/toolkit-js gets a v9 variant. |
There was a problem hiding this comment.
I'm working on a toolkit-js update that will allow this - we'll need a new compactc version, PR is here: https://github.com/LFDT-Minokawa/compact/pull/459/changes
4 participants
Overview
Add Ledger9 alpha.1 support.
At this point it allows new chain to start in the local-environment.
🗹 TODO before merging
📌 Submission Checklist
git commit -s) for the DCO🧪 Testing Evidence
Please describe any additional testing aside from CI:
🔱 Fork Strategy
Links
#1579