Skip to content

apollo_consensus_orchestrator,apollo_batcher,apollo_batcher_types: drop accessed_keys from blob - #14808

Merged
yoavGrs merged 1 commit into
mainfrom
remove-accessed-keys-from-blob
Jul 27, 2026
Merged

apollo_consensus_orchestrator,apollo_batcher,apollo_batcher_types: drop accessed_keys from blob#14808
yoavGrs merged 1 commit into
mainfrom
remove-accessed-keys-from-blob

Conversation

@yoavGrs

@yoavGrs yoavGrs commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Remove the os_input-gated accessed_keys: AccessedKeys field from AerospikeBlob and its dead
plumbing: the matching BlobParameters field, the blob construction, and the now-unused
CentralObjects.accessed_keys (its only reader was the blob build). The batcher still computes
the block's accessed keys for the committer and for trimming initial_reads; only the blob no
longer carries them.

Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com

@reviewable-StarkWare

Copy link
Copy Markdown

This change is Reviewable

yoavGrs commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

@yoavGrs
yoavGrs force-pushed the remove-accessed-keys-from-blob branch from a3b312a to 7181c50 Compare July 15, 2026 07:33
@yoavGrs yoavGrs changed the title apollo_consensus_orchestrator,apollo_batcher,apollo_batcher_types: drop accessed_keys from AerospikeBlob apollo_consensus_orchestrator,apollo_batcher,apollo_batcher_types: drop accessed_keys from blob Jul 15, 2026
@yoavGrs
yoavGrs marked this pull request as ready for review July 15, 2026 08:31
@yoavGrs yoavGrs self-assigned this Jul 15, 2026
@cursor

cursor Bot commented Jul 15, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Narrow data-shape cleanup on os_input blob serialization; execution and storage of accessed keys in the batcher are unchanged.

Overview
Removes the os_input-gated accessed_keys field from the Aerospike/Cende blob path (AerospikeBlob, BlobParameters, orchestrator blob build) and from CentralObjects, which previously existed only to feed that blob.

The batcher still builds accessed keys on decision_reached for proposal commit / storage and to trim initial_reads before they are returned in CentralObjects; it no longer duplicates them on the decision response or passes an extra clone into the commitment task where ownership allows. Regression tests stop stripping accessed_keys when comparing Rust blob JSON to the Python reference.

Reviewed by Cursor Bugbot for commit c67243f. Bugbot is set up for automated code reviews on this repo. Configure here.

@yoavGrs
yoavGrs requested a review from itamar-starkware July 15, 2026 08:31
@yoavGrs
yoavGrs force-pushed the remove-accessed-keys-from-blob branch from 7181c50 to 15d5ccb Compare July 22, 2026 07:50
@yoavGrs
yoavGrs changed the base branch from main to graphite-base/14808 July 22, 2026 08:03
@yoavGrs
yoavGrs force-pushed the remove-accessed-keys-from-blob branch from 15d5ccb to ceea775 Compare July 22, 2026 08:03
@yoavGrs
yoavGrs force-pushed the graphite-base/14808 branch from 64a743e to 34fca2e Compare July 22, 2026 08:03
@yoavGrs
yoavGrs changed the base branch from graphite-base/14808 to accessed-keys-atomic-write July 22, 2026 08:03

@itamar-starkware itamar-starkware left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One constructor was missed by the removal: crates/central_systest_blobs/src/cende_blob_regression_test.rs:250 (impl From<BlockData> for BlobParameters) still initializes the deleted field, so the crate no longer compiles with the feature:

$ cargo check -p central_systest_blobs --features os_input --tests
error[E0560]: struct `BlobParameters` has no field named `accessed_keys`
  --> crates/central_systest_blobs/src/cende_blob_regression_test.rs:250:13

CI stays green only because apollo_storage_os_input_ci.yml doesn't build central_systest_blobs with os_input (the crate's feature forwards to apollo_consensus_orchestrator/os_input). Fix — drop the two stale lines:

             #[cfg(feature = "os_input")]
             recent_state_commitment_infos: vec![],
