fix: Adjust block access service to be correct without workarounds (#1147)

Signed-off-by: Joseph S <121976561+jsync-swirlds@users.noreply.github.com>

Author: jsync-swirlds

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

@@ -3,7 +3,7 @@
 
 import static java.lang.System.Logger.Level.DEBUG;
 import static java.lang.System.Logger.Level.ERROR;
-import static java.lang.System.Logger.Level.INFO;
+import static java.lang.System.Logger.Level.WARNING;
 
 import com.hedera.hapi.block.stream.Block;
 import com.hedera.pbj.runtime.grpc.GrpcException;
@@ -54,31 +54,18 @@ private BlockResponse handleBlockRequest(BlockRequest request) {
         requestCounter.increment();
 
         try {
-            // Log in case both block_number and retrieve_latest are set, this should not happen
-            if (request.retrieveLatest() && request.blockNumber() != -1) {
-                LOGGER.log(
-                        INFO,
-                        "Both block_number and retrieve_latest set. Using retrieve_latest instead of block_number: {0}",
-                        request.blockNumber());
-                responseCounterNotFound.increment();
-                return new BlockResponse(Code.READ_BLOCK_NOT_FOUND, null);
-            }
-            // when block_number is -1 and retrieve_latest is false, return an NOT_FOUND error
-            if (request.blockNumber() == -1 && !request.retrieveLatest()) {
-                LOGGER.log(INFO, "Block number is -1 and retrieve_latest is false");
-                responseCounterNotFound.increment();
-                return new BlockResponse(Code.READ_BLOCK_NOT_FOUND, null);
-            }
-
             long blockNumberToRetrieve;
+            // The block number and retrieve latest are mutually exclusive in
+            // the proto definition, so no need to check for that here.
 
-            // if retrieveLatest is set, get the latest block number
-            if (request.retrieveLatest()) {
+            // if retrieveLatest is set, or the request is for the largest
+            // possible block number, get the latest block.
+            if (request.retrieveLatestOrElse(false) || request.blockNumberOrElse(0L) == -1) {
                 blockNumberToRetrieve = blockProvider.availableBlocks().max();
                 if (blockNumberToRetrieve < 0) {
-                    LOGGER.log(INFO, "Latest block number not available");
+                    LOGGER.log(WARNING, "Latest available block number is not available, this should not be possible.");
                     responseCounterNotAvailable.increment();
-                    return new BlockResponse(Code.READ_BLOCK_NOT_AVAILABLE, null);
+                    return new BlockResponse(Code.NOT_AVAILABLE, null);
                 }
             } else {
                 blockNumberToRetrieve = request.blockNumber();
@@ -95,18 +82,19 @@ private BlockResponse handleBlockRequest(BlockRequest request) {
                         lowestBlockNumber,
                         highestBlockNumber);
                 responseCounterNotAvailable.increment();
-                return new BlockResponse(Code.READ_BLOCK_NOT_AVAILABLE, null);
+                return new BlockResponse(Code.NOT_AVAILABLE, null);
             }
 
             // Retrieve the block
             Block block = blockProvider.block(blockNumberToRetrieve).block();
             responseCounterSuccess.increment();
-            return new BlockResponse(Code.READ_BLOCK_SUCCESS, block);
+            return new BlockResponse(Code.SUCCESS, block);
 
         } catch (RuntimeException e) {
-            LOGGER.log(ERROR, "Failed to retrieve block number: {0}", request.blockNumber());
-            responseCounterNotFound.increment();
-            return new BlockResponse(Code.READ_BLOCK_NOT_FOUND, null);
+            String message = "Failed to retrieve block number %d.".formatted(request.blockNumber());
+            LOGGER.log(ERROR, message, e);
+            responseCounterNotFound.increment(); // Should this be a failure counter?
+            return new BlockResponse(Code.NOT_FOUND, null);
         }
     }
 
@@ -127,10 +115,10 @@ public void init(BlockNodeContext context, ServiceBuilder serviceBuilder) {
                         .withDescription("Number of successful single block requests"));
         responseCounterNotAvailable = context.metrics()
                 .getOrCreate(new Counter.Config(METRICS_CATEGORY, "single-block-requests-not-available")
-                        .withDescription("Number of single block requests that were not available"));
+                        .withDescription("Number of single block requests that responded not available"));
         responseCounterNotFound = context.metrics()
                 .getOrCreate(new Counter.Config(METRICS_CATEGORY, "single-block-requests-not-found")
-                        .withDescription("Number of single block requests that were not found"));
+                        .withDescription("Number of single block requests that responded not found"));
         // Get the block provider
         this.blockProvider = context.historicalBlockProvider();
         // Register this service

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

