RPC Consistency Proposal #2473

acerone85 · 2024-12-05T12:09:34Z

Linked Issues/PRs

Related issue: #1897

See https://www.notion.so/fuellabs/RPC-Consistency-Proposal-V2-13b2f2293f31809bbce0d93a4c28d633 for the proposal specs.

This PR implements a variation of the proposal in the link, due to techincal limitations of the HTTP protocol when using graphql subscriptions.

for context: Headers cannot be used, the reason being that subscriptions will return multiple graphql responses in a single HTTP response (and therefore have a single HeaderMap), while we require that each item in the response has its associated CURRENT_FUEL_BLOCK_HEIGHT field. Moreover the Response metadata for subscriptions is returned before the actual response body (as it is standard with HTTP).

in the end graphql has a (very undocumented) way to add an extensions field to responses, at the same level of data/errors field. This does not require to tweak anything at the HTTP level.

We will implement an improvement where the server node will wait for a configurable number of blocks before returning a PRECONDITION FAILED response, in a follow-up PR.

Entrypoint for reviews:

https://github.com/FuelLabs/fuel-core/pull/2473/files#diff-9caf873d3167ab9485e7882d621b5147e98b2478cc6462095b88374e2f914f3aR428 for the graphql handler
https://github.com/FuelLabs/fuel-core/pull/2473/files#diff-9caf873d3167ab9485e7882d621b5147e98b2478cc6462095b88374e2f914f3aR428 for the extension definition

Manual testing

TODO:

Tests

Description

Checklist

~~Breaking changes are clearly marked as such in the PR description and changelog~~
New behavior is reflected in tests
The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

I have reviewed the code myself
~~I have created follow-up issues caused by this PR and linked them here~~

After merging, notify other teams

[Add or remove entries as needed]

Rust SDK
Sway compiler
Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
Someone else?

acerone85 · 2024-12-05T15:36:35Z

crates/client/src/client.rs

+    pub async fn query_with_headers<ResponseData, Vars>(
+        &self,
+        q: Operation<ResponseData, Vars>,
+        headers: impl IntoIterator<Item = (String, String)>,


TODO: HeadersMap here?

I assume this one https://docs.rs/http/1.2.0/http/header/struct.HeaderMap.html - makes sense 👍

Forgot to mention this is done, see d9091e6

acerone85 · 2024-12-05T15:37:17Z

crates/fuel-core/src/graphql_api/worker_service.rs

 };

 #[cfg(test)]
 mod tests;

+// The last known block height that was processed by the GraphQL service.
+pub static LAST_KNOWN_BLOCK_HEIGHT: OnceLock<AtomicU32> = OnceLock::new();


todo: remove Static and store in Service? Should still be accessible from middleware

Is this so performance critical that we can't do a database lookup for this value? I'd default to querying this from the database, to ensure we don't have any weird consistency issues when this field and the db isn't in sync. I guess this isn't a big problem right now, but it just doesn't feel right to allow for inconsistencies in a field used to synchronize with other nodes.

We shouldn't use static. Each extension has access to the context and you can get ReadDatabase from the context. ReadDatabase knows the latest block height.

You can check how ViewExtension works

I agree that we should fetch the value from the DB (it is probably going to be fetched from the memtable anyway, since it's frequently updated). I don't agree that we should do this with async_graphql Extensions, the reason being that support for headers is pretty limited in there.

Headers do not seem to be set when building the query context in async_graphql (see https://github.com/async-graphql/async-graphql/blob/7f1791488463d4e9c5adcd543962173e2f6cbd34/src/schema.rs#L927).
I have confirmed this by applying the patch below to my PR and running the test. The output confirms that the header is not set by the time we execute the query.

diff --git a/crates/fuel-core/src/schema/balance.rs b/crates/fuel-core/src/schema/balance.rs index b6b95228e8..956d5961bb 100644 --- a/crates/fuel-core/src/schema/balance.rs +++ b/crates/fuel-core/src/schema/balance.rs @@ -60,6 +60,10 @@ impl BalanceQuery { #[graphql(desc = "address of the owner")] owner: Address, #[graphql(desc = "asset_id of the coin")] asset_id: AssetId, ) -> async_graphql::Result<Balance> { + println!( + "header set: {}", + ctx.http_header_contains("REQUIRED_FUEL_BLOCK_HEIGHT") + ); let query = ctx.read_view()?; let base_asset_id = *ctx .data_unchecked::<ConsensusProvider>()

Maybe we could get the header somehow from the ExtensionContext, but this is going to be an ad hoc, undocumented solution and likely not readable.

What I think we should do instead is stick with Axum's middleware: get a handle to the OnchainDatabase from the CombinedDatabase (just because I don't want to wrap the instance to the CombinedDatabase inside an Arc), and use it to fetch the value of the last height in the middleware.

Let me know what you think.

Sorry completely lost this discussion, I think this makes sense. I guess you've already implemented this. Re-reviewing now...

for context: Headers cannot be used anymore, the reason being that subscriptions will return multiple graphql responses in a single HTTP response (and therefore have a single HeaderMap), while we require that each item in the response has its associated CURRENT_FUEL_BLOCK_HEIGHT field. Moreover the Response metadata for subscriptions is returned before the actual response body (as it is standard with HTTP).

in the end graphql has a (very undocumented) way to add an extensions field to responses, at the same level of data/errors field. This does not require to tweak anything at the HTTP level.

netrome

Nice stuff! I have some thoughts and comments, but no showstoppers.

netrome · 2024-12-05T21:01:50Z

crates/client/src/client.rs

+    pub async fn query_with_headers<ResponseData, Vars>(
+        &self,
+        q: Operation<ResponseData, Vars>,
+        headers: impl IntoIterator<Item = (String, String)>,


I assume this one https://docs.rs/http/1.2.0/http/header/struct.HeaderMap.html - makes sense 👍

crates/client/src/client.rs

netrome · 2024-12-05T21:19:23Z

crates/fuel-core/src/graphql_api/worker_service.rs

 };

 #[cfg(test)]
 mod tests;

+// The last known block height that was processed by the GraphQL service.
+pub static LAST_KNOWN_BLOCK_HEIGHT: OnceLock<AtomicU32> = OnceLock::new();


Is this so performance critical that we can't do a database lookup for this value? I'd default to querying this from the database, to ensure we don't have any weird consistency issues when this field and the db isn't in sync. I guess this isn't a big problem right now, but it just doesn't feel right to allow for inconsistencies in a field used to synchronize with other nodes.

netrome · 2024-12-05T21:21:20Z

crates/fuel-core/src/graphql_api/api_service.rs

+    .get()
+    //Maybe too strict?


I find it weird that this can be unset. We could always default to 0 as well, which would just mean that we're not in sync yet (which I presume we're not if this value hasn't been set).

It will be unset until the first block is processed. But if we are going to fetch the value directly from the DB then it won't be the case anymore. I am waiting to hear what approach @xgreenx prefers before refactoring this bit

crates/fuel-core/src/graphql_api/api_service.rs

Co-authored-by: Mårten Blankfors <[email protected]>

crates/fuel-core/src/graphql_api/required_fuel_block_height_extension.rs

crates/fuel-core/src/graphql_api/api_service.rs

crates/fuel-core/src/graphql_api/required_fuel_block_height_extension.rs

Torres-ssf · 2024-12-13T17:15:30Z

Hi @acerone85,

Could you kindly confirm if the response header for subscriptions is also intended to include current_fuel_block_height?

acerone85 · 2024-12-14T18:11:56Z

Hi @acerone85,

Could you kindly confirm if the response header for subscriptions is also intended to include current_fuel_block_height?

In the current implementation there is no response header for the current_fuel_block_height. Would it be desirable to have one?

…test

tests/tests/required_fuel_block_height_extension.rs

This reverts commit dcec472.

netrome

Looks good to me. The current solution pushes more responsibility on the caller for constructing and maintaining the headers in the client. While this is fine, I wonder if we should take more responsibility for this particular header to relieve users from having to think about it.

I'd imagine FuelClient to just hold a required_fuel_block_height parameter which it maintains internally (it could be exposed to users as well), and uses to construct the header when making requests. When receiving responses it would update the required_fuel_block_height with the returned block height for subsequent requests.

crates/fuel-core/src/graphql_api/required_fuel_block_height_extension.rs

xgreenx · 2025-01-31T16:23:57Z

crates/client/src/client.rs

+            serde_json::to_value(self.extensions.clone())?,
+        );
+
+        println!("{}", serde_json::to_string_pretty(&operation_json).unwrap());


Suggested change

println!("{}", serde_json::to_string_pretty(&operation_json).unwrap());

xgreenx · 2025-01-31T16:24:56Z

crates/client/src/client.rs

            .await
            .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;

+        println!("Response: {}", response);


Suggested change

println!("Response: {}", response);

xgreenx · 2025-01-31T16:26:24Z

crates/client/src/client.rs

+        let operation_object = operation_json
+            .as_object_mut()
+            .expect("Graphql operation is a valid json object");


I don't think that you can be sure that it is an object. It would be better if you checked it via if let Value::Object() ...

xgreenx · 2025-01-31T16:27:32Z

crates/fuel-core/src/graphql_api/api_service.rs

+    let response = schema.execute(req.0).await;
+
+    response.into()


Suggested change

let response = schema.execute(req.0).await;

response.into()

schema.execute(req.0).await.into()

xgreenx · 2025-01-31T16:27:55Z

crates/fuel-core/src/graphql_api/api_service.rs

 }

 async fn graphql_subscription_handler(
    schema: Extension<CoreSchema>,
    req: Json<Request>,
 ) -> Sse<impl Stream<Item = anyhow::Result<Event, serde_json::Error>>> {
+    let request = req.0;


Suggested change

let request = req.0;

xgreenx · 2025-01-31T16:28:19Z

crates/fuel-core/src/graphql_api/api_service.rs

    let stream = schema
-        .execute_stream(req.0)
+        .execute_stream(request)


Suggested change

.execute_stream(request)

.execute_stream(req.0)

xgreenx · 2025-01-31T17:44:53Z

crates/client/src/client.rs

@@ -223,6 +250,34 @@ impl FuelClient {
        Self::from_str(url.as_ref())
    }

+    pub fn set_header(


I think we don't use it and can remove

xgreenx · 2025-01-31T17:46:54Z

crates/client/src/client.rs

+        self.extensions.remove("required_fuel_block_height");
+        self
+    }
+
    /// Send the GraphQL query to the client.
    pub async fn query<ResponseData, Vars>(


What about fn subscribe? We also want to support setting heights for the submit_and_await_commit like queries

xgreenx · 2025-01-31T17:48:20Z

tests/tests/required_fuel_block_height_extension.rs

+
+#[tokio::test]
+async fn current_fuel_block_height_header_is_present_on_successful_request() {
+    // TODO: Figure out a way to get the current fuel block height from FuelClient queries


I think it is very important to extract the current height in fuel-core-client. But yeah, it can be done in a follow up PR. Could you create an issue to track it and put link in todos(below you have the same todos).

xgreenx

Also in proposal we wanted to return 412. But based on the implementation we just return an error. Can we customize the number?

acerone85 added 2 commits December 5, 2024 11:30

Start structuring middleware

a8f5f41

Use a static atomic to keep track of the current block height

778dd7c

acerone85 requested a review from Voxelot December 5, 2024 12:09

acerone85 requested review from xgreenx, Dentosal and MitchTurner as code owners December 5, 2024 12:09

acerone85 linked an issue Dec 5, 2024 that may be closed by this pull request

RPC Consistency Proposal #1897

Open

acerone85 requested a review from a team December 5, 2024 12:09

acerone85 added 2 commits December 5, 2024 12:13

Add changelog entry

d1b1c33

Add tests and fix middleware

b77506c

acerone85 self-assigned this Dec 5, 2024

acerone85 commented Dec 5, 2024

View reviewed changes

netrome previously approved these changes Dec 5, 2024

View reviewed changes

Update crates/fuel-core/src/graphql_api/api_service.rs

b764cd4

Co-authored-by: Mårten Blankfors <[email protected]>

acerone85 dismissed netrome’s stale review via b764cd4 December 6, 2024 13:34

acerone85 and others added 6 commits December 6, 2024 14:18

Set headers in fuel client

d9091e6

AxumRequest/AxumResponse -> http::Request/http::Response

90c411f

Fix compilation

625bf8f

Extension instead of Axum middleware

085d173

Merge branch 'master' into 1897-rpc-consistency-proposal

a98ef9b

Add response header

c95f09a

acerone85 commented Dec 12, 2024

View reviewed changes

crates/fuel-core/src/graphql_api/required_fuel_block_height_extension.rs Show resolved Hide resolved

acerone85 commented Dec 12, 2024

View reviewed changes

crates/fuel-core/src/graphql_api/api_service.rs Outdated Show resolved Hide resolved

acerone85 commented Dec 12, 2024

View reviewed changes

crates/fuel-core/src/graphql_api/required_fuel_block_height_extension.rs Outdated Show resolved Hide resolved

acerone85 commented Dec 12, 2024

View reviewed changes

crates/fuel-core/src/graphql_api/required_fuel_block_height_extension.rs Outdated Show resolved Hide resolved

acerone85 commented Dec 12, 2024

View reviewed changes

crates/fuel-core/src/graphql_api/required_fuel_block_height_extension.rs Outdated Show resolved Hide resolved

Better handling of response

6099651

acerone85 and others added 16 commits December 16, 2024 19:56

Remove use of Arc<Mutex<_>>

0b1e31f

Remove stray comment

8e1b0a5

WIP

abe13c2

Example of how it can be implemented via extensions

eae1e22

Merge branch 'master' into 1897-rpc-consistency-proposal

0a3da1f

Add current height header on failed requests

e7e1db7

Return current level after request has been executed

6b76769

WIP: Use only graphql extensions: tests still to be adjusted

b6f206d

Merge branch 'master' into 1897-rpc-consistency-proposal

2f52aaa

Add capability to the client to set required_fuel_block_height + fix …

cd3be50

…test

Fix other tests

dbc5cdc

Rename test file

c5db12f

Fix BlockHeight base format in extension error

24b8d69

Fix typo

4a8d839

Merge branch 'master' into 1897-rpc-consistency-proposal

51ec4a9

Fix compilation error after rename

37b03f1

acerone85 mentioned this pull request Jan 21, 2025

Extend fuel-client queries to return the "extensions" map from responses #2605

Open

acerone85 commented Jan 21, 2025

View reviewed changes

tests/tests/required_fuel_block_height_extension.rs Outdated Show resolved Hide resolved

acerone85 and others added 7 commits January 21, 2025 13:16

Reference follow-up issue

396a953

Fix rustfmt

af0c6d6

Fix subscriptions feature

b1e89d6

Use HashMap::new instead of Default::default

6e50f3b

Downgrade netlink-proto

dcec472

Revert "Downgrade netlink-proto"

8c6af1f

This reverts commit dcec472.

Merge branch 'master' into 1897-rpc-consistency-proposal

818a717

This was referenced Jan 29, 2025

feat: support optimistic concurrency control headers FuelLabs/fuels-ts#3462

Draft

Review and Add Special Headers for Optimistic Concurrency Control FuelLabs/fuels-ts#3443

Open

netrome approved these changes Jan 29, 2025

View reviewed changes

crates/fuel-core/src/graphql_api/required_fuel_block_height_extension.rs Show resolved Hide resolved

xgreenx reviewed Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RPC Consistency Proposal #2473

RPC Consistency Proposal #2473

acerone85 commented Dec 5, 2024 •

edited

Loading

acerone85 Dec 5, 2024

netrome Dec 5, 2024

acerone85 Dec 6, 2024

acerone85 Dec 5, 2024

netrome Dec 5, 2024

xgreenx Dec 6, 2024

acerone85 Dec 6, 2024

netrome Jan 29, 2025

acerone85 Jan 30, 2025

netrome left a comment

netrome Dec 5, 2024

netrome Dec 5, 2024

netrome Dec 5, 2024

acerone85 Dec 6, 2024

Torres-ssf commented Dec 13, 2024

acerone85 commented Dec 14, 2024

netrome left a comment

xgreenx Jan 31, 2025

xgreenx Jan 31, 2025

xgreenx Jan 31, 2025

xgreenx Jan 31, 2025

xgreenx Jan 31, 2025

xgreenx Jan 31, 2025

xgreenx Jan 31, 2025

xgreenx Jan 31, 2025

xgreenx Jan 31, 2025

xgreenx left a comment

RPC Consistency Proposal #2473

Are you sure you want to change the base?

RPC Consistency Proposal #2473

Conversation

acerone85 commented Dec 5, 2024 • edited Loading

Linked Issues/PRs

Manual testing

TODO:

Description

Checklist

Before requesting review

After merging, notify other teams

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netrome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Torres-ssf commented Dec 13, 2024

acerone85 commented Dec 14, 2024

netrome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xgreenx left a comment

Choose a reason for hiding this comment

acerone85 commented Dec 5, 2024 •

edited

Loading