-            #[cfg(feature = "os_input")]
-            accessed_keys: Default::default(),
             #[cfg(feature = "os_input")]
             initial_reads: Default::default(),

Consider also adding central_systest_blobs to the os_input CI workflow (separate PR is fine) so the next blob-shape change can't break it silently.

Otherwise LGTM — verified the blob field had no remaining reader (the parity test always stripped it, i.e. the python schema never had it), the batcher still computes accessed keys locally for the committer and the initial_reads trim, and the clone() drop at batcher.rs:1011 is a strict win.

@itamar-starkware+AGNT made 1 comment.
Reviewable status: 0 of 5 files reviewed, all discussions resolved (waiting on yoavGrs).

@yoavGrs
yoavGrs changed the base branch from accessed-keys-atomic-write to graphite-base/14808 July 26, 2026 08:22

@itamar-starkware itamar-starkware left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at the comment, there is a spot you missed using accessed_keys in blob.

@itamar-starkware reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on yoavGrs).

@yoavGrs
yoavGrs force-pushed the graphite-base/14808 branch from 34fca2e to e51ca6a Compare July 26, 2026 08:48
@yoavGrs
yoavGrs force-pushed the remove-accessed-keys-from-blob branch from ceea775 to 6a2cdc5 Compare July 26, 2026 08:48
@yoavGrs
yoavGrs changed the base branch from graphite-base/14808 to accessed-keys-atomic-write July 26, 2026 08:48

@yoavGrs yoavGrs left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
And fixed the workflow file.

@yoavGrs made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on yoavGrs).

@itamar-starkware itamar-starkware left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

@itamar-starkware reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on yoavGrs).

…op accessed_keys from blob

Remove the os_input-gated `accessed_keys: AccessedKeys` field from `AerospikeBlob` and its dead
plumbing: the matching `BlobParameters` field, the blob construction, and the now-unused
`CentralObjects.accessed_keys` (its only reader was the blob build). The batcher still computes
the block's accessed keys for the committer and for trimming `initial_reads`; only the blob no
longer carries them.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

apollo_batcher: pass accessed_keys by move instead of clone in decision_reached

The clone became redundant once CentralObjects.accessed_keys was dropped: the write task was the
last consumer.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@yoavGrs
yoavGrs changed the base branch from accessed-keys-atomic-write to graphite-base/14808 July 26, 2026 14:00
@yoavGrs
yoavGrs force-pushed the remove-accessed-keys-from-blob branch from 6a2cdc5 to c67243f Compare July 26, 2026 14:00
@yoavGrs
yoavGrs force-pushed the graphite-base/14808 branch from e51ca6a to e23b028 Compare July 26, 2026 14:00
@yoavGrs
yoavGrs changed the base branch from graphite-base/14808 to main July 26, 2026 14:01

@yoavGrs yoavGrs left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yoavGrs made 1 comment.
Reviewable status: 1 of 7 files reviewed, 1 unresolved discussion (waiting on itamar-starkware).


.github/workflows/apollo_storage_os_input_ci.yml line 78 at r2 (raw file):

      # Check that the crate is compilable with the os_input feature. Not running the tests to
      # avoid the time-consuming GCS access.
      - run: cargo check -p central_systest_blobs --features os_input --tests

Compiling this crate is still heavy.
But, IMO no CI change needed: the merge queue runs cargo clippy --workspace --all-targets --all-features , which compiles central_systest_blobs's tests with os_input enabled.

@itamar-starkware itamar-starkware left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@itamar-starkware reviewed 6 files and all commit messages, and resolved 1 discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on yoavGrs).

@yoavGrs
yoavGrs added this pull request to the merge queue Jul 27, 2026
Merged via the queue into main with commit 5a1d8dc Jul 27, 2026
20 of 32 checks passed

Copy link
Copy Markdown
Contributor

Security scan complete — no issues detected.


Generated by Claude Code

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.

4 participants