Skip to content

Commit 19f50f4

Browse files
jsync-swirldsjsync-swirlds
committed
Adjust block access service to be correct without workarounds
* Modified the proto definition to remove unverified and prevent calling with both latest and a block number. * Added an "invalid" response and removed the verified/unverified responses * Removed unnecessary prefixes in the response enum names * Removed check for both fields set in Java (it's no longer possible). * Fixed up all references * Removed tests that set both fields as it's no longer possible (the last one set is the only one set). Signed-off-by: Joseph S <121976561+jsync-swirlds@users.noreply.github.com>
1 parent 0995038 commit 19f50f4

File tree

6 files changed

+77
-169
lines changed

6 files changed

+77
-169
lines changed

‎block-node/block-access/src/main/java/org/hiero/block/node/access/service/BlockAccessServicePlugin.java

Lines changed: 16 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
import static java.lang.System.Logger.Level.DEBUG;
55
import static java.lang.System.Logger.Level.ERROR;
6-
import static java.lang.System.Logger.Level.INFO;
6+
import static java.lang.System.Logger.Level.WARNING;
77

88
import com.hedera.hapi.block.stream.Block;
99
import com.hedera.pbj.runtime.grpc.GrpcException;
@@ -54,31 +54,18 @@ private BlockResponse handleBlockRequest(BlockRequest request) {
5454
requestCounter.increment();
5555

5656
try {
57-
// Log in case both block_number and retrieve_latest are set, this should not happen
58-
if (request.retrieveLatest() && request.blockNumber() != -1) {
59-
LOGGER.log(
60-
INFO,
61-
"Both block_number and retrieve_latest set. Using retrieve_latest instead of block_number: {0}",
62-
request.blockNumber());
63-
responseCounterNotFound.increment();
64-
return new BlockResponse(Code.READ_BLOCK_NOT_FOUND, null);
65-
}
66-
// when block_number is -1 and retrieve_latest is false, return an NOT_FOUND error
67-
if (request.blockNumber() == -1 && !request.retrieveLatest()) {
68-
LOGGER.log(INFO, "Block number is -1 and retrieve_latest is false");
69-
responseCounterNotFound.increment();
70-
return new BlockResponse(Code.READ_BLOCK_NOT_FOUND, null);
71-
}
72-
7357
long blockNumberToRetrieve;
58+
// The block number and retrieve latest are mutually exclusive in
59+
// the proto definition, so no need to check for that here.
7460

75-
// if retrieveLatest is set, get the latest block number
76-
if (request.retrieveLatest()) {
61+
// if retrieveLatest is set, or the request is for the largest
62+
// possible block number, get the latest block.
63+
if (request.retrieveLatestOrElse(false) || request.blockNumberOrElse(0L) == -1) {
7764
blockNumberToRetrieve = blockProvider.availableBlocks().max();
7865
if (blockNumberToRetrieve < 0) {
79-
LOGGER.log(INFO, "Latest block number not available");
66+
LOGGER.log(WARNING, "Latest available block number is not available, this should not be possible.");
8067
responseCounterNotAvailable.increment();
81-
return new BlockResponse(Code.READ_BLOCK_NOT_AVAILABLE, null);
68+
return new BlockResponse(Code.NOT_AVAILABLE, null);
8269
}
8370
} else {
8471
blockNumberToRetrieve = request.blockNumber();
@@ -95,18 +82,19 @@ private BlockResponse handleBlockRequest(BlockRequest request) {
9582
lowestBlockNumber,
9683
highestBlockNumber);
9784
responseCounterNotAvailable.increment();
98-
return new BlockResponse(Code.READ_BLOCK_NOT_AVAILABLE, null);
85+
return new BlockResponse(Code.NOT_AVAILABLE, null);
9986
}
10087

10188
// Retrieve the block
10289
Block block = blockProvider.block(blockNumberToRetrieve).block();
10390
responseCounterSuccess.increment();
104-
return new BlockResponse(Code.READ_BLOCK_SUCCESS, block);
91+
return new BlockResponse(Code.SUCCESS, block);
10592

10693
} catch (RuntimeException e) {
107-
LOGGER.log(ERROR, "Failed to retrieve block number: {0}", request.blockNumber());
108-
responseCounterNotFound.increment();
109-
return new BlockResponse(Code.READ_BLOCK_NOT_FOUND, null);
94+
String message = "Failed to retrieve block number %d.".formatted(request.blockNumber());
95+
LOGGER.log(ERROR, message, e);
96+
responseCounterNotFound.increment(); // Should this be a failure counter?
97+
return new BlockResponse(Code.NOT_FOUND, null);
11098
}
11199
}
112100

@@ -127,10 +115,10 @@ public void init(BlockNodeContext context, ServiceBuilder serviceBuilder) {
127115
.withDescription("Number of successful single block requests"));
128116
responseCounterNotAvailable = context.metrics()
129117
.getOrCreate(new Counter.Config(METRICS_CATEGORY, "single-block-requests-not-available")
130-
.withDescription("Number of single block requests that were not available"));
118+
.withDescription("Number of single block requests that responded not available"));
131119
responseCounterNotFound = context.metrics()
132120
.getOrCreate(new Counter.Config(METRICS_CATEGORY, "single-block-requests-not-found")
133-
.withDescription("Number of single block requests that were not found"));
121+
.withDescription("Number of single block requests that responded not found"));
134122
// Get the block provider
135123
this.blockProvider = context.historicalBlockProvider();
136124
// Register this service

