Skip to content

Order LogView index keys so a range of entries is contiguous in storage - #6829

Draft
ndr-ds wants to merge 1 commit into
linera-io:mainfrom
ndr-ds:ndr-ds/logview-ordered-index-keys
Draft

ndr-ds wants to merge 1 commit into
linera-io:mainfrom
ndr-ds:ndr-ds/logview-ordered-index-keys

Conversation

@ndr-ds

@ndr-ds ndr-ds commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

⚠️ BREAKING STORAGE-FORMAT CHANGE — do not backport to a branch with live data without a migration. See "Release Plan".

Motivation

LogView::read(range) fetches a contiguous range of entries as one point read per entry. On the validator side this is what serves received_log pages, and it is the per-entry cost behind the PM restart storms: a 20,000-entry page becomes 20,000 keys, chunked by the ScyllaDB backend into ~203 CQL IN statements of 99 keys each (MAX_MULTI_KEYS, scylla_db.rs:63).

The reason it cannot do better today is the key encoding. derive_tag_key serializes the index with BCS, and BCS writes integers little-endian:

index key bytes
0 00000000
1 01000000
256 00010000
65536 00000100

Those sort as 0, 65536, 256, 1, … — so a contiguous range of indices is scattered across the whole key space. The 99 keys in each IN statement land on ~99 different pages of a multi-GB partition instead of 1–2 adjacent ones, and k >= start AND k < end cannot express the range at all.

This is not a new idea in the codebase: CustomSerialize exists precisely for this (common.rs, doc: "Sometimes we need a serialization that is different from the usual one and for example preserves order"), and BlockHeight already uses to_be_bytes(). LogView simply never used it.

Measured context on testnet_conway: one PM chain's received_log holds 4,466,673 entries, with one validator's sync tracker 3,871,738 entries behind. Walking that on restart drives validator statement-class IOPS from ~100/s to ~5,500/s and read p99 from 6 ms to 4.3 s for 20–60 minutes.

Proposal

Append the index big-endian, so key order matches index order and the entries of a range are adjacent in any store that keeps keys sorted.

fn append_index(mut key: Vec<u8>, index: u32) -> Vec<u8> {
    key.extend_from_slice(&index.to_be_bytes());
    key
}

Applied at the three sites that derive an entry key: pre_save, get, read_context.

Deliberately not included here: changing read_context to issue a range scan. The store trait exposes only prefix scans (find_keys_by_prefix / find_key_values_by_prefix), so a range read needs a new store API across every backend. That is a separate PR — and most of the win is already in the locality: adjacent keys mean each IN chunk reads ~1 storage chunk rather than ~99.

What this does not change

  • No hash changes. LogView::hash is computed over read(..) values (hasher.update_with_bcs_bytes(&elements)), never over keys. State hashes and consensus are unaffected.
  • key_value_store_view.rs has its own unrelated KeyTag::Index; it is untouched.
  • No API change: append_index is private and the public surface is identical.

Test Plan

Two unit tests in log_view.rs:

  • index_keys_sort_like_indices — sorting keys for [0, 1, 2, 255, 256, 257, 20_000, 65_536, u32::MAX] yields the indices in ascending order.
  • a_range_of_indices_is_a_contiguous_range_of_keys — every key in 1000..1100 lies between the range's first and last key, and the neighbours immediately outside sort outside it. This is the property that makes a page adjacent in storage, not merely ordered.

