add proto and s3 to client #3168

MitchTurner · 2025-12-16T15:09:47Z

Description

This is adding support for accessing the new block aggregation RPC and subsequent S3 calls using the Fuel Client.

…-s3-to-client

cursor · 2025-12-17T01:20:38Z

PR Summary

Adds RPC support to FuelClient for fetching/streaming blocks via block-aggregator and S3, and exposes rpc_address on FuelService; tests updated to use new client APIs.

Client (crates/client):
- Feature flag: New rpc feature with deps (fuel-core-block-aggregator-api, prost, tonic, aws-sdk-s3, aws-config, flate2).
- RPC integration:
  - New constructor FuelClient::new_with_rpc(...) and optional fields rpc_client/aws_client.
  - New APIs: get_block_range(...), new_block_subscription(...), get_aggregated_height().
  - Handles block responses from bytes or remote S3; downloads via AWS S3 client and gzip-decompresses; converts protobuf blocks to internal types.
Service (crates/fuel-core):
- Exposes FuelService::rpc_address (when rpc feature enabled).
Tests (tests/):
- Update RPC/S3 tests to use FuelClient RPC methods and new streaming APIs.
- Enable fuel-core-client/rpc in tests/Cargo.toml feature set.

^{Written by Cursor Bugbot for commit 54f45d6. This will update automatically on new commits. Configure here.}

crates/client/src/client.rs

…-s3-to-client

rymnc · 2025-12-17T16:33:14Z

crates/client/src/client.rs

@@ -1,3 +1,5 @@
+#![allow(unused_imports)]


rymnc · 2025-12-17T16:35:09Z

crates/client/src/client.rs

    require_height: ConsistencyPolicy,
    chain_state_info: ChainStateInfo,
+    #[cfg(feature = "rpc")]
+    rpc_client: Option<ProtoBlockAggregatorClient<Channel>>,


probably a good chance to create a FuelClientBuilder now, all the constructors are duped code right now

I don't think it's quite there. I'm a big fan of builders, but essentially it's just 1 optional param.

I've deduped the code (and one of them I didn't end up using so I removed): 5681c3e

rymnc · 2025-12-17T16:35:39Z

crates/client/src/client.rs