‎block-node/block-access/src/test/java/org/hiero/block/node/access/service/BlockAccessServicePluginTest.java

Lines changed: 16 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -61,18 +61,15 @@ void testServiceInterfaceBasics() {
6161
@DisplayName("Happy Path Test, BlockAccessServicePlugin for an existing Block Number")
6262
void happyTestGetBlock() throws ParseException {
6363
final long blockNumber = 1;
64-
final BlockRequest request = BlockRequest.newBuilder()
65-
.blockNumber(blockNumber)
66-
.allowUnverified(true)
67-
.retrieveLatest(false)
68-
.build();
64+
final BlockRequest request =
65+
BlockRequest.newBuilder().blockNumber(blockNumber).build();
6966
toPluginPipe.onNext(BlockRequest.PROTOBUF.toBytes(request));
7067
// Check we get a response
7168
assertEquals(1, fromPluginBytes.size());
7269
// parse the response
7370
BlockResponse response = BlockResponse.PROTOBUF.parse(fromPluginBytes.get(0));
7471
// check that the status is success
75-
assertEquals(Code.READ_BLOCK_SUCCESS, response.status());
72+
assertEquals(Code.SUCCESS, response.status());
7673
// check that the block number is correct
7774
assertEquals(1, response.block().items().getFirst().blockHeader().number());
7875
}
@@ -81,74 +78,48 @@ void happyTestGetBlock() throws ParseException {
8178
@DisplayName("Negative Test, GetBlock for a non-existing Block Number")
8279
void negativeTestNonExistingBlock() throws ParseException {
8380
final long blockNumber = 1000;
84-
final BlockRequest request = BlockRequest.newBuilder()
85-
.blockNumber(blockNumber)
86-
.allowUnverified(true)
87-
.retrieveLatest(false)
88-
.build();
81+
final BlockRequest request =
82+
BlockRequest.newBuilder().blockNumber(blockNumber).build();
8983
toPluginPipe.onNext(BlockRequest.PROTOBUF.toBytes(request));
9084
// Check we get a response
9185
assertEquals(1, fromPluginBytes.size());
9286
// parse the response
9387
BlockResponse response = BlockResponse.PROTOBUF.parse(fromPluginBytes.get(0));
94-
// check that the status is NOT FOUND
95-
assertEquals(Code.READ_BLOCK_NOT_AVAILABLE, response.status());
88+
// check that the status is NOT AVAILABLE
89+
assertEquals(Code.NOT_AVAILABLE, response.status());
9690
// check block is null
9791
assertNull(response.block());
9892
}
9993

10094
@Test
10195
@DisplayName("Request Latest Block")
10296
void testRequestLatestBlock() throws ParseException {
103-
final BlockRequest request = BlockRequest.newBuilder()
104-
.blockNumber(-1)
105-
.allowUnverified(true)
106-
.retrieveLatest(true)
107-
.build();
97+
final BlockRequest request =
98+
BlockRequest.newBuilder().retrieveLatest(true).build();
10899
toPluginPipe.onNext(BlockRequest.PROTOBUF.toBytes(request));
109100
// Check we get a response
110101
assertEquals(1, fromPluginBytes.size());
111102
// parse the response
112103
BlockResponse response = BlockResponse.PROTOBUF.parse(fromPluginBytes.get(0));
113104
// check that the status is success
114-
assertEquals(Code.READ_BLOCK_SUCCESS, response.status());
105+
assertEquals(Code.SUCCESS, response.status());
115106
// check that the block number is correct
116107
assertEquals(24, response.block().items().getFirst().blockHeader().number());
117108
}
118109

119110
@Test
120-
@DisplayName("Request Latest and a specific Block different from latest, should fail with READ_BLOCK_NOT_FOUND")
121-
void testRequestLatestBlockDifferent() throws ParseException {
122-
final long blockNumber = 1;
123-
final BlockRequest request = BlockRequest.newBuilder()
124-
.blockNumber(blockNumber)
125-
.allowUnverified(true)
126-
.retrieveLatest(true)
127-
.build();
111+
@DisplayName("block_number is -1 - should return latest block")
112+
void testBlockNumberIsMinusOne() throws ParseException {
113+
final BlockRequest request = BlockRequest.newBuilder().blockNumber(-1).build();
128114
toPluginPipe.onNext(BlockRequest.PROTOBUF.toBytes(request));
129115
// Check we get a response
130116
assertEquals(1, fromPluginBytes.size());
131117
// parse the response
132118
BlockResponse response = BlockResponse.PROTOBUF.parse(fromPluginBytes.get(0));
133119
// check that the status is success
134-
assertEquals(Code.READ_BLOCK_NOT_FOUND, response.status());
135-
}
136-
137-
@Test
138-
@DisplayName("block_number is -1 and retrieve_latest is false - should return READ_BLOCK_NOT_FOUND")
139-
void testBlockNumberIsMinusOneAndRetrieveLatestIsFalse() throws ParseException {
140-
final BlockRequest request = BlockRequest.newBuilder()
141-
.blockNumber(-1)
142-
.allowUnverified(true)
143-
.retrieveLatest(false)
144-
.build();
145-
toPluginPipe.onNext(BlockRequest.PROTOBUF.toBytes(request));
146-
// Check we get a response
147-
assertEquals(1, fromPluginBytes.size());
148-
// parse the response
149-
BlockResponse response = BlockResponse.PROTOBUF.parse(fromPluginBytes.get(0));
150-
// check that the status is READ_BLOCK_NOT_FOUND
151-
assertEquals(Code.READ_BLOCK_NOT_FOUND, response.status());
120+
assertEquals(Code.SUCCESS, response.status());
121+
// check that the block number is correct
122+
assertEquals(24, response.block().items().getFirst().blockHeader().number());
152123
}
153124

154125
private void sendBlocks(int numberOfBlocks) {

‎protobuf/src/main/proto/org/hiero/block/api/block_access_service.proto

Lines changed: 25 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -13,50 +13,24 @@ import "stream/block.proto";
1313
* A request to read a single block.
1414
*
1515
* A client system SHALL send this message to request a single block,
16-
* including the block state proof.<br/>
17-
* A client MAY request that the block be sent without verification.
18-
* A compliant Block Node MAY respond to requests that allow unverified
19-
* responses by returning the full requested block before verifying
20-
* the included block proof.<br/>
21-
* A compliant Block Node MAY support _only_ requests that allow unverified
22-
* blocks, but MUST clearly document that limitation, and MUST respond to
23-
* a request that does not allow unverified blocks with the
24-
* `ALLOW_UNVERIFIED_REQUIRED` response code.
16+
* including the block state proof.
2517
*/
2618
message BlockRequest {
27-
/**
28-
* The block number of a block to retrieve.
29-
* <p>
30-
* The requested block MUST exist on the block node.<br/>
31-
* This value MUST NOT be set if `retrieve_latest` is set `true`.<br/>
32-
* This value MUST be set to a valid block number if `retrieve_latest` is
33-
* unset or is set `false`.
34-
*/
35-
uint64 block_number = 1;
36-
37-
/**
38-
* A flag to indicate that the requested block may be sent without
39-
* verifying its `BlockProof`.<br/>
40-
* This might be set by a client that expects to perform its own
41-
* verification and wishes lower latency or, potentially, lower cost.
42-
* <p>
43-
* If this value is set, then the responding Block Node MAY respond with a
44-
* block that has not completed verification of its `BlockProof`.<br/>
45-
* If this is _not_ set then the Block Node MUST respond with either a
46-
* fully verified and validated block, or `VERIFIED_BLOCK_UNAVAILABLE` if
47-
* the requested block is not yet verified.<br/>
48-
* The default value is _not set_.
49-
*/
50-
bool allow_unverified = 2;
19+
oneof block_specifier {
20+
/**
21+
* The block number of a block to retrieve.
22+
* <p>
23+
* The requested block MUST exist on the block node.
24+
*/
25+
uint64 block_number = 1;
5126

52-
/**
53-
* A flag to request the latest available block.
54-
* <p>
55-
* This value MAY be set `true` to request the last block available.<br/>
56-
* If this value is set to `true` then `block_number` MUST NOT be set and
57-
* SHALL be ignored.
58-
*/
59-
bool retrieve_latest = 3;
27+
/**
28+
* A flag to request the latest available block.
29+
* <p>
30+
* This value MUST be set `true` to request the last block available.
31+
*/
32+
bool retrieve_latest = 3;
33+
}
6034
}
6135

6236
/**
@@ -81,21 +55,21 @@ message BlockResponse {
8155
* This status indicates the server software failed to set a status,
8256
* and SHALL be considered a software defect.
8357
*/
84-
READ_BLOCK_UNKNOWN = 0;
58+
UNKNOWN = 0;
8559

8660
/**
8761
* The requesting client account lacks sufficient HBAR to pay the
8862
* service fee for this request.<br/>
8963
* The client MAY retry the request, but MUST increase the client
9064
* account balance with this block node server before doing so.
9165
*/
92-
READ_BLOCK_INSUFFICIENT_BALANCE = 1;
66+
INSUFFICIENT_BALANCE = 1;
9367

9468
/**
9569
* The request succeeded.<br/>
9670
* The requested block SHALL be returned in the `block` field.
9771
*/
98-
READ_BLOCK_SUCCESS = 2;
72+
SUCCESS = 2;
9973

10074
/**
10175
* The requested block was not found.<br/>
@@ -104,33 +78,23 @@ message BlockResponse {
10478
* The client MAY retry the request; if this result is repeated the
10579
* request SHOULD be directed to a different block node server.
10680
*/
107-
READ_BLOCK_NOT_FOUND = 3;
81+
NOT_FOUND = 3;
10882

10983
/**
11084
* The requested block is not available on this block node server.<br/>
11185
* The client SHOULD send a `serverStatus` request to determine the
11286
* lowest and highest block numbers available at this block node server.
11387
*/
114-
READ_BLOCK_NOT_AVAILABLE = 4;
115-
116-
/**
117-
* The request for a verified block cannot be fulfilled.<br/>
118-
* The client requested a verified block from a block node that does not
119-
* offer verified blocks.
120-
* <p>
121-
* The client MAY retry the request with the `allow_unverified` flag set.
122-
*/
123-
ALLOW_UNVERIFIED_REQUIRED = 5;
88+
NOT_AVAILABLE = 4;
12489

12590
/**
126-
* The request for a verified block cannot be fulfilled.<br/>
127-
* The client requested a verified block from a block node but the
128-
* requested block is not yet verified.
91+
* The request cannot be fulfilled.<br/>
92+
* The client sent a malformed or structurally incorrect request.
12993
* <p>
130-
* The client MAY retry the request after a short delay
131-
* (typically 2 seconds or more).
94+
* The client MAY retry the request after correcting the form and
95+
* structure.
13296
*/
133-
VERIFIED_BLOCK_UNAVAILABLE = 6;
97+
INVALID_REQUEST = 5;
13498
}
13599

136100
/**

‎suites/src/main/java/org/hiero/block/suites/block/access/GetBlockApiTests.java

Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -66,11 +66,11 @@ void publishSomeBlocks() throws IOException, InterruptedException {
6666
void requestExistingBlockUsingBlockAPI() {
6767
// Request block number 1 (which should have been published by the simulator)
6868
final long blockNumber = 1;
69-
final BlockResponse response = getBlock(blockAccessStub, blockNumber, false);
69+
final BlockResponse response = getBlock(blockAccessStub, blockNumber);
7070

7171
// Verify the response
7272
assertNotNull(response, "Response should not be null");
73-
assertEquals(Code.READ_BLOCK_SUCCESS, response.getStatus(), "Block retrieval should be successful");
73+
assertEquals(Code.SUCCESS, response.getStatus(), "Block retrieval should be successful");
7474

7575
// Verify the block content
7676
assertTrue(response.hasBlock(), "Response should contain a block");
@@ -85,14 +85,11 @@ void requestExistingBlockUsingBlockAPI() {
8585
void requestNonExistingBlockUsingBlockAPI() {
8686
// Request a non-existing block number
8787
final long blockNumber = 1000;
88-
final BlockResponse response = getBlock(blockAccessStub, blockNumber, false);
88+
final BlockResponse response = getBlock(blockAccessStub, blockNumber);
8989

9090
// Verify the response
9191
assertNotNull(response, "Response should not be null");
92-
assertEquals(
93-
Code.READ_BLOCK_NOT_AVAILABLE,
94-
response.getStatus(),
95-
"Block retrieval should fail for non-existing block");
92+
assertEquals(Code.NOT_AVAILABLE, response.getStatus(), "Block retrieval should fail for non-existing block");
9693

9794
// Verify that the block is null
9895
assertFalse(response.hasBlock(), "Response should not contain a block");
@@ -102,11 +99,11 @@ void requestNonExistingBlockUsingBlockAPI() {
10299
@DisplayName("Get a Single Block using API - Request Latest Block")
103100
void requestLatestBlockUsingBlockAPI() {
104101
// Request the latest block
105-
final BlockResponse response = getLatestBlock(blockAccessStub, false);
102+
final BlockResponse response = getLatestBlock(blockAccessStub);
106103

107104
// Verify the response
108105
assertNotNull(response, "Response should not be null");
109-
assertEquals(Code.READ_BLOCK_SUCCESS, response.getStatus(), "Block retrieval should be successful");
106+
assertEquals(Code.SUCCESS, response.getStatus(), "Block retrieval should be successful");
110107

111108
// Verify the block content
112109
final long latestPublishedBlock =
@@ -128,8 +125,7 @@ void requestLatestBlockAndSpecificBlockUsingBlockAPI() {
128125

129126
// Verify the response
130127
assertNotNull(response, "Response should not be null");
131-
assertEquals(
132-
Code.READ_BLOCK_NOT_FOUND, response.getStatus(), "Block retrieval should fail for non-existing block");
128+
assertEquals(Code.NOT_FOUND, response.getStatus(), "Block retrieval should fail for non-existing block");
133129

134130
// Verify that the block is null
135131
assertFalse(response.hasBlock(), "Response should not contain a block");
@@ -145,8 +141,7 @@ void requestWithoutBlockNumberAndRetrieveLatestFalse() {
145141

146142
// Verify the response
147143
assertNotNull(response, "Response should not be null");
148-
assertEquals(
149-
Code.READ_BLOCK_NOT_FOUND, response.getStatus(), "Block retrieval should fail for non-existing block");
144+
assertEquals(Code.NOT_FOUND, response.getStatus(), "Block retrieval should fail for non-existing block");
150145

151146
// Verify that the block is null
152147
assertFalse(response.hasBlock(), "Response should not contain a block");

0 commit comments

Comments
 (0)