experiment: combine tx revalidation cache (#744) and live intra-block ledger states (#2050) - #2087
chrispalaskas wants to merge 24 commits into
Conversation
Remove the soft transaction cache and introduce tx revalidation: cached VerifiedTransaction entries are reused when state changes by revalidating against a RevalidationReference instead of re-running full ZK proof verification. Adds cache metrics (miss, strict hit, revalidation hit) and tests covering the full validation lifecycle.
Signed-off-by: Michał Skowron <michal.skowron@shielded.io>
Signed-off-by: Michał Skowron <michal.skowron@shielded.io>
Signed-off-by: Michał Skowron <michal.skowron@shielded.io>
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
DCO Remediation Commit for Michał Skowron <michal.skowron@iohk.io> I, Michał Skowron <michal.skowron@iohk.io>, hereby add my Signed-off-by to this commit: 0009021 Signed-off-by: Michał Skowron <michal.skowron@iohk.io>
DCO Remediation Commit for Michał Skowron <michal.skowron@shielded.io> I, Michał Skowron <michal.skowron@shielded.io>, hereby add my Signed-off-by to this commit: f684d77 I, Michał Skowron <michal.skowron@shielded.io>, hereby add my Signed-off-by to this commit: 86ba80a Signed-off-by: Michał Skowron <michal.skowron@shielded.io>
Reconcile the revalidation-based validation cache with main's `tblock` correction (#1924). The cache value now carries the effective `well_formed` timestamp alongside the ledger state, and an entry is only served as a strict hit when both match. A differing timestamp revalidates instead, which re-runs exactly the two time-dependent checks — intent TTL and the dust validity window, both under the ledger's `param_check(always = true)` — without redoing the ZK work. Without this, a mempool entry verified at `parent + slot_duration * (1 + MaxSkippedSlots)` would strict-hit `pre_dispatch` and `apply_transaction` at block start, letting a transaction enter a block having only ever been checked against a future timestamp, and making block import depend on local mempool cache contents. The correction itself is threaded through to both `well_formed` call sites so historical blocks below `tblock_correction_disable_after` still import; the mempool path stays uncorrected, as its block context is already skewed. Revalidation hits now also dry-run the guaranteed segment. `well_formed` never checks applicability and `RevalidationReference` skips the stateless checks, so an already-applied transaction revalidated clean and survived in the pool until the producing node's `pre_dispatch` rejected it. Also drops a duplicate `midnight-primitives-ledger` dev-dependency that both sides added to pallets/midnight in different syntaxes, which left the workspace manifest unloadable. Assisted-by: Claude:claude-opus-5 Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
Every new block import now re-validates cached entries, including a dry-run of `apply`, so relay nodes evict stale transactions from the mempool without needing an unconditional time-based eviction. TTI still caps memory growth on quiet chains. Assisted-by: Claude:claude-opus-5 Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
A cache entry records what `well_formed` proved about a transaction at a given state and timestamp, not a validity verdict, so a rejection does not falsify it. Dropping the entry only forced the next attempt to redo the ZK verification to reach the same conclusion. Remove the three invalidation sites — the revalidation failure in `get_verified_transaction` and the dry-run failures in `do_validate_transaction` and `do_validate_guaranteed_execution` — leaving the cache with no invalidation path at all: entries are evicted by capacity or TTI only. Every read still either strict-hits an identical state and timestamp or re-runs the checks that can change, so no stale verdict can survive. Keeping entries for rejected and already-applied transactions is the point rather than a leak: a reorg that returns one to the pool revalidates it instead of re-verifying it from scratch, which matters more as forks become routine. Assisted-by: Claude:claude-opus-5 Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
Head 3dd295e. Merged into main (28eb943) first, deliberately: main is #744's only merge base (3edc676), so the merge is a clean 3-way. Merging it after #2050 instead produces a second merge base (14ec098, inherited from #1443's old history on #2050's branch) and the recursive strategy's virtual base then generates ~90 spurious conflicts across CI workflows, the Earthfile and old genesis files. Six real conflicts, all main's STRICT/SOFT validation caches versus #744's single revalidation-based TX_VALIDATION_CACHE. #744's cache is the whole point of the PR, so its side wins in each — but not verbatim, because main has since replaced the tblock model #744 was written against: - #744 computes the effective `well_formed` tblock from a runtime-configured `TBlockCorrection` (offset + `disable_after` cutoff) read out of a `TBlockCorrectionExt` externality. That is the #1964 design. - #2031 replaced it on main with `well_formed_tblock(ledger, block_context, skew_tblock: bool)`, gating the correction on the host-function version (v1 skews a block's first transaction, v2 does not) with a compile-time offset, and **deleted `TBlockCorrection`/`TBlockCorrectionExt` outright**. So keeping main's model was required, not preferred. `tblock_correction: Option<&TBlockCorrection>` becomes `skew_tblock: bool` through `get_verified_transaction` and `do_validate_guaranteed_execution`, the `TBlockCorrectionExt` lookup in `apply_transaction` is dropped in favour of the plumbed-through flag, and the mempool path's `None` becomes `false` (same meaning: `validate_unsigned` already skewed the block context it passes, so no correction applies there — which is what main does too). Main's now-dead `strict_cache_key` and its `strict_cache_key_separates_corrected_from_uncorrected` test are removed along with the `StrictTxValidationKey` they built. The invariant that test protected — the two host-function versions must not share an entry when they verify at different timestamps — is preserved and now documented on `TxValidationKey`: `runtime_version` is part of the key, and on top of that a tblock that does not match the entry's routes to `revalidate_transaction` rather than strict-hitting. `pallets/midnight/Cargo.toml` needed `midnight-primitives-ledger` restored as a dev-dependency (the line-level merge dropped it). #744 wanted it to register `TBlockCorrectionExt`; here it is for `LedgerMetrics` / `LedgerMetricsExt`, so the cache tests can assert on the counters. Comment updated accordingly. Assisted-by: Claude:claude-opus-5 Signed-off-by: chrispalaskas <chris.palaskas@gmail.com>
Applied as a squashed three-way patch of 8d3e0cf..1da3723 (PR #2050 head) rather than a git merge. #2050's branch carries #1443's old history, which gives it a second merge base against this branch (14ec098); the recursive strategy's virtual base then produces ~39 spurious conflicts across CI workflows, the Earthfile and old genesis files. Forcing a single base with `-s resolve` picks the wrong one (14ec098), which silently mis-merges files neither PR touches. Patching against #2050's true base is the only trustworthy option. The patch itself applies with ZERO conflicts against main + #744 — the adjacency conflicts seen on the #1872-based branch were entirely #1872's proof-verification cache sitting where #2050 inserts its keep-alive block. #744 and #2050 touch disjoint parts of `versions/common/mod.rs`: #744 owns the tx-validation cache, #2050 the persist/keep-alive contract. Two adaptations: - `Cargo.lock` regenerated rather than taken from #2050. The only real change is moka 0.11.3 -> 0.12.15, which #2050 needs for `Cache::and_compute_with`. - `pallets/midnight/Cargo.toml`: #2050's `test-utils` feature compiles `mock.rs` into the lib (for the persist_refcount integration test), where dev-dependencies are unavailable, so #744's metrics and spec-version mocks break the build. `sp-version`, `prometheus-endpoint` and `midnight-primitives-ledger` are promoted to optional dependencies enabled by `test-utils`. This is the same interaction ca3e1ab fixed on the #1872-based branch — it is a #744 <-> #2050 interaction, so it survives dropping #1872. Also narrowed `warp_ledger_sync::storage_key_tests::raw_storage_keys_match_the_runtime`, added by main's #2012 after #2050 forked. It pinned two hand-built raw storage keys to the runtime's derived keys; #2050 deletes `state_key_storage_key` and has `read_state_key` derive that key from `pallet_midnight::StateKey::<Runtime>::hashed_key()` directly, so a rename there is now a compile error and the assertion had nothing left to guard. The `PreForkStateKey` half is still hand-built and still asserted. Assisted-by: Claude:claude-opus-5 Signed-off-by: chrispalaskas <chris.palaskas@gmail.com>
…eight Two independent limits closed a block well before the node ran out of execution capacity, which would mask what #744 and #2050 are meant to improve: - `frame_system::BlockLength` was `max_with_normal_ratio(1 MiB, 75%)`, i.e. ~786 KiB usable for normal dispatches. Midnight transactions are large relative to a typical Substrate extrinsic, so blocks hit the length limit rather than the weight limit. Now 5 MiB. - `ConfigurableTransactionSizeWeight` shared `DefaultWeight` with `ConfigurableOnInitializeWeight` and `ConfigurableOnRuntimeUpgradeWeight`, so its default was `EXTRA_WEIGHT_TX_SIZE` — 20 ms ref-time, ~1% of a 2 s block — added flat to every transaction on top of its real gas-metered cost, and it was the dominant term. It now gets its own `#[pallet::type_value]` returning `Weight::zero()`. `DefaultWeight` and the `get_tx_weight` fallback for an unmeterable transaction are deliberately unchanged and still use `EXTRA_WEIGHT_TX_SIZE`. This changes exactly the flat per-transaction add-on and nothing else; the value stays settable at runtime via the root-only `set_tx_size_weight` extrinsic. `spec_version` 002_001_000 -> 002_001_001. Both values are metadata-visible (`BlockLength` is a `frame_system` constant, the weight default is a storage-entry default), so runtime metadata needs rebuilding via `/bot rebuild-metadata`. Assisted-by: Claude:claude-opus-5 Signed-off-by: chrispalaskas <chris.palaskas@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b827cb6063
ℹ️ 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: 002_001_000, | ||
| spec_version: 002_001_001, |
There was a problem hiding this comment.
Teach the fetcher to accept runtime version 2.1.1
This bumps every newly produced block's MNSV digest to 002_001_001, but util/toolkit/src/fetcher/runtimes.rs::RuntimeVersion::try_from only recognizes 002_001_000. Consequently, FetchTask::fetch_block now returns UnsupportedBlockVersion(2001001) for every block produced after this upgrade, preventing toolkit/indexer block fetching; map the new spec version to compatible metadata or add a corresponding runtime variant.
Useful? React with 👍 / 👎.
| if matches!(cache_outcome, TxValidationCacheOutcome::StrictCacheHit) { | ||
| return Ok(cache_outcome); |
There was a problem hiding this comment.
Do not skip dry-runs for unproven strict cache hits
A cache entry is inserted immediately after well_formed, before validate_guaranteed_execution runs, and it remains cached when that dry-run rejects the transaction. If the same well-formed but inapplicable transaction is submitted again at the same state and timestamp, this branch treats the entry as a strict hit and returns success without repeating the failed applicability check, admitting the invalid transaction to the pool. Only skip the dry-run when the cache also records a previous successful dry-run, or always perform it here.
Useful? React with 👍 / 👎.
This comment has been minimized.
This comment has been minimized.
Raising spec_version to 002_001_001 in this branch left the toolkit's block fetcher unable to read the chain this same ref produces: RuntimeVersion's TryFrom<u32> knows a closed set of spec versions, so every block failed with UnsupportedBlockVersion(2001001). Observed on perfnet immediately after the regenesis wipe -- `midnight-node-toolkit fetch -s ws://<validator>:9944` dies on the first block, and with it every fetch-path caller (fund_wallets.py, register_dust.py, generate_txs_round_robin.py, tx_load_applier.py). Only send_batch_txs.py, which takes neither --fetch-cache nor --ledger-state-db, was unaffected. Add a V2_1_1 variant mapped to 002_001_001. It reuses the 2.1.0 subxt metadata snapshot: this spec bump changes only frame_system::BlockLength (a constant) and the default of ConfigurableTransactionSizeWeight (a storage-entry default), and neither touches the extrinsic envelope or the event types process_block_with_protocol decodes -- the same reasoning under which MidnightMetadata2_0_0 already reuses the 1.0.0 snapshot. When the runtime metadata is rebuilt for this spec (the branch's outstanding /bot rebuild-metadata item), the variant should bind its own metadata/static/midnight_metadata_2.1.1.scale. The variant is appended last, so RuntimeVersion::latest_version() -- an iter().max() over the derived Ord -- still resolves to the newest runtime. LedgerVersion::from_spec_version needs no change: it matches the open range 002_000_000.. onto Ledger9, which already covers 2_001_001. The only exhaustive match on RuntimeVersion is the one in compute_task.rs updated here; client.rs's get_state_root_at has a catch-all arm. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: chrispalaskas <chris.palaskas@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6236adaec2
ℹ️ 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".
| // struct and cannot be transformed to match. We have no way to derive | ||
| // the pure `LedgerState` typed key without the runtime API, so skip | ||
| // verification for V0_21_0 blocks. | ||
| Ok(None) |
There was a problem hiding this comment.
Preserve state-root verification for 0.21 blocks
When the toolkit replays historical V0.21 blocks, returning None here silently disables LedgerContext::verify_state_root, so a divergent replay is accepted. The previous StorableLedgerState implementation reconstructed the old Ledger { state, block_fullness: 0 } layout specifically to compare against the raw Midnight::StateKey; retain that version-specific calculation and raw storage query rather than dropping verification for the entire supported runtime version.
Useful? React with 👍 / 👎.
| // 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: 002_001_000, | ||
| spec_version: 002_001_001, |
There was a problem hiding this comment.
Rebuild and register the 2.1.1 runtime metadata
The spec version is bumped to 2.1.1 while metadata/src/lib.rs:17-20 still exposes the 2.1.0 snapshot as midnight_metadata_latest, and no 2.1.1 snapshot or refreshed midnight_metadata.scale is included. Because this change also modifies the BlockLength constant and a pallet storage default, shipped metadata consumers remain out of sync with the runtime; run the repository metadata rebuild and register the new version before shipping.
AGENTS.md reference: AGENTS.md:L144-L149
Useful? React with 👍 / 👎.
Overview
Combined test branch for perfnet A/B runs, integrating the two caching PRs that we
believe do help, plus the two throughput-limit changes needed to reach the regime where
they can show anything:
skowron/tx-revalidation, head3dd295e, incl. the tblock + stale-check fixes202ba1d4,d1d6cc9f,624b3f55)ozgb-ledger-intermediate-states-less-persists, head1da3723e)Batch proof verification (#1872) is deliberately excluded. It was measured and did not
help, so this branch drops it — which also means dropping its
midnight-ledgerrev pin.The ledger dependency here is main's
ledger-9.1.0.0-rc.4tag.This supersedes #2041 as the perf-test vehicle. Not intended to merge into
mainas-is.Why this is much cleaner than #2041
Removing #1872 removes essentially all of the semantic merge work. On #2041 the hard part
was that #1872 and #744 had both rewritten the same caching layer in
ledger/src/versions/common/mod.rs, and #2050 then landed adjacent to #1872'sproof-verification cache. With #1872 gone, #744 and #2050 touch disjoint parts of that
file — #744 owns the tx-validation cache, #2050 owns the persist/keep-alive contract —
and #2050 applies with zero conflicts.
Net diff vs
main: 22 files, +1379 / −474.How the branch was assembled
Merge of #744 (commit
3613dd7)Merged into
mainfirst, deliberately.mainis #744's only merge base(
3edc67697), so this is a clean 3-way merge. Merging it after #2050 instead produces asecond merge base (
14ec09884, inherited from #1443's old history, which #2050's branchstill carries) and the recursive strategy's virtual base then generates ~90 spurious
conflicts across CI workflows, the Earthfile and old genesis files.
Six real conflicts, all
main's STRICT/SOFT validation caches versus #744's singlerevalidation-based
TX_VALIDATION_CACHE. #744's cache is the point of the PR, so its sidewins in each — but not verbatim, because
mainhas since replaced the tblock model#744 was written against:
well_formedtblock from a runtime-configuredTBlockCorrection(offset +disable_aftercutoff), read out of aTBlockCorrectionExtexternalitymain(#2031)well_formed_tblock(ledger, block_context, skew_tblock: bool), gating the correction on the host-function version (v1 skews a block's first transaction, v2 does not) with a compile-time offset#2031 deleted
TBlockCorrection/TBlockCorrectionExtoutright, so keepingmain'smodel was required, not preferred. Concretely:
tblock_correction: Option<&TBlockCorrection>→skew_tblock: boolthroughget_verified_transactionanddo_validate_guaranteed_execution.TBlockCorrectionExtlookup inapply_transactionis dropped in favour of the flag that is already a parameter there.Nonebecomesfalse— same meaning (validate_unsignedalready skewed the block context it passes, so no correction applies), and the same thingmaindoes.main's now-deadstrict_cache_keyand itsstrict_cache_key_separates_corrected_from_uncorrectedtest are removed along with theStrictTxValidationKeythey built. The invariant that test protected is preserved —the two host-function versions must not share a cache entry when they verify at different
timestamps — and is now documented on
TxValidationKey:runtime_versionis part of thekey, and on top of that a tblock that does not match the entry's routes to
revalidate_transactionrather than strict-hitting. #744's owntest_tblock_correction_not_applied_by_the_current_runtimeandtest_tblock_correction_does_not_affect_mempool_validationboth pass againstmain's model.pallets/midnight/Cargo.tomlneededmidnight-primitives-ledgerrestored as adev-dependency — the line-level merge dropped it. #744 wanted it to register
TBlockCorrectionExt; here it is forLedgerMetrics/LedgerMetricsExtso the cache testscan assert on the counters.
Application of #2050 (commit
194ec5b)Applied as a squashed three-way patch of
8d3e0cf4..1da3723erather than agit merge,for the criss-cross reason above. Forcing a single base with
-s resolveis not ausable workaround: it picks
14ec09884, the wrong one, and silently mis-merges filesneither PR touches (it reported conflicts in
primitives/mainchain-follower, which isoutside both PRs' diffs). Patching against #2050's true base is the only trustworthy option.
The patch applies with zero conflicts. Two adaptations:
Cargo.lockregenerated rather than taken from feat(node): keep intra-block ledger states live instead of persisting them #2050 — the only real change is moka 0.11.3 → 0.12.15, which feat(node): keep intra-block ledger states live instead of persisting them #2050 needs forCache::and_compute_with.pallets/midnight/Cargo.toml: feat(node): keep intra-block ledger states live instead of persisting them #2050'stest-utilsfeature compilesmock.rsinto the lib (for thepersist_refcountintegration test), where dev-dependencies are unavailable, so feat: replace soft tx cache with revalidation-based validation cache #744's metrics and spec-version mocks break the build.sp-version,prometheus-endpointandmidnight-primitives-ledgerare promoted to optional dependencies enabled bytest-utils. This is a genuine feat: replace soft tx cache with revalidation-based validation cache #744 ↔ feat(node): keep intra-block ledger states live instead of persisting them #2050 interaction — it is the one adaptation from experiment: combine batch proof verification (#1872), tx revalidation cache (#744) and live intra-block ledger states (#2050) #2041 (ca3e1ab) that survives dropping experiment: batch verification #1872.Also narrowed
warp_ledger_sync::storage_key_tests::raw_storage_keys_match_the_runtime,which
mainadded in #2012 after #2050 forked. It pinned two hand-built raw storage keysto the runtime's derived keys; #2050 deletes
state_key_storage_keyand hasread_state_keyderive that key frompallet_midnight::StateKey::<Runtime>::hashed_key()directly, so a rename there is now a compile error and the assertion had nothing left to
guard. The
PreForkStateKeyhalf is still hand-built and still asserted.Block length and the flat per-transaction weight (commit
b827cb6)Two independent limits closed a block well before the node ran out of execution capacity,
which would mask what the two caching PRs are meant to improve:
frame_system::BlockLength1 MiB → 5 MiB. It wasmax_with_normal_ratio(1 MiB, 75%), i.e. ~786 KiB usable for normal dispatches. Midnight transactions are large relative to a typical Substrate extrinsic, so blocks were hitting the length limit, not the weight limit.ConfigurableTransactionSizeWeightnow defaults toWeight::zero(). It sharedDefaultWeightwithConfigurableOnInitializeWeightandConfigurableOnRuntimeUpgradeWeight, so its default wasEXTRA_WEIGHT_TX_SIZE— 20 ms ref-time, ~1% of a 2 s block — added flat to every transaction on top of its real gas-metered cost, and it was the dominant term. It now gets its own#[pallet::type_value].DefaultWeightand theget_tx_weightfallback for a transaction whose cost cannot bemetered are deliberately unchanged and still use
EXTRA_WEIGHT_TX_SIZE. This changesexactly the flat per-transaction add-on and nothing else; the value stays settable at
runtime via the root-only
set_tx_size_weightextrinsic, so a chain that already zeroed itby extrinsic sees no change from the new default.
spec_versionbumped002_001_000→002_001_001.🗹 TODO before merging
/bot rebuild-metadata—BlockLengthis aframe_systemconstant and the weight default is a storage-entry default, so both are metadata-visible and the checked-in snapshots are stale. Needed only for the throughput commit; feat: replace soft tx cache with revalidation-based validation cache #744 and feat(node): keep intra-block ledger states live instead of persisting them #2050 are both purely client-side.mainor stays a perf-test vehicleearthly +node-image) — local WASM build not verified (environment lacked clang)📌 Submission Checklist
git commit -s) for the DCOchanges/runtime/changed/block-length-5mib-zero-tx-size-weight.mdcovers the two throughput changes🧪 Testing Evidence
All native, with
SKIP_WASM_BUILD=1(WASM runtime build not run locally — clang unavailable in the assembly environment):cargo check --workspace --all-targets— cleancargo fmt --all -- --check— cleancargo test -p midnight-node-ledger— 83 passedcargo test -p pallet-midnight— 28 passed / 2 ignored, including feat: replace soft tx cache with revalidation-based validation cache #744's full cache suite againstmain's tblock model:test_validation_cache_strict_hit,test_validation_cache_revalidation_hit,test_validation_cache_miss_after_runtime_version_change,test_revalidation_hit_still_dry_runs_guaranteed_execution,test_tblock_correction_not_applied_by_the_current_runtime,test_tblock_correction_does_not_affect_mempool_validationcargo test -p pallet-midnight --features test-utils --test persist_refcount— 1 passed (feat(node): keep intra-block ledger states live instead of persisting them #2050'skeep_alive_invariants, the functional proof that the keep-alive caches behave alongside feat: replace soft tx cache with revalidation-based validation cache #744)cargo test -p midnight-node— 149 passed / 2 ignoredAdditional tests are provided (if possible)
🔱 Fork Strategy
#744 and #2050 are both client-side (#2050 explicitly has no storage-layout, host-ABI or
runtime change). The block-length and per-transaction-weight changes are a runtime update
and need a
setCode.Links
🤖 Generated with Claude Code