Verified the tests actually catch the bug rather than merely passing: flipping to_be_bytes back to to_le_bytes (today's behaviour) makes both fail; restoring it makes both pass.

$ cargo test -p linera-views --lib views::log_view
test result: ok. 2 passed; 0 failed

Not yet done, and worth doing before this is more than a draft: an end-to-end measurement of a large read(range) against a real ScyllaDB backend, to put a number on the improvement rather than an argument.

Release Plan

  • Backporting is not possible but we may want to deploy a new devnet and release a new SDK soon.

Every existing LogView becomes unreadable — entries written with LE keys are not found by BE lookups, and stored_count still claims they exist, so read fails with MissingEntries. This affects received_log, confirmed_log and every other LogView in chain state. There is no storage-format version constant in the repo to gate this on (checked: no STORAGE_VERSION / FORMAT_VERSION in linera-views or linera-storage).

For testnet_conway this needs either a migration pass that rewrites log keys, or acceptance that the data is re-synced — that decision is deliberately not part of this PR.

Links

ndr-ds added a commit that referenced this pull request Sep 17, 2026
…trackers fresh (#6830)

Supersedes #6632 — same change, rebased onto current `main` (the
original branched 2026-07-28 and was 81 commits behind), plus a defaults
fix and a test found by review.

## Motivation

`ChainClient::find_received_certificates` is the **only** writer of the
per-validator received-certificate trackers, and its only callers are
the chain listener's one-shot background sync and
`synchronize_from_validators` (whose non-test callers are CLI paths,
`linera-bridge`, the GraphQL `sync` mutation and the web client). A
running worker's inbox loop calls `process_inbox_without_prepare`, which
bypasses both.

So **the trackers advance only at process restart**, and every restart
walks the entire backlog accumulated since the previous one. Confirmed
in production on `testnet_conway`:

- `linera_find_received_certificates_latency_count` on a PM worker: **2
calls at restart, zero in the following 75 minutes**.
- The `info!` line `Starting background certificate sync` appears **8
times, all within the same second**, at restart.
- On one live chain, `receivedLog.count` = 4,466,673 with one
validator's tracker at 594,935 — a **3,871,738-entry** backlog armed for
the next restart.

Walking that drives validator statement-class IOPS from ~100/s to
~5,500/s and read p99 from 6 ms to 4.3 s for 20–60 minutes, which in
turn stalls chains on every worker in the fleet.

## Proposal

Repeat the background sync on an interval instead of running it once, so
a restart only walks what accumulated since the last refresh.

```rust
loop {
    if let Err(e) = Self::background_sync_received_certificates(...).await { warn!(...) }
    if interval.is_zero() { return; }
    sleep(interval).await;
}
```

New knob `--listener-background-sync-interval-ms` /
`LINERA_LISTENER_BACKGROUND_SYNC_INTERVAL_MS`, default 900 000 (15 min);
`0` restores the one-shot behaviour. The loop is sequential, so syncs
cannot overlap or pile up.

At the rates measured on that chain, 15 minutes bounds each walk to
~500–7 000 entries instead of ~4.5 M — three to four orders of magnitude
smaller, and small enough that it never reaches the paging threshold in
#6631.

### Beyond the original PR

Review flagged that `#[derive(Default)]` produced
`background_sync_interval_ms = 0` — "never repeat" — disagreeing with
both the clap and serde defaults of 900 000. Not a live defect
(production configs come from clap), but a trap: any future
`..Default::default()` caller would silently opt out of the periodic
sync.

- `Default` is now written out and uses the same value as clap and
serde.
- `linera benchmark`, the one reachable `..Default::default()` site, now
sets `background_sync_interval_ms: 0` **explicitly**, preserving its
current behaviour — repeating the sync there would add unrelated
validator traffic to the measurement.

## Test Plan

`background_sync_interval_defaults_agree` asserts all three defaults —
clap, serde and `Default` — are 900 000. Mutation-verified: setting the
`Default` back to `0` makes it fail with `left: 0, right: 900000`;
restoring it passes.

```
$ cargo test -p linera-client --lib background_sync_interval_defaults_agree
test result: ok. 1 passed; 0 failed
$ cargo check -p linera-service     # the benchmark call site
    Finished
```

Reviewed with the adversarial pre-PR flow against this rebased branch:
23 agents, 0 errors, 9 candidate findings, all 9 refuted with executed
evidence.

**Not covered:** no test exercises a *second* iteration of the loop —
the existing listener tests all construct the config via `Default`,
which after this change means a 15-minute interval, so they still only
ever observe the first sync. Driving a second iteration needs a test
clock in the listener harness; worth doing, called out rather than
silently skipped.

## Release Plan

- These changes should be backported to the latest `testnet` branch,
then
    - be released in a validator hotfix.

The restart storms are live on `testnet_conway` today. Note the
**first** sync after deploying still walks the whole accumulated backlog
— schedule that rollout deliberately.

## Links

- [reviewer
checklist](https://github.com/linera-io/linera-protocol/blob/main/CONTRIBUTING.md#reviewer-checklist)
- Supersedes #6632.
- Complementary: #6631 paces a long walk; #6629 makes one visible at
`info`; #6829 makes each entry of any walk cheap (it also covers the
cold-takeover case this PR cannot help, where a chain reassigned to a
new worker starts from tracker 0).
ndr-ds added a commit that referenced this pull request Sep 17, 2026
Supersedes #6629 — same change, rebased onto current `main` (the
original branched 2026-07-28 and was 81 commits behind), plus two fixes
from review.