+
+#[cfg(feature = "rpc")]
+impl FuelClient {
+    fn rpc_client(&self) -> io::Result<ProtoBlockAggregatorClient<Channel>> {


io::Result ?

I know. It's what all the other responses from the client give. I'm not sure why.

crates/client/src/client.rs

rymnc · 2025-12-17T16:38:47Z

crates/client/src/client.rs

+    fn unzip_bytes(bytes: &[u8]) -> Vec<u8> {
+        let mut decoder = GzDecoder::new(bytes);
+        let mut output = Vec::new();
+        decoder.read_to_end(&mut output).unwrap();
+        output


not guaranteed that we used GzEncoder to push to s3, i think this part needs some reworking to make it a bit more stricter. you can check the header to see if it can be gzip decompressed ~ unwrap probably not a good idea here

We are making 2 assumptions here:

the bytes are the serialized ProtoBlock

the bytes have been gzipped

Would you prefer the protobuf message to include compression and serialization enums to allow expansion of this in the future? Or we could add more clarfication to the Location type:

/// Nested message and enum types in `RemoteBlockResponse`. pub mod remote_block_response { #[derive(serde::Serialize, serde::Deserialize)] #[allow(clippy::large_enum_variant)] #[derive(Clone, PartialEq, ::prost::Oneof)] pub enum Location { #[prost(message, tag = "1")] Http(super::RemoteHttpEndpoint), #[prost(message, tag = "2")] S3(super::RemoteS3Bucket), } }

Maybe change S3 to S3ProtoGZip lol. Then we could just add new variants if they ever happen.

I understand the concern, but I'm also OK with the current design.

I think we should parse the HTTP response headers and check if the Content-Encoding header has been set. Then, if it's set to gzip, we ungzip it, otherwise we just assume it's not encoded.

This is similar to how reqwest works - https://github.com/seanmonstar/reqwest/blob/master/src/async_impl/client.rs#L1062

mchristopher · 2025-12-17T18:43:39Z

crates/client/src/client.rs

+        let mut url = reqwest::Url::parse(&raw_url)
+            .map_err(anyhow::Error::msg)
+            .with_context(|| format!("Invalid Fuel GraphQL URL: {raw_url}"))?;
+        url.set_path("/v1/graphql");


This is not a valid assumption, other RPC providers like Quicknode and Ankr add prefixes onto the URL and don't support accessing the graphql endpoint from the root.

The URLs for both RPC & GraphQL should be explicitly passed in here, I don't think there is a way we can auto-configure this.

This is the existing behavior. I'm not changing anything to do with the GraphQL server in this code. I think it would be a breaking change to change this here.

I did have a couple bugs in the way the rpc url was being set though, and I've fixed those.

mchristopher · 2025-12-17T18:47:17Z

crates/client/src/client.rs

+                            ..
+                        } = s3;
+                        let s3_url = endpoint
+                            .unwrap_or_else(|| "https://s3.amazonaws.com".to_string());


You should not pass in manual endpoints if one is not provided. If something is not provided, let the S3 SDK figure it out based on the bucket name.

mchristopher · 2025-12-17T18:48:05Z

crates/client/src/client.rs

+    async fn aws_client(url: &str) -> Client {
+        let credentials = DefaultCredentialsChain::builder().build().await;
+        let _aws_region =
+            std::env::var("AWS_REGION").expect("AWS_REGION env var must be set");


This is not a valid assumption on the client side. Even when it's required, the AWS SDK itself will throw an error when it doesn't have the right data.

Thanks. Nice catch. I just moved a bunch of stuff over from the test code but didn't fix all of the differences.

mchristopher · 2025-12-17T18:49:13Z

crates/client/src/client.rs

+        let builder = aws_sdk_s3::config::Builder::from(&sdk_config);
+        let config = builder.force_path_style(true).build();
+        Client::from_conf(config)
+    }


Should create a global client that gets reused after the first init. Need to verify, but I'm pretty sure everything is thread safe in the AWS SDK, so cross-thread access shouldn't be a problem

tbh i don't think it's worth adding these dependencies if we can fetch using http

Copying from my convo with Cursor below:

We handle each response in a stream and we'd have to clone the shared client into each thread of that stream:

.then(|res| async move { let resp = res.map_err(|e| io::Error::other(format!("RPC error: {:?}", e)))?; Self::convert_block_response(resp).await });

We could wrap it in a Arc and clone and move it into each thread, but idk if we are getting much at that point. I don't think the aws client is particularly expensive to construct.

tbh i don't think it's worth adding these dependencies if we can fetch using http

It sounds like we have a whole stack of AWS credential management setup and we will be reusing that with this new code. That's more complicated than just simple HTTP requests.

We need to include a basic S3 client (not just HTTP) since we want to support requestor pays and potentially other authenticated calls. We don't need a ton of logic, but we do need basic AWS SDK support for authenticated calls.

I added a shared client. It looks like it's the intended use of the aws client, as it has an Arc inside and is Clone.

crates/client/src/client.rs

cursor · 2025-12-17T22:56:02Z

tests/tests/rpc_s3.rs

 };
 use aws_sdk_s3::Client;
-use flate2::read::GzDecoder;
+// use flate2::read::GzDecoder;


Bug: Commented-out import left in test file

A commented-out import // use flate2::read::GzDecoder; appears to be leftover from the refactoring where the unzip_bytes function was moved from the test file into the FuelClient. This dead code was accidentally left in during the changes.

cursor · 2025-12-17T22:56:02Z

crates/fuel-core/src/service.rs

+            .config
+            .rpc_config
+            .clone()
+            .map(|config| config.addr);


Bug: RPC address uses config value not actual bound address

The rpc_address field is populated from config.rpc_config.addr (the configured address) rather than the actual bound address. In contrast, bound_address for GraphQL is obtained from runner.shared.graph_ql.bound_address (the actual bound address). If the RPC server is configured with port 0 (OS auto-assigns), rpc_address would contain port 0 instead of the actual assigned port. Tests pass because free_local_addr() returns a real port, but production use with port 0 would break client connection attempts using rpc_address.

cursor · 2025-12-18T15:17:41Z

crates/client/src/client.rs

+                Err(io::Error::other("Literal payloads are not supported yet"))
+            }
+            Payload::Bytes(bytes) => {
+                let proto_block = ProtoBlock::decode(bytes.as_slice()).unwrap();


Bug: Inconsistent error handling causes panic on malformed data

The Payload::Bytes branch in convert_block_response uses .unwrap() on ProtoBlock::decode(), which will panic if the RPC server sends malformed protobuf bytes. This is inconsistent with the Payload::Remote branch at lines 1764-1767, which properly handles the decode error using map_err() and returns it. Since the function signature returns io::Result, malformed data from the bytes payload path causes a panic instead of returning a proper error.

cursor · 2025-12-18T15:35:01Z

crates/fuel-core/src/service.rs

+            .config
+            .rpc_config
+            .clone()
+            .map(|config| config.addr);


Bug: RPC address reads from config instead of bound address

The rpc_address field is populated from the configuration (config.rpc_config.addr) rather than the actual bound address from the RPC server. This is inconsistent with how bound_address is obtained for GraphQL (line 169), which correctly reads from runner.shared.graph_ql.bound_address. While the current test helpers use free_local_addr() which pre-allocates a specific port, this creates a time-of-check-time-of-use race condition where another process could claim the port between allocation and binding. The RPC service should expose its actual bound address similar to the GraphQL pattern.

That's fine. We have two addresses here.

cursor · 2025-12-18T15:35:01Z

crates/client/src/client.rs

+                            "Failed to convert RPC block to internal block: {e:?}"
+                        ))
+                    },
+                )