@@ -61,18 +61,15 @@ void testServiceInterfaceBasics() {
     @DisplayName("Happy Path Test, BlockAccessServicePlugin for an existing Block Number")
     void happyTestGetBlock() throws ParseException {
         final long blockNumber = 1;
-        final BlockRequest request = BlockRequest.newBuilder()
-                .blockNumber(blockNumber)
-                .allowUnverified(true)
-                .retrieveLatest(false)
-                .build();
+        final BlockRequest request =
+                BlockRequest.newBuilder().blockNumber(blockNumber).build();
         toPluginPipe.onNext(BlockRequest.PROTOBUF.toBytes(request));
         // Check we get a response
         assertEquals(1, fromPluginBytes.size());
         // parse the response
         BlockResponse response = BlockResponse.PROTOBUF.parse(fromPluginBytes.get(0));
         // check that the status is success
-        assertEquals(Code.READ_BLOCK_SUCCESS, response.status());
+        assertEquals(Code.SUCCESS, response.status());
         // check that the block number is correct
         assertEquals(1, response.block().items().getFirst().blockHeader().number());
     }
@@ -81,74 +78,48 @@ void happyTestGetBlock() throws ParseException {
     @DisplayName("Negative Test, GetBlock for a non-existing Block Number")
     void negativeTestNonExistingBlock() throws ParseException {
         final long blockNumber = 1000;
-        final BlockRequest request = BlockRequest.newBuilder()
-                .blockNumber(blockNumber)
-                .allowUnverified(true)
-                .retrieveLatest(false)
-                .build();
+        final BlockRequest request =
+                BlockRequest.newBuilder().blockNumber(blockNumber).build();
         toPluginPipe.onNext(BlockRequest.PROTOBUF.toBytes(request));
         // Check we get a response
         assertEquals(1, fromPluginBytes.size());
         // parse the response
         BlockResponse response = BlockResponse.PROTOBUF.parse(fromPluginBytes.get(0));
-        // check that the status is NOT FOUND
-        assertEquals(Code.READ_BLOCK_NOT_AVAILABLE, response.status());
+        // check that the status is NOT AVAILABLE
+        assertEquals(Code.NOT_AVAILABLE, response.status());
         // check block is null
         assertNull(response.block());
     }
 
     @Test
     @DisplayName("Request Latest Block")
     void testRequestLatestBlock() throws ParseException {
-        final BlockRequest request = BlockRequest.newBuilder()
-                .blockNumber(-1)
-                .allowUnverified(true)
-                .retrieveLatest(true)
-                .build();
+        final BlockRequest request =
+                BlockRequest.newBuilder().retrieveLatest(true).build();
         toPluginPipe.onNext(BlockRequest.PROTOBUF.toBytes(request));
         // Check we get a response
         assertEquals(1, fromPluginBytes.size());
         // parse the response
         BlockResponse response = BlockResponse.PROTOBUF.parse(fromPluginBytes.get(0));
         // check that the status is success
-        assertEquals(Code.READ_BLOCK_SUCCESS, response.status());
+        assertEquals(Code.SUCCESS, response.status());
         // check that the block number is correct
         assertEquals(24, response.block().items().getFirst().blockHeader().number());
     }
 
     @Test
-    @DisplayName("Request Latest and a specific Block different from latest, should fail with READ_BLOCK_NOT_FOUND")
-    void testRequestLatestBlockDifferent() throws ParseException {
-        final long blockNumber = 1;
-        final BlockRequest request = BlockRequest.newBuilder()
-                .blockNumber(blockNumber)
-                .allowUnverified(true)
-                .retrieveLatest(true)
-                .build();
+    @DisplayName("block_number is -1 - should return latest block")
+    void testBlockNumberIsMinusOne() throws ParseException {
+        final BlockRequest request = BlockRequest.newBuilder().blockNumber(-1).build();
         toPluginPipe.onNext(BlockRequest.PROTOBUF.toBytes(request));
         // Check we get a response
         assertEquals(1, fromPluginBytes.size());
         // parse the response
         BlockResponse response = BlockResponse.PROTOBUF.parse(fromPluginBytes.get(0));
         // check that the status is success
-        assertEquals(Code.READ_BLOCK_NOT_FOUND, response.status());
-    }
-
-    @Test
-    @DisplayName("block_number is -1 and retrieve_latest is false - should return READ_BLOCK_NOT_FOUND")
-    void testBlockNumberIsMinusOneAndRetrieveLatestIsFalse() throws ParseException {
-        final BlockRequest request = BlockRequest.newBuilder()
-                .blockNumber(-1)
-                .allowUnverified(true)
-                .retrieveLatest(false)
-                .build();
-        toPluginPipe.onNext(BlockRequest.PROTOBUF.toBytes(request));
-        // Check we get a response
-        assertEquals(1, fromPluginBytes.size());
-        // parse the response
-        BlockResponse response = BlockResponse.PROTOBUF.parse(fromPluginBytes.get(0));
-        // check that the status is READ_BLOCK_NOT_FOUND
-        assertEquals(Code.READ_BLOCK_NOT_FOUND, response.status());
+        assertEquals(Code.SUCCESS, response.status());
+        // check that the block number is correct
+        assertEquals(24, response.block().items().getFirst().blockHeader().number());
     }
 
     private void sendBlocks(int numberOfBlocks) {

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

@@ -13,50 +13,29 @@ import "stream/block.proto";
  * A request to read a single block.
  *
  * A client system SHALL send this message to request a single block,
- * including the block state proof.<br/>
- * A client MAY request that the block be sent without verification.
- * A compliant Block Node MAY respond to requests that allow unverified
- * responses by returning the full requested block before verifying
- * the included block proof.<br/>
- * A compliant Block Node MAY support _only_ requests that allow unverified
- * blocks, but MUST clearly document that limitation, and MUST respond to
- * a request that does not allow unverified blocks with the
- * `ALLOW_UNVERIFIED_REQUIRED` response code.
+ * including the block state proof.
  */
 message BlockRequest {
-    /**
-     * The block number of a block to retrieve.
-     * <p>
-     * The requested block MUST exist on the block node.<br/>
-     * This value MUST NOT be set if `retrieve_latest` is set `true`.<br/>
-     * This value MUST be set to a valid block number if `retrieve_latest` is
-     * unset or is set `false`.
-     */
-    uint64 block_number = 1;
-
-    /**
-     * A flag to indicate that the requested block may be sent without
-     * verifying its `BlockProof`.<br/>
-     * This might be set by a client that expects to perform its own
-     * verification and wishes lower latency or, potentially, lower cost.
-     * <p>
-     * If this value is set, then the responding Block Node MAY respond with a
-     * block that has not completed verification of its `BlockProof`.<br/>
-     * If this is _not_ set then the Block Node MUST respond with either a
-     * fully verified and validated block, or `VERIFIED_BLOCK_UNAVAILABLE` if
-     * the requested block is not yet verified.<br/>
-     * The default value is _not set_.
-     */
-    bool allow_unverified = 2;
+    oneof block_specifier {
+        /**
+         * The block number of a block to retrieve.
+         * <p>
+         * The requested block MUST exist on the block node.<br/>
+         * A request MAY specify `uint64_max` to signal that the last possible
+         * block should be returned (which is subtly different from setting
+         * `retrieve_latest` instead of setting a `block_number`, though the
+         * result will always be the same, in most implementations).
+         */
+        uint64 block_number = 1;
 
-    /**
-     * A flag to request the latest available block.
-     * <p>
-     * This value MAY be set `true` to request the last block available.<br/>
-     * If this value is set to `true` then `block_number` MUST NOT be set and
-     * SHALL be ignored.
-     */
-    bool retrieve_latest = 3;
+        /**
+         * A flag to request the latest available block.
+         * <p>
+         * This value SHOULD be set `true`, instead of setting `block_number`
+         * if the intent is to request the latest block available.
+         */
+        bool retrieve_latest = 2;
+    }
 }
 
 /**
@@ -81,21 +60,22 @@ message BlockResponse {
          * This status indicates the server software failed to set a status,
          * and SHALL be considered a software defect.
          */
-        READ_BLOCK_UNKNOWN = 0;
+        UNKNOWN = 0;
 
         /**
-         * The requesting client account lacks sufficient HBAR to pay the
-         * service fee for this request.<br/>
-         * The client MAY retry the request, but MUST increase the client
-         * account balance with this block node server before doing so.
+         * The request succeeded.<br/>
+         * The requested block SHALL be returned in the `block` field.
          */
-        READ_BLOCK_INSUFFICIENT_BALANCE = 1;
+        SUCCESS = 1;
 
         /**
-         * The request succeeded.<br/>
-         * The requested block SHALL be returned in the `block` field.
+         * The request cannot be fulfilled.<br/>
+         * The client sent a malformed or structurally incorrect request.
+         * <p>
+         * The client MAY retry the request after correcting the form and
+         * structure.
          */
-        READ_BLOCK_SUCCESS = 2;
+        INVALID_REQUEST = 2;
 
         /**
          * The requested block was not found.<br/>
@@ -104,33 +84,14 @@ message BlockResponse {
          * The client MAY retry the request; if this result is repeated the
          * request SHOULD be directed to a different block node server.
          */
-        READ_BLOCK_NOT_FOUND = 3;
+        NOT_FOUND = 3;
 
         /**
          * The requested block is not available on this block node server.<br/>
          * The client SHOULD send a `serverStatus` request to determine the
          * lowest and highest block numbers available at this block node server.
          */
-        READ_BLOCK_NOT_AVAILABLE = 4;
-
-        /**
-         * The request for a verified block cannot be fulfilled.<br/>
-         * The client requested a verified block from a block node that does not
-         * offer verified blocks.
-         * <p>
-         * The client MAY retry the request with the `allow_unverified` flag set.
-         */
-        ALLOW_UNVERIFIED_REQUIRED = 5;
-
-        /**
-         * The request for a verified block cannot be fulfilled.<br/>
-         * The client requested a verified block from a block node but the
-         * requested block is not yet verified.
-         * <p>
-         * The client MAY retry the request after a short delay
-         * (typically 2 seconds or more).
-         */
-        VERIFIED_BLOCK_UNAVAILABLE = 6;
+        NOT_AVAILABLE = 4;
     }
 
     /**