Skip to content

[proto] Remove redundant RPC GetLastCommittedBlockNumber()#155

Merged
liran-funaro merged 1 commit into
hyperledger:mainfrom
liran-funaro:coord-remove-rpc
Nov 19, 2025
Merged

[proto] Remove redundant RPC GetLastCommittedBlockNumber()#155
liran-funaro merged 1 commit into
hyperledger:mainfrom
liran-funaro:coord-remove-rpc

Conversation

@liran-funaro

Copy link
Copy Markdown
Contributor

Type of change

  • Improvement (improvement to code, performance, etc)

Description

  • Remove redundant RPC in the coordinator: GetLastCommittedBlockNumber() - It was already covered by GetNextExpectedBlockNumber()
  • Remove unnecessary stream check in GetNextExpectedBlockNumber() - The sidecar verifies this anyway before calling this method
  • Align API with VC - Replaced GetLastCommittedBlockNumber() with GetNextExpectedBlockNumber()
  • Adjust relevant tests

Related issues

@liran-funaro liran-funaro requested a review from cendhu October 22, 2025 10:05

@cendhu cendhu 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.

My suggestion is to use the name GetLastCommittedBlockNumber instead of GetNextExpectedBlockNumber. This also aligns with the existing SetLastCommittedBlockNumber. The caller (i.e., the sidecar) can then derive the next expected block number from the last committed block number.

@liran-funaro

Copy link
Copy Markdown
Contributor Author

@cendhu The reason GetLastCommittedBlockNumber() was preferred is that it does not require special care when there are no blocks committed.
But I understand the readability issue you raised.

@cendhu

cendhu commented Nov 12, 2025

Copy link
Copy Markdown
Contributor

@cendhu The reason GetLastCommittedBlockNumber() was preferred is that it does not require special care when there are no blocks committed. But I understand the readability issue you raised.

Yes, when I read, I was thinking that GetNextExpectedBlockNumber is the one that is expected by the coordinator which is one greater than the last received block. Later, I understood it is returning the last committed block + 1. We can rename it to GetNextToBeCommittedBlockNumber or GetNextBlockNumberToCommit and still retain the removal of the extra proto message.

@liran-funaro

Copy link
Copy Markdown
Contributor Author

I opted for GetNextBlockNumberToCommit()

@liran-funaro liran-funaro requested a review from cendhu November 12, 2025 08:20
@cendhu

cendhu commented Nov 19, 2025

Copy link
Copy Markdown
Contributor

Resolve conflicts. Otherwise, LGTM.

Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
@liran-funaro liran-funaro merged commit 312d6e9 into hyperledger:main Nov 19, 2025
12 checks passed
@liran-funaro liran-funaro deleted the coord-remove-rpc branch November 19, 2025 09:24
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.

[proto] Unused RPC in the Coordinator: GetLastCommittedBlockNumber

2 participants