Bug: Block reconstruction uses empty message IDs and default root

The convert_block_response function calls fuel_block_from_protobuf with hardcoded empty msg_ids (&[]) and Bytes32::default() for event_inbox_root. The existing roundtrip test in the codebase demonstrates these parameters are required for correct block reconstruction - it passes actual values and asserts block == deserialized_block. For blocks containing L1 relay messages, passing empty values causes the reconstructed block's computed roots to differ from the original, resulting in an invalid block that won't match the original.

Additional Locations (1)

crates/client/src/client.rs#L1768-L1773

mchristopher · 2025-12-18T16:20:19Z

crates/client/src/client.rs

+        } else {
+            s3_client.ok_or(io::Error::other("No AWS client configured"))?
+        };
+        tracing::info!("getting block from bucket: {} with key {}", bucket, key);


I'd put this at debug - it's going to be very noisy if it's info

mchristopher · 2025-12-18T16:28:21Z

crates/client/src/client.rs

+            s3_client.ok_or(io::Error::other("No AWS client configured"))?
+        };
+        tracing::debug!("getting block from bucket: {} with key {}", bucket, key);
+        let req = client.get_object().bucket(bucket).key(key);


Need to add a check for requester pays here & add support. Something like:

let req = if requester_pays { req.request_payer(RequestPayer::Requester) }

cursor · 2025-12-18T16:30:51Z

crates/client/src/client.rs

+                            "Failed to convert RPC block to internal block: {e:?}"
+                        ))
+                    },
+                )


Bug: Hardcoded empty values cause incorrect block reconstruction

The convert_block_response function always passes empty msg_ids (&[]) and Bytes32::default() for event_inbox_root to fuel_block_from_protobuf. These parameters are used to regenerate the block header's message_outbox_root and event_inbox_root fields. For any block that originally had outbox messages or a non-default event_inbox_root, the reconstructed block will have an incorrect header with wrong merkle roots, resulting in a different block ID than the original. The protobuf format stores these computed values but the deserialization function ignores them and regenerates from the caller-provided parameters.

