DO NOT MERGE: feat(ledger): upgrade ledger 8 to 8.2.0-rc.1#1869
DO NOT MERGE: feat(ledger): upgrade ledger 8 to 8.2.0-rc.1#1869gilescope wants to merge 4 commits into
Conversation
Signed-off-by: Giles Cope <gilescope@gmail.com>
gilescope
added
skip-changes-check-issue
ai-assisted
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Giles Cope <gilescope@gmail.com>
Jina ReviewJina has completed this review. Review: https://app.usejina.com/reviews/77bfa622-d600-4a6c-a403-7d3ae5b5b4df
|
|
Now that's green, let's add in being able to sync with a ledger8 genesis blockchain... |
![jina-simulation[bot]](https://avatars.githubusercontent.com/u/97225700?s=60&v=4){kind=small}
There was a problem hiding this comment.
Runtime Review - Merge Readiness 5/5
0 issues found - Ready to merge
Ready to merge based on accepted issues: none were identified.
Investigation agents returned no findings, and the available source/diff review did not establish a grounded defect.
Final review: No candidate issues were produced.
Source and diff review found no additional publishable runtime issue.
Issues
No medium/high-confidence runtime issues were found.
Summary
- Status: warned
- Areas investigated: 6
- Tasks/probes performed: 0
- Issues reported: 0
- Publishable issues: 0
- Inline comments: 0
- File-level comments: 0
- Review comments: 0
- Unanchored publishable findings posted as file-level comments: 0
- Blocked validations: 0
- Dashboard: full runtime review
- Commit: cd43e00
- Changed files: 18
- Tool calls: none
![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: cd43e007d6
ℹ️ 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".
| # like midnight-storage-core 1.2.0 into two source-incompatible copies (E0277: trait `DB` not | ||
| # satisfied). Consequently L8 no longer shares coin-structure/base-crypto/transient-crypto/ | ||
| # serialize with L7 — each gets a dedicated `-ledger-8` entry pinned to the tag. | ||
| mn-ledger-8 = { git = "https://github.com/midnightntwrk/midnight-ledger", tag = "ledger-8.2.0-rc.1", package = "midnight-ledger" } |
There was a problem hiding this comment.
This bump brings Ledger 8 to the 8.2 split-phase API, but the version-specific validation shim is still on the old path: ledger/src/versions/guaranteed_validation/ledger_8.rs:16-35 says Ledger 8 has no apply_guaranteed_only and dry-runs the full state.apply(...). When a node/runtime is still using the Ledger8 bridge, mempool and pre-dispatch validation of a transaction with an expensive or failing fallible segment will still execute that fallible work even though acceptance only depends on the guaranteed segment, undermining the purpose of the 8.2 upgrade and leaving a avoidable validation-cost/DoS path. Please switch the Ledger8 shim to the new apply_guaranteed_only API as part of this dependency bump.
Useful? React with 👍 / 👎.
Signed-off-by: Giles Cope <gilescope@gmail.com>
Jina ReviewJina has completed this review. Review: https://app.usejina.com/reviews/c9ce91d1-6a99-4a94-b238-92520dd9050a
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e99718e5b9
ℹ️ 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".
| // L8 (git-8.2 storage-core) and L9 (crates.io storage-core) no longer share a storage arena, | ||
| // so the old in-place arena-key reinterpret is invalid. Move the value across by serializing in | ||
| // the L8 format and deserializing in L9, then allocate it into L9 storage. | ||
| let value: T2 = old_to_new_ser(&*t1)?; |
There was a problem hiding this comment.
When fork_context_8_to_9 calls this for the ledger state, this now serializes with ledger-8's tagged format and deserializes with ledger-9's tagged format. The state tags are intentionally different (ledger-state[v13] vs ledger-state[v18], as documented in ledger/src/versions/common/storage.rs), so any toolkit path that forks an L8 context to L9 will return a deserialization error before migration instead of producing the new context; this replacement for the old arena-key copy needs an untagged or otherwise version-aware conversion for types whose tag changed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Runtime Review - Merge Readiness 1/5
9 issues found - Strongly do not merge
Strongly do not merge.
High-confidence or high-likelihood findings affect the core Ledger-8 workflow: the runtime uses the Ledger-9 bridge for Ledger-8 genesis chains, potentially halting first-block finalization and synchronization; Ledger-8 synthetic fullness limits are bypassed across validators and oversized work is accepted with clamped fee accounting; and bridge failures can permanently commit inconsistent bookkeeping.
Additional medium-confidence correctness and data-i...
[truncated]
Final review: The review investigated Ledger-8 genesis dispatch and runtime bootstrap, transaction validation and fullness enforcement, persistent storage, toolkit proving and serialized-context replay, cross-version state conversion, host ABI compatibility, and bridge/system-transaction paths.
Source tracing and executable probes found: "Known-tag garbage is accepted by genesis dispatch and creates partial storage before failing"; "Ledger-8 transactions bypass the new 8.2 post-block fullness prevalidation"; "Ledger-8 accepts over-limit work and clamps fee accounting"; "Reloading saved toolkit transactions discards the Ledger-8 parent block hash"; "Ledger-8 genesis chains execute block hooks through the Ledger-9 bridge"; "Ledger-8 genesis chains still use the Ledger-9 host bridge"; "cNIGHT spend ownership is deleted when event construction fails"; "Bridge bookkeeping is committed when Ledger execution fails"; "File-loaded batches are reordered by timestamp instead of chain order"; and "Mixed-context transactions in one batch are silently replayed under the first context".
The Ledger-8 bridge and fullness issues must be addressed before merge because they can pre...
[truncated]
Issues
Inline comments posted
-
Known-tag garbage is accepted by genesis dispatch and creates partial storage before failing
Location:
ledger/src/lib.rs:188; Risk / impact: medium; Evidence confidence: medium; Production likelihood: low; Category: correctness; Validation: source tracegenesis_version::detect classifies any input containing a known serialization tag within its first 64 bytes as Ledger 8 or Ledger 9, without validating the complete tagged blob. A malformed input such as
midnight:ledger-state[v13]:garbagetherefore enters the Ledger-8 initializer instead of being rejected as malformed.
File-level or unanchored issues
-
Ledger-8 transactions bypass the new 8.2 post-block fullness prevalidation
Location:
ledger/src/versions/post_block_update/ledger_8.rs:30; Risk / impact: high; Evidence confidence: high; Production likelihood: medium; Category: correctness; Validation: hybridThe Ledger-8.2.0-rc.1 implementation provides LedgerState::prevalidate_post_block_update, but the repository's Ledger-8 adapter still returns Ok(()) unconditionally. Consequently, apply_verified_transaction and apply_system_tx never reject a transaction whose accumulated Ledger synthetic fullness exceeds configured limits.
-
Ledger-8 genesis chains execute block hooks through the Ledger-9 bridge
Location:
ledger/src/lib.rs:299; Risk / impact: high; Evidence confidence: medium; Production likelihood: high; Category: integration; Validation: hybridThe version-dispatched genesis path initializes v13 genesis data with Ledger 8, but active_ledger_bridge remains hardwired to ledger_9_bridge. The Midnight pallet uses this active bridge for every block's storage initialization and finalization.
-
Ledger-8 genesis chains still use the Ledger-9 host bridge
Location:
ledger/src/lib.rs:299; Risk / impact: high; Evidence confidence: medium; Production likelihood: high; Category: integration; Validation: source traceThe runtime-wide active_ledger_bridge is hardcoded to ledger_9_bridge. This alias is imported by cNIGHT observation, midnight-system governance, c2m bridge, and the Midnight pallet, while genesis_version separately initializes Ledger-8 storage for v13 genesis blobs.
-
Bridge bookkeeping is committed when Ledger execution fails
Location:
pallets/c2m-bridge/src/lib.rs:256; Risk / impact: high; Evidence confidence: medium; Production likelihood: medium; Category: data; Validation: source traceexecute_serialized_tx swallows both construction and execution errors. Callers mutate FRAME bookkeeping before or after this helper without receiving a failure result.
-
Reloading saved toolkit transactions discards the Ledger-8 parent block hash
Location:
util/toolkit/src/serde_def/transactions.rs:107; Risk / impact: medium; Evidence confidence: medium; Production likelihood: medium; Category: integration; Validation: source traceSourceTransactions::from_batches reads SerializedTx.context but only carries its timestamp into RawBlockData. RawBlockData::new_from_timestamp initializes parent_block_hash to zeroes, so the parent hash serialized by Ledger-8 transaction generation is lost during file reload.
-
cNIGHT spend ownership is deleted when event construction fails
Location:
pallets/cnight-observation/src/lib.rs:601; Risk / impact: medium; Evidence confidence: medium; Production likelihood: medium; Category: data; Validation: source tracehandle_spend removes UtxoOwners with take before calling the LedgerApi constructor. If Ledger-8 serialization or owner deserialization fails, the function logs the error and returns None without restoring the owner.
-
File-loaded batches are reordered by timestamp instead of chain order
Location:
util/toolkit/src/serde_def/transactions.rs:116; Risk / impact: medium; Evidence confidence: medium; Production likelihood: medium; Category: correctness; Validation: source traceSourceTransactions::from_batches sorts reconstructed blocks by tblock_secs. This discards the original batch order and can reverse valid chain history when timestamps regress. It also regenerates last_block_time_secs from the reordered predecessor.
-
Mixed-context transactions in one batch are silently replayed under the first context
Location:
ledger/helpers/src/fork/raw_block_data.rs:198; Risk / impact: medium; Evidence confidence: medium; Production likelihood: low; Category: correctness; Validation: source traceSerializedTxBatches::get_context validates only tblock equality. Transactions sharing a timestamp but carrying different parent hashes, timestamp error margins, or last-block times are accepted, and the first transaction's context is used for the whole batch.
Areas Investigated
- Version-aware genesis bootstrap and block production - 4 task(s), 1 issue(s) found
- Ledger-8 transaction admission, execution, and end-of-block consensus behavior - 3 task(s), 1 issue(s) found
- Ledger-8 persistent state integrity across database modes and restarts - 3 task(s), 0 issue(s) found
- Ledger-8 transaction generation, serialization, and proving interoperability - 4 task(s), 1 issue(s) found
- Cross-version ledger and wallet state conversion - 3 task(s), 0 issue(s) found
- Prove Ledger-8 genesis remains usable when the embedded runtime activates Ledger-9 host calls - 2 task(s), 1 issue(s) found
- Historical Ledger-8 block import and runtime-interface ABI compatibility - 4 task(s), 0 issue(s) found
- Quantify oversized Ledger-8 block acceptance and its consensus/fee effects - 4 task(s), 1 issue(s) found
- Ledger-8 system transactions across cNIGHT, governance, reserve, and runtime bridges - 3 task(s), 3 issue(s) found
- Make saved Ledger-8 context corruption conclusive across replay and proving - 3 task(s), 2 issue(s) found
Review Details
- Status: issues found
- Areas investigated: 10
- Tasks/probes performed: 33
- Issues found: 9
- Inline comments: 1
- File-level comments: 8
- Full investigation detail (areas, tasks, evidence): view on the Jina dashboard
- Commit: e99718e
- Changed files: 26
| /// is unreachable while that tie holds - it stays as self-healing dispatch | ||
| /// should the tags ever diverge. `None` if no known version matches | ||
| /// (corrupt or future-version blob). | ||
| pub fn detect(genesis_state: &[u8]) -> Option<LedgerVersion> { |
There was a problem hiding this comment.
Known-tag garbage is accepted by genesis dispatch and creates partial storage before failing
Risk: medium
Confidence: medium
Likelihood: low
Grounding: runtime review, source trace
What happens
genesis_version::detect classifies any input containing a known serialization tag within its first 64 bytes as Ledger 8 or Ledger 9, without validating the complete tagged blob. A malformed input such as midnight:ledger-state[v13]:garbage therefore enters the Ledger-8 initializer instead of being rejected as malformed.
Root cause
Detection is substring-based and initialization performs filesystem/database setup before validating the full genesis blob.
Why it matters
Corrupt genesis deployment or interrupted file delivery produces an abrupt startup panic and leftover partial state, complicating recovery and risking operators treating the database as initialized.
Evidence
- ledger/src/lib.rs:188-190 scans only the first 64 bytes for a substring.
- ledger/src/lib.rs:248-251 dispatches any v13-tagged input to Ledger-8 initialization.
- ledger/src/versions/common/storage.rs:140-145 creates the directory and opens ParityDB before deserialization.
- ledger/src/versions/common/storage.rs:151 calls alloc_with_initial_state, whose deserializer uses expect("failed to deserialize ledger genesis state").
- Shell-equivalent probe:
midnight:ledger-state[v13]:garbage -> V8;xxxxmidnight:ledger-state[v18]:garbage -> V9.
Reproduction or trace
Pass a byte buffer beginning with midnight:ledger-state[v13]: followed by invalid or truncated bytes to genesis_version::init_storage_paritydb_separate. detect returns V8, create_dir_all and ParityDb::open run, and tagged deserialization then panics.
Suggested fix
Validate the complete tagged genesis and return a structured error before opening or creating storage. Alternatively, make initialization fallible and deserialize before filesystem/database setup; avoid substring matching except as a preliminary non-authoritative hint.
There was a problem hiding this comment.
Ledger-8 transactions bypass the new 8.2 post-block fullness prevalidation
Risk: high
Confidence: high
Likelihood: medium
Grounding: runtime review, hybrid
What happens
The Ledger-8.2.0-rc.1 implementation provides LedgerState::prevalidate_post_block_update, but the repository's Ledger-8 adapter still returns Ok(()) unconditionally. Consequently, apply_verified_transaction and apply_system_tx never reject a transaction whose accumulated Ledger synthetic fullness exceeds configured limits.
Root cause
ledger/src/versions/post_block_update/ledger_8.rs was retained from the pre-8.2 compatibility shim. It assumes Ledger 8 has no prevalidation API, but Ledger 8.2.0-rc.1 adds prevalidate_post_block_update and an infallible apply_post_block_update.
Why it matters
Ledger-specific resource limits are bypassed even though FRAME weight checks remain satisfied. This permits oversized ledger blocks, distorts fee-price adjustment, and creates validator/accounting behavior inconsistent with the intended Ledger-8.2 limits.
Evidence
- Local ledger/src/versions/post_block_update/ledger_8.rs:30-37 returns Ok(()) without inspecting fullness.
- ledger/src/versions/common/api/ledger.rs:140-149 invokes this shim before applying every verified transaction.
- ledger/src/versions/common/api/ledger.rs:205-222 clamps fullness during on_finalize.
- Pinned Ledger-8.2.0-rc.1 upstream semantics.rs defines LedgerState::prevalidate_post_block_update and calls it from post_block_update.
- pallets/midnight/src/lib.rs:537-560 checks FRAME weight only, not Ledger synthetic fullness.
Reproduction or trace
A block with cumulative Ledger synthetic cost above configured limits follows: apply_verified_transaction -> Ledger-8 prevalidate_post_block_update -> unconditional Ok -> state.apply -> block_fullness accumulation -> apply_post_block_update -> clamp_and_normalize -> post_block_update. The upstream 8.2 API confirms the missing prevalidation step.
Suggested fix
Implement Ledger-8 prevalidate_post_block_update by normalizing the supplied fullness against block limits, computing overall fullness, and calling state.prevalidate_post_block_update. Change the Ledger-8 apply adapter to call the new infallible state.apply_post_block_update API after prevalidation.
View full runtime review audit trail
This issue references a file outside this PR diff, so Jina posted it on this changed file to make the thread resolvable.
There was a problem hiding this comment.
Ledger-8 genesis chains execute block hooks through the Ledger-9 bridge
Risk: high
Confidence: medium
Likelihood: high
Grounding: runtime review, hybrid
What happens
The version-dispatched genesis path initializes v13 genesis data with Ledger 8, but active_ledger_bridge remains hardwired to ledger_9_bridge. The Midnight pallet uses this active bridge for every block's storage initialization and finalization.
Root cause
Genesis version dispatch was added only to chain-spec root derivation and backend initialization; runtime-facing bridge selection remains compile-time latest-version selection.
Why it matters
The advertised Ledger-8 genesis compatibility can fail immediately after bootstrap. Nodes may appear to start but fail to produce or import subsequent blocks, preventing synchronization of immutable Ledger-8/v13 chains.
Evidence
- ledger/src/lib.rs:299 exports ledger_9_bridge as active_ledger_bridge.
- pallets/midnight/src/lib.rs:324 invokes active LedgerApi::ensure_storage_initialized during on_initialize.
- pallets/midnight/src/lib.rs:344 invokes active LedgerApi::apply_post_block_update during on_finalize.
- ledger/src/host_api/ledger_9.rs:480-499 checks Ledger-9 storage, drops all default storage, and registers Ledger-9 storage when absent.
- ledger/src/versions/common/mod.rs:294-310 loads and updates the state through the bridge's concrete typed ledger and default storage.
- The Ledger-8 fixture begins with midnight:ledger-state[v13].
- 1 more evidence item(s) recorded.
Reproduction or trace
Genesis initialization dispatches v13 to ledger_8::storage::init_storage_paritydb_*. The first runtime hook resolves active_ledger_bridge to ledger_9_bridge. Ledger-9 ensure_storage_initialized checks only Ledger-9's TypeId, calls drop_all_default_storage when absent, and installs Ledger-9 storage. on_finalize then calls Ledger-9 apply_post_block_update with the Ledger-8-derived StateKey.
Suggested fix
Select the runtime bridge based on the chain/genesis ledger version and preserve that selection through on_initialize, on_finalize, transaction, query, and RPC paths. The storage registration and state-key decoding must use the same Ledger version that created the genesis state. Add an integration test that boots the Ledger-8 fixture and finalizes several blocks while asserting changing readable StateKeys.
View full runtime review audit trail
This issue does not have a precise diff line, so Jina posted it as a file-level comment to make the thread resolvable.
There was a problem hiding this comment.
Ledger-8 genesis chains still use the Ledger-9 host bridge
Risk: high
Confidence: medium
Likelihood: high
Grounding: runtime review, source trace
What happens
The runtime-wide active_ledger_bridge is hardcoded to ledger_9_bridge. This alias is imported by cNIGHT observation, midnight-system governance, c2m bridge, and the Midnight pallet, while genesis_version separately initializes Ledger-8 storage for v13 genesis blobs.
Root cause
Genesis storage dispatch is version-aware, but runtime host API dispatch remains unconditionally newest-version based. There is no chain/genesis-version selection for active_ledger_bridge.
Why it matters
The stated PR goal of restoring bootstrap and synchronization for Ledger-8 genesis chains is not met. Privileged cNIGHT, governance, reserve, and bridge transactions cannot be trusted on those chains.
Evidence
- ledger/src/lib.rs:299 aliases active_ledger_bridge to ledger_9_bridge.
- node/src/backend/custom_parity_db.rs:112-123 dispatches genesis initialization by detected ledger version, including Ledger-8.
- pallets/cnight-observation/src/lib.rs:101-103 imports active_ledger_bridge as LedgerApi.
- pallets/midnight-system/src/lib.rs imports active_ledger_bridge as LedgerApi.
- pallets/midnight/src/lib.rs:324 calls LedgerApi::ensure_storage_initialized during on_initialize.
- ledger/src/host_api/ledger_9.rs:484-500 checks Ledger-9 storage and calls drop_all_default_storage before installing Ledger-9 storage.
Reproduction or trace
Ledger-8 genesis -> genesis_version::init_storage_paritydb_* -> Ledger-8 storage initialized -> runtime on_initialize -> active_ledger_bridge::ensure_storage_initialized -> Ledger-9 storage check/drop/install -> Ledger-9 APIs process Ledger-8 chain state.
Suggested fix
Select the host bridge and active version from the detected chain/genesis ledger version, or provide a runtime bridge that dispatches every operation to the matching Ledger-7/8/9 implementation and storage type.
View full runtime review audit trail
This issue does not have a precise diff line, so Jina posted it as a file-level comment to make the thread resolvable.
There was a problem hiding this comment.
Bridge bookkeeping is committed when Ledger execution fails
Risk: high
Confidence: medium
Likelihood: medium
Grounding: runtime review, source trace
What happens
execute_serialized_tx swallows both construction and execution errors. Callers mutate FRAME bookkeeping before or after this helper without receiving a failure result.
Root cause
The helper returns unit and only logs failures; state mutation is not coupled transactionally to the Ledger executor result.
Why it matters
Bridge transfers can be lost or become unrecoverable, and emitted success events are absent while FRAME bookkeeping claims the input was consumed.
Evidence
- pallets/c2m-bridge/src/lib.rs:263-285 logs errors and returns no failure.
- pallets/c2m-bridge/src/lib.rs:342 removes ApprovedMcTxHashes before calling the helper.
- pallets/c2m-bridge/src/lib.rs:362 generates/increments a nonce before calling the helper.
- pallets/c2m-bridge/src/lib.rs:305 kills SubminimalTransfers regardless of executor success.
Reproduction or trace
Configure the executor to return Err for a valid serialized bridge transaction; process an approved transfer or a threshold-crossing subminimal transfer; observe approval/accumulator removal without a Ledger state change.
Suggested fix
Return the executor result, propagate failure, and perform bookkeeping only after successful application, or wrap FRAME and Ledger updates in an explicit retry/idempotency protocol.
View full runtime review audit trail
This issue references a file outside this PR diff, so Jina posted it on this changed file to make the thread resolvable.
There was a problem hiding this comment.
Reloading saved toolkit transactions discards the Ledger-8 parent block hash
Risk: medium
Confidence: medium
Likelihood: medium
Grounding: runtime review, source trace
What happens
SourceTransactions::from_batches reads SerializedTx.context but only carries its timestamp into RawBlockData. RawBlockData::new_from_timestamp initializes parent_block_hash to zeroes, so the parent hash serialized by Ledger-8 transaction generation is lost during file reload.
Root cause
from_batches passes only context.tblock.to_secs() to RawBlockData::new_from_timestamp and does not copy context.parent_block_hash, context.tblock_err, or the original last-block context.
Why it matters
The serialized transaction format promises context preservation, but a save/reload workflow silently changes the context. This can make transactions generated from historical or remote contexts unusable after reload.
Evidence
- util/toolkit/src/tx_generator/source.rs:218-226 routes normal file loading to SourceTransactions::from_batches
- util/toolkit/src/serde_def/transactions.rs:91-112 extracts context but passes only tblock to RawBlockData::new_from_timestamp
- ledger/helpers/src/fork/raw_block_data.rs:139-148 sets parent_block_hash to [0u8; 32]
- ledger/helpers/src/fork/fork_aware_context.rs:186-191 rebuilds Ledger-8 BlockContext from RawBlockData.parent_block_hash
Reproduction or trace
SerializedTx.context.parent_block_hash -> SourceTransactions::from_batches -> RawBlockData::new_from_timestamp -> RawBlockData.parent_block_hash=[0;32] -> block_context_from_raw_8 -> Ledger-8 BlockContext.parent_block_hash=[0;32].
Suggested fix
Construct RawBlockData from the complete SerializedTx context, preserving parent_block_hash and tblock_err, or retain the serialized context alongside RawBlockData until version-specific context reconstruction.
View full runtime review audit trail
This issue references a file outside this PR diff, so Jina posted it on this changed file to make the thread resolvable.
There was a problem hiding this comment.
cNIGHT spend ownership is deleted when event construction fails
Risk: medium
Confidence: medium
Likelihood: medium
Grounding: runtime review, source trace
What happens
handle_spend removes UtxoOwners with take before calling the LedgerApi constructor. If Ledger-8 serialization or owner deserialization fails, the function logs the error and returns None without restoring the owner.
Root cause
State mutation precedes fallible system-transaction construction, and construction errors are converted into a successful no-op.
Why it matters
DUST supply and cNIGHT observation state can permanently diverge, violating exactly-once create/spend accounting.
Evidence
- pallets/cnight-observation/src/lib.rs:601 calls UtxoOwners::::take(nonce).
- pallets/cnight-observation/src/lib.rs:610-623 constructs the event and returns None on error.
- pallets/cnight-observation/src/lib.rs:727 advances NextCardanoPosition before system transaction construction.
- pallets/cnight-observation/src/lib.rs:748-750 logs construction failure without returning an error.
Reproduction or trace
Persist an owner for a nonce, submit the matching spend while LedgerApi::construct_cnight_generates_dust_event returns Err, and observe owner removal plus Cardano position advancement with no emitted Ledger system transaction.
Suggested fix
Construct and validate all event bytes before mutating ownership/position state, or propagate construction failure so the enclosing operation rolls back.
View full runtime review audit trail
This issue references a file outside this PR diff, so Jina posted it on this changed file to make the thread resolvable.
There was a problem hiding this comment.
File-loaded batches are reordered by timestamp instead of chain order
Risk: medium
Confidence: medium
Likelihood: medium
Grounding: runtime review, source trace
What happens
SourceTransactions::from_batches sorts reconstructed blocks by tblock_secs. This discards the original batch order and can reverse valid chain history when timestamps regress. It also regenerates last_block_time_secs from the reordered predecessor.
Root cause
from_batches has no block height or linkage metadata after converting SerializedTx values, so it uses timestamp sorting as a surrogate for chain order.
Why it matters
Replay, DUST validity-window handling, proving, and any resulting transaction generation can diverge from the original chain history.
Evidence
- util/toolkit/src/serde_def/transactions.rs:116 sorts blocks with sort_by_key(|b| b.tblock_secs).
- util/toolkit/src/serde_def/transactions.rs:121 derives last_block_time_secs from the sorted predecessor.
- util/toolkit/src/tx_generator/builder/mod.rs comments and replay code assume the supplied blocks are ordered by block number.
Reproduction or trace
Serialized batches A then B with tblock_secs A=100 and B=90 enter from_batches in [A,B], are sorted to [B,A], and are replayed in that order.
Suggested fix
Preserve input order for file-loaded batches, or carry and validate block height/parent linkage. Do not derive chain order from timestamp alone.
View full runtime review audit trail
This issue references a file outside this PR diff, so Jina posted it on this changed file to make the thread resolvable.
There was a problem hiding this comment.
Mixed-context transactions in one batch are silently replayed under the first context
Risk: medium
Confidence: medium
Likelihood: low
Grounding: runtime review, source trace
What happens
SerializedTxBatches::get_context validates only tblock equality. Transactions sharing a timestamp but carrying different parent hashes, timestamp error margins, or last-block times are accepted, and the first transaction's context is used for the whole batch.
Root cause
Batch context validation checks only one field and then collapses per-transaction contexts into a single timestamp-only RawBlockData.
Why it matters
A corrupted or manually composed saved file can produce context-bound proof failures or replay against the wrong reference block without an early, actionable validation error.
Evidence
- ledger/helpers/src/fork/raw_block_data.rs:197-205 compares only context.tblock and clones only the first context.
- util/toolkit/src/serde_def/transactions.rs:94-111 drops all context fields except tblock.
- ledger/helpers/src/fork/fork_aware_context.rs:186-191 constructs Ledger-8 BlockContext from the synthesized RawBlockData.
Reproduction or trace
Construct one batch with two serialized transactions sharing tblock but using distinct parent_block_hash values. get_context accepts it; from_batches uses the first context's timestamp and new_from_timestamp supplies a zero parent hash before replay context reconstruction.
Suggested fix
Require complete context equality for every transaction in a batch, or define batches as carrying one explicit context and reject per-transaction mismatches before conversion.
View full runtime review audit trail
This issue does not have a precise diff line, so Jina posted it as a file-level comment to make the thread resolvable.
gilescope
changed the title
1 participant
UPDATE: We'd prefer to move HF onwards and get to ledger 9 rather than upgrade 8.1.
Overview
Bumps the ledger-8 stack from 8.1.0 to
midnight-ledger8.2.0-rc.1 (picking upapply_guaranteed_only, infalliblepost_block_update, split-phase execution with deferred events, and the segment-type refactor), keeping ledger 9 as-is.8.2.0-rc.1 is not published to crates.io, so the ledger-8 crates are sourced from the
ledger-8.2.0-rc.1git tag. Because the tag carries path deps, ledger 8 no longer shares companion crates with ledger 7 — each companion gets a dedicated-ledger-8workspace entry pinned to the tag (a git-sourced 2.0.1 is a distinct type family from the crates.io 2.0.1, hence the separate conversion impl set inbuilders/mod.rs).Notable mechanics:
DefaultHasherinstead of hardcodedsha2::Sha256, so it tracks the git-8.2 storage-core's sha2 0.11 bump automatically.SerializedTxis generic over theBlockContextversion (SerializedTx<C = ledger_9::BlockContext>);extract_tx_with_context_ledger_8deserializes directly into an L8 context (the JSON wire shape is version-agnostic viaSerdeBlockContext), replacing the field-by-field rebuild.Follow up PRs:
🗹 TODO before merging
📌 Submission Checklist
git commit -s) for the DCOchanges/node/changed/ledger-8-2-upgrade.md🧪 Testing Evidence
cargo test -p midnight-node-ledger -p pallet-midnight— 93 + 28ledger_8::common::apitests green, includingshould_validate_transaction/should_apply_transactionwhich parse the realDEPLOY_TXfixture through the new directSerializedTx<ledger_8::BlockContext>deserialization and apply it to genesis state.cargo clippyclean acrossmidnight-node-ledger-helpers(--features can-panic) andmidnight-node-toolkit.Additional tests are provided (if possible)
🔱 Fork Strategy
Assisted-by: Claude:claude-fable-5