apollo_committer: add new request handler#14003
Conversation
3bb9c52 to
cd31e55
Compare
b6af15b to
cc78b2a
Compare
cd31e55 to
1525ea7
Compare
cc78b2a to
168b164
Compare
bc9526b to
7f3cc68
Compare
2e6e8c8 to
10eb928
Compare
7f3cc68 to
e791353
Compare
256a296 to
ec63490
Compare
e791353 to
b63892f
Compare
fa8519b to
b5a5268
Compare
e720964 to
9c253a8
Compare
9c253a8 to
8a3d6fb
Compare
b5a5268 to
20bdbc6
Compare
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 2 files and made 1 comment.
Reviewable status: 4 of 6 files reviewed, 3 unresolved discussions (waiting on ArielElp).
crates/apollo_committer_types/src/communication.rs line 68 at r5 (raw file):
RevertBlock(RevertBlockRequest), #[cfg(feature = "os_input")] ReadPathsAndCommitBlock(ReadPathsAndCommitBlockRequest),
Consider not adding a new variant, as the committer blocks commit_block API under os_input.
Code quote:
#[cfg(feature = "os_input")]
ReadPathsAndCommitBlock(ReadPathsAndCommitBlockRequest),
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs made 1 comment.
Reviewable status: 2 of 7 files reviewed, 2 unresolved discussions (waiting on ArielElp).
crates/apollo_committer_types/src/communication.rs line 68 at r5 (raw file):
Previously, yoavGrs wrote…
Consider not adding a new variant, as the committer blocks
commit_blockAPI underos_input.
Or set the commit block under not "os_input". WDYT?
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 7 files and all commit messages, and made 5 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on ArielElp and yoavGrs).
crates/apollo_committer_types/src/communication.rs line 68 at r5 (raw file):
Previously, yoavGrs wrote…
Or set the commit block under not "os_input". WDYT?
yes please, commented above also
crates/apollo_committer/Cargo.toml line 20 at r7 (raw file):
async-trait.workspace = true starknet_api.workspace = true starknet_committer = { workspace = true, features = ["os_input"] }
NO
this activates the feature in production!!
Code quote:
starknet_committer = { workspace = true, features = ["os_input"] }crates/apollo_committer/src/communication.rs line 25 at r7 (raw file):
for Committer<S, ForestDB> where ForestDB: ForestStorageWithEmptyReadContext<Storage = S> + ForestWriterWithMetadataAndWitnesses,
if the os_input feature is off, why add the ForestWriterWithMetadataAndWitnesses bound?
Code quote:
#[cfg(not(feature = "os_input"))]
#[async_trait]
impl<S: StorageConstructor, ForestDB> ComponentRequestHandler<CommitterRequest, CommitterResponse>
for Committer<S, ForestDB>
where
ForestDB: ForestStorageWithEmptyReadContext<Storage = S> + ForestWriterWithMetadataAndWitnesses,crates/apollo_committer/src/communication.rs line 59 at r7 (raw file):
self.read_paths_and_commit_block(req).await, ) }
@yoavGrs @ArielElp IIRC we said that, if the os_input feature is active, CommitBlock is replaced by ReadPathsAndCommitBlock. we shouldn't allow both
Code quote:
CommitterRequest::CommitBlock(commit_block_request) => {
CommitterResponse::CommitBlock(self.commit_block(commit_block_request).await)
}
CommitterRequest::RevertBlock(revert_block_request) => {
CommitterResponse::RevertBlock(self.revert_block(revert_block_request).await)
}
CommitterRequest::ReadPathsAndCommitBlock(req) => {
CommitterResponse::ReadPathsAndCommitBlock(
self.read_paths_and_commit_block(req).await,
)
}crates/apollo_committer_types/src/communication.rs line 80 at r7 (raw file):
RevertBlock(CommitterResult<RevertBlockResponse>), #[cfg(feature = "os_input")] ReadPathsAndCommitBlock(CommitterResult<ReadPathsAndCommitBlockResponse>),
Suggestion:
#[cfg(not(feature = "os_input"))]
CommitBlock(CommitterResult<CommitBlockResponse>),
RevertBlock(CommitterResult<RevertBlockResponse>),
#[cfg(feature = "os_input")]
ReadPathsAndCommitBlock(CommitterResult<ReadPathsAndCommitBlockResponse>),
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs made 1 comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on ArielElp).
crates/apollo_committer/src/communication.rs line 59 at r7 (raw file):
Previously, dorimedini-starkware wrote…
@yoavGrs @ArielElp IIRC we said that, if the
os_inputfeature is active,CommitBlockis replaced byReadPathsAndCommitBlock. we shouldn't allow both
In the last team sync, we said that ReadPathsAndCommitBlock is for desicion_reached, and CommitBlock is for syncing.
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware resolved 2 discussions.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on ArielElp).
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp made 3 comments.
Reviewable status: 3 of 7 files reviewed, 4 unresolved discussions (waiting on dorimedini-starkware).
crates/apollo_committer/src/communication.rs line 25 at r7 (raw file):
Previously, dorimedini-starkware wrote…
if the
os_inputfeature is off, why add theForestWriterWithMetadataAndWitnessesbound?
doesn't seem to make sense, reverted
(maybe it used to make sense in some earlier version of the stack when I tried to revert via write_with_metadata_and_witnesses always)
crates/apollo_committer/Cargo.toml line 20 at r7 (raw file):
Previously, dorimedini-starkware wrote…
NO
this activates the feature in production!!
Ups, reverted.
crates/apollo_committer_types/src/communication.rs line 80 at r7 (raw file):
RevertBlock(CommitterResult<RevertBlockResponse>), #[cfg(feature = "os_input")] ReadPathsAndCommitBlock(CommitterResult<ReadPathsAndCommitBlockResponse>),
pending resolution to the previous comment
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 4 files and all commit messages, made 1 comment, and resolved 4 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ArielElp).
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 65ddea4. Configure here.
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on ArielElp).
crates/apollo_batcher/src/commitment_manager/commitment_manager_impl.rs line 560 at r10 (raw file):
milliseconds." ); }
hmmmm I would reuse the same metrics for CommitBlock here, with a TODO to create separate metrics if and when the os_input feature reaches production
Code quote:
#[cfg(feature = "os_input")]
CommitterRequestLabelValue::ReadPathsAndCommitBlock => {
debug!(
"Read paths and commit block latency for block {height}: {task_duration} \
milliseconds."
);
}crates/apollo_batcher/src/commitment_manager/types.rs line 132 at r10 (raw file):
#[cfg(feature = "os_input")] CommitterRequestLabelValue::ReadPathsAndCommitBlock => return None, };
see above: why not reuse the timer for commit? we won't be mixing the two, right? once we sync, its read-paths-and-commit forever?
Code quote:
#[cfg(feature = "os_input")]
CommitterRequestLabelValue::ReadPathsAndCommitBlock => {}
}
}
/// Returns the duration of the task in milliseconds.
pub(crate) fn stop_timer(
&mut self,
task: CommitterRequestLabelValue,
height: BlockNumber,
) -> Option<u128> {
let map = match task {
CommitterRequestLabelValue::CommitBlock => &mut self.commit,
CommitterRequestLabelValue::RevertBlock => &mut self.revert,
#[cfg(feature = "os_input")]
CommitterRequestLabelValue::ReadPathsAndCommitBlock => return None,
};
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp made 2 comments and resolved 1 discussion.
Reviewable status: 9 of 11 files reviewed, 2 unresolved discussions (waiting on dorimedini-starkware).
crates/apollo_batcher/src/commitment_manager/commitment_manager_impl.rs line 560 at r10 (raw file):
Previously, dorimedini-starkware wrote…
hmmmm I would reuse the same metrics for
CommitBlockhere, with a TODO to create separate metrics if and when theos_inputfeature reaches production
Done.
crates/apollo_batcher/src/commitment_manager/types.rs line 132 at r10 (raw file):
Previously, dorimedini-starkware wrote…
see above: why not reuse the timer for
commit? we won't be mixing the two, right? once we sync, its read-paths-and-commit forever?
Reusing for now, but I don't think "forever" is accurate. You occasionally go back to sync, so it's not only a prefix of the blocks.
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 2 files and all commit messages, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ArielElp).
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp reviewed 11 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ArielElp).


No description provided.