Additional Locations (1)

crates/client/src/client.rs#L1768-L1773

WIP

38cdef2

MitchTurner self-assigned this Dec 16, 2025

MitchTurner added 5 commits December 16, 2025 08:13

Merge branch 'chore/use-new-proto-types-2' into feature/add-proto-and…

f630f42

…-s3-to-client

Add get_block_range method to fuel-client

e0df9ef

Merge branch 'chore/use-new-proto-types-2' into feature/add-proto-and…

123e0f1

…-s3-to-client

Add get_aggregated_height

b37f1b2

Add new block stream

f9021b5

MitchTurner added the no changelog Skip the CI check of the changelog modification label Dec 16, 2025

MitchTurner added 4 commits December 16, 2025 18:03

ADd s3 support to fuel client :)

345cb94

lint toml

eea4f92

Appease Clippy-sama

9dd26dc

Move import

d68bf16

MitchTurner marked this pull request as ready for review December 17, 2025 01:20

MitchTurner requested review from a team, Dentosal, rymnc and xgreenx as code owners December 17, 2025 01:20

cursor bot reviewed Dec 17, 2025

View reviewed changes

crates/client/src/client.rs Show resolved Hide resolved

crates/client/src/client.rs Outdated Show resolved Hide resolved

crates/client/src/client.rs Outdated Show resolved Hide resolved

crates/client/src/client.rs Show resolved Hide resolved

Merge branch 'chore/use-new-proto-types-2' into feature/add-proto-and…

ca97a52

…-s3-to-client

rymnc reviewed Dec 17, 2025

View reviewed changes

mchristopher reviewed Dec 17, 2025

View reviewed changes

MitchTurner added 2 commits December 17, 2025 13:50

Remove ignore, fix url

1286e13

Remove unwraps

a7a6c7a

cursor bot reviewed Dec 17, 2025

View reviewed changes

crates/client/src/client.rs Show resolved Hide resolved

crates/client/src/client.rs Outdated Show resolved Hide resolved

Cleanup client constructors

5681c3e

cursor bot reviewed Dec 17, 2025

View reviewed changes

crates/client/src/client.rs Show resolved Hide resolved

crates/client/src/client.rs Outdated Show resolved Hide resolved

MitchTurner added 4 commits December 17, 2025 14:36

Make aws endpoint completely optional

09cdf7d

Remove env var check

49e6e24

Fix url construction again

d3da847

move import

cbfdefb

cursor bot reviewed Dec 17, 2025

View reviewed changes

MitchTurner added 2 commits December 18, 2025 08:12

Reuse AWS Client

fc75191

Remove commented code

8ede9dc

cursor bot reviewed Dec 18, 2025

View reviewed changes

MitchTurner added 2 commits December 18, 2025 08:26

Remove unwrap

9c0322e

Remove unwrap

313f7d5

cursor bot reviewed Dec 18, 2025

View reviewed changes

mchristopher reviewed Dec 18, 2025

View reviewed changes

Use debug for noisy trace

0db6b83

mchristopher reviewed Dec 18, 2025

View reviewed changes

cursor bot reviewed Dec 18, 2025

View reviewed changes

Add requesterr pays

54f45d6

add proto and s3 to client #3168

Are you sure you want to change the base?

add proto and s3 to client #3168

Uh oh!

Conversation

MitchTurner commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

cursor bot commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MitchTurner Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cursor bot Dec 17, 2025

Choose a reason for hiding this comment

Bug: Commented-out import left in test file

Uh oh!

cursor bot Dec 17, 2025

Choose a reason for hiding this comment

Bug: RPC address uses config value not actual bound address

Uh oh!

cursor bot Dec 18, 2025

Choose a reason for hiding this comment

Bug: Inconsistent error handling causes panic on malformed data

Uh oh!

MitchTurner commented Dec 16, 2025 •

edited

Loading

cursor bot commented Dec 17, 2025 •

edited

Loading

MitchTurner Dec 17, 2025 •

edited

Loading