## Motivation

`ChainClient::find_received_certificates` can run for tens of minutes
walking millions of received-log entries from validators, and at
`RUST_LOG=info` — what production workers run — it is **completely
silent**: all of its progress logging is `debug!`/`trace!`.

That silence is the main reason a multi-hour validator ScyllaDB storm
took days to attribute. From the logs alone you could not tell which
chain was syncing, how far along it was, or how much was left. The
diagnosis eventually had to come from a Prometheus histogram plus a
per-pod log-line census.

## Proposal

Report start, progress and completion at `info` for syncs large enough
to matter:

- start, progress every 100 000 certificates, and a completion line with
elapsed seconds;
- gated on a 50 000-certificate threshold, so ordinary syncs stay quiet.

For the 3.87 M-entry backlog measured on `testnet_conway` that is
roughly 40 lines for the whole walk, not one per page.

### Beyond the original PR

**Per-validator backlog sizes in the start line.** The walk is
per-validator and the sizes differ by orders of magnitude — on a live
chain the four internal validators were 686–2 813 entries behind while
one external was **3 871 738** behind. A single aggregate number hides
exactly the validator whose walk is expensive. The start line now
carries `entries_per_validator` keyed by address (falling back to the
public key), plus the entry total alongside the certificate count.

The address lookup is done inside the `is_large_sync` branch, so the
quiet path allocates nothing.

**Threshold unit corrected in the docs.** The constant's doc comment
said "Number of received-log **entries**", but the code compares
`received_logs.num_certs()`. Entries and certificates are different
quantities — the entry total is summed over validators — and a doc that
names the wrong unit is worse than none, especially in an investigation
where those two numbers were confused. The comment now says
certificates, and the log line reports both so neither has to be
inferred.

## Test Plan

`cargo check -p linera-core` passes on current `main`.

Behaviour verified by reading, not by execution: the logging is gated on
`is_large_sync`, the progress counter advances by
`received_log.num_certs()` per batch and re-arms with
`next_progress_message = num_synced_certs + INTERVAL` (so it cannot
drift or fire twice for one batch), and nothing was added inside a
per-entry loop.

Honest gap: there is **no automated test** that a large sync emits these
lines — it would need a harness that drives `find_received_certificates`
past 50 000 certificates. Logging-only change, so I did not build one;
flagging rather than implying coverage.

## Release Plan

- These changes should be backported to the latest `testnet` branch,
then
    - be released in a validator hotfix.

Purely additive logging; no behavioural change.

## Links

- [reviewer
checklist](https://github.com/linera-io/linera-protocol/blob/main/CONTRIBUTING.md#reviewer-checklist)
- Supersedes #6629.
- Related: #6830 keeps the backlog small so these walks stay short;
#6631 paces a long walk; #6829 makes each entry of any walk cheap.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant