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.

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

Checking that the response header is set, with a node listening on port 4000

curl -X POST -D - http://localhost:4000/v1/graphql \
-H "Content-Type: application/json" \
-d '{
    "query": "{ contract(id:\"0x7e2becd64cd598da59b4d1064b711661898656c6b1f4918a787156b8965dc83c\") { id bytecode } }"
}'
HTTP/1.1 200 OK
content-type: application/json
current_fuel_block_height: 219
content-length: 10993
access-control-allow-origin: *
access-control-allow-methods: *
access-control-allow-headers: *
date: Mon, 16 Dec 2024 13:05:04 GMT

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.

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 👍

netrome · 2024-12-05T21:09:30Z

crates/client/src/client.rs

+            .query_with_headers(
+                query,
+                vec![(
+                    "REQUIRED_FUEL_BLOCK_HEIGHT".to_string(),


Hmm, I'm not 100% satisfied with this name. Especially since we provide a block height as input to some queries. When squinting your eyes it could be confusing if the "required_bock_height" is part of the query, when it is more a notion of the state of the node to ensure we're in sync.

How would REQUIRED_CHAIN_TIP_HEIGHT sound to you? Here it's clear that we're referring to the block height of the chain tip.

I agree that we could have a better name (maybe CHAIN_HEAD instead of CHAIN_TIP) , but if we change it we have to notify other teams as well as we are deviating from the spec: see for example FuelLabs/fuels-ts#3443

netrome · 2024-12-05T21:15:30Z

crates/client/src/client.rs

+    pub async fn balance_with_required_block_header(
+        &self,
+        owner: &Address,
+        asset_id: Option<&AssetId>,
+        required_block_height: u32,
+    ) -> io::Result<u128> {


I'm a bit hesitant to have the required_block_height be a parameter here. It feels like we're cluttering the interface and are increasing the cognitive load for our users. This feels more like a state that should be maintained by the client and not even directly exposed to the user.

I'd imagine the client maintaining a chain_tip parameter locally which is updated whenever it receives a response, and it's appended to the header of all requests made.

What are your thoughts on this?

If I understood correctly you suggest to change the client to have the headers it will send in the requests in its state. I think that makes a lot of sense, let me have a go :)

That was a good suggestion :) I have implemented functionalities to set/remove headers for the client in d9091e6 and it looks much better.

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

netrome · 2024-12-05T21:26:42Z

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

+        Request as AxumRequest,
+        Response as AxumResponse,


Nit: This is a re-export so I'd alias it to HttpRequest, or even better just import axum::http and keep the namespacing: http::Request.

agreed and done: 90c411f

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?

rafal-ch · 2024-12-16T13:38:23Z

crates/client/src/client.rs

+    ) -> Result<&mut Self, Error> {
+        let header_value: HeaderValue = value
+            .try_into()
+            .map_err(|_err| anyhow::anyhow!("Cannot parse value for header"))?;


If we now have an proper Error, maybe we should introduce specific variant instead of anyhow?

rafal-ch · 2024-12-16T13:44:30Z

crates/client/src/client.rs

+        for (header_name, header_value) in self.headers.iter() {
+            request_builder = request_builder.header(header_name, header_value);
+        }


Can we use headers() to avoid manual loop, like:

request_builder = request_builder.headers(&self.headers);

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

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

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

Co-authored-by: Rafał Chabowski <[email protected]>

Torres-ssf · 2024-12-16T16:40:29Z

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

@acerone85 I believe it would because I have seen the following scenario happening a few times when submitting TXs sequentially:

flowchart TD
    %% Transaction n1
    A1[Start TX n1] --> A2[Query coinsToSpend to fetch resources]
    A2 --> A3[Submit TX n1 using submitAndAwaitStatus]
    A3 --> A4[Await status update]
    A4 --> A5[Success]
    A5 --> B1[Start TX n2]

%% Transaction n+1
    B1 --> B2[Query coinsToSpend]
    B2 --> B3[Query hits unsync node, fetching a spent UTXO on TX n1]
    B3 --> B4[Submit TX n2 using submitAndAwaitStatus]
    B4 --> B5[Sync node validates UTXO was spent]
    B5 --> B6[Error]

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 added 12 commits December 14, 2024 20:14

Use BlockHeight type when possible

3b81537

Fix compilation error

11e6876

Use Request data to retrieve required block height

88b9c52

Remove useless .into

c5f82ff

Move up const definitions

844e4e6

Remove useless .into()

71f8087

Inject request data in subscription handler

cbea694

Improvements

da7f057

Add documentation

f5e63f6

Do not fetch the read view twice

5f4500b

Remove commented code

37dd63e

Typo

9913620

rafal-ch reviewed Dec 16, 2024

View reviewed changes

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

f849eb5

Co-authored-by: Rafał Chabowski <[email protected]>

acerone85 and others added 11 commits December 16, 2024 18:32

Improve setting headers in client

1fddd80

Revert to setting header in graphql extension when possible

e1f71d4

Update comment

be7f927

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

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 left a comment

netrome Dec 5, 2024

netrome Dec 5, 2024

acerone85 Dec 6, 2024

netrome Dec 5, 2024

acerone85 Dec 6, 2024

acerone85 Dec 6, 2024

netrome Dec 5, 2024

netrome Dec 5, 2024

acerone85 Dec 6, 2024

netrome Dec 5, 2024

acerone85 Dec 6, 2024

Torres-ssf commented Dec 13, 2024

acerone85 commented Dec 14, 2024

rafal-ch Dec 16, 2024

rafal-ch Dec 16, 2024

Torres-ssf commented Dec 16, 2024

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Torres-ssf commented Dec 16, 2024

acerone85 commented Dec 5, 2024 •

edited

Loading