Rpc consistency proposal: Wait for up to a number of blocks specified in configuration before failing #2666

acerone85 · 2025-02-04T14:19:06Z

I am using a rebase heavy workflow to handle stacked PR. If you want to push updates to this PR and make sure that the whole PR is consistent with the target branch, please do the following instead of `git merge 1897-rpc-consistency-proposal`:

git checkout ac@rpc-consistency-proposal-wait-for-block
git fetch origin
git rebase --onto origin/1897-rpc-consistency-proposal 1897-rpc-consistency-proposal
# Fix conflicts if necessary
git branch -f 1897-rpc-consistency-proposal origin/1897-rpc-consistency-proposal
git push -f origin

Linked Issues/PRs

Follow up on #1897

This PR implements a mechansim that allows a node operator to configure a tolerance level, in number of blocks, for the required fuel_block_height extension. When checking that the current_fuel_block_height is at least the required_fuel_block_height, the node will check first whether the current_fuel_block_height is at most required_fuel_block_height_tolerance "too far away" from the required_fuel_block_height. If that is the case, the node will check every second if the precondition has been met before fulfilling the request. Otherwise, the request will fail because the precondition has not been met.

Because the HTTP server of the node has a timeout layer, requests by a node that has lost synchronisation with the network will eventually fail.

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?

xgreenx · 2025-02-07T05:25:32Z

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

+                while current_block_height < *required_block_height {
+                    tokio::time::sleep(SLEEP_DURATION).await;
+                    match view.latest_block_height() {


It would be nice if we could subscribe for the height that we need and notify all dependent tasks.

BTW, without #2646 it is very expensive operation, and do it for each query can be a lot. Before releasing RPC consistancy, we need merge this optimization

If we subscribe to a channel that broadcasts the height when it is updated in the offchain worker, would that still require #2646?

xgreenx · 2025-02-07T05:28:09Z

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

+                        Ok(block_height) => {
+                            current_block_height = block_height;
+                        }
+                    }
                }
            }
        }


After you wait for the required height, the view that you have inside of the context is old. We need to update it.

I think it would be better if we combined ViewExtension and RequiredFuelBlockHeightInner together, where you create a view only if you reach the required height.

I tried a different approach where we subscribe to changes to the block height in the extension. It does not require merging the ViewExtension and RequiredFuelBlockHeightInner, curious to hear your opinion

You need to update the ReadView that was created by the ViewExtension. If required height is 9 blocks higher than the current height, ReadView will be 9 blocks old.

You need to actualize the view to be at required_block_height. It is where you need to create the view second time, and it is where merging ViewExtension and RequiredFuelBlockHeightInner can help to avoid creation of 2 views.

yep, thanks for explaining.
I had a look, and merging the two views will totally help. Will amend

xgreenx · 2025-02-11T04:07:27Z

CHANGELOG.md

@@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Changed 
 - [2473](https://github.com/FuelLabs/fuel-core/pull/2473): Graphql requests and responses make use of a new `extensions` object to specify request/response metadata. A request `extensions` object can contain an integer-valued `required_fuel_block_height` field. When specified, the request will return an error unless the node's current fuel block height is at least the value specified in the `required_fuel_block_height` field. All graphql responses now contain an integer-valued `current_fuel_block_height` field in the `extensions` object, which contains the block height of the last block processed by the node.

+### Added
+- [2666](https://github.com/FuelLabs/fuel-core/pull/2666) Added `--required-fuel-block-height-tolerance` CLI arg (default: `10`). Requests with `required_fuel_block_height` set will wait if within tolerance of `current_fuel_block_height` instead of failing.
+
 ## [Version 0.41.5]


It seems you need to merge with master, because the latest version is 0.41.6=)

xgreenx · 2025-02-11T04:08:11Z

bin/fuel-core/src/cli/run/graphql.rs

@@ -66,6 +66,13 @@ pub struct GraphQLArgs {
    #[clap(long = "api-request-timeout", default_value = "30s", env)]
    pub api_request_timeout: humantime::Duration,

+    #[clap(
+        long = "required-fuel-block-height-tolerance",


Suggested change

long = "required-fuel-block-height-tolerance",

long = "graphql_required-fuel-block-height-tolerance",

Also it would be nice to have a comment to describe the functionality behind this CLI argument.

xgreenx · 2025-02-11T04:14:28Z

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

+                        Ok(block_height) => {
+                            current_block_height = block_height;
+                        }
+                    }
                }
            }
        }


You need to update the ReadView that was created by the ViewExtension. If required height is 9 blocks higher than the current height, ReadView will be 9 blocks old.

You need to actualize the view to be at required_block_height. It is where you need to create the view second time, and it is where merging ViewExtension and RequiredFuelBlockHeightInner can help to avoid creation of 2 views.

xgreenx · 2025-02-11T04:15:35Z

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

+                    self.block_height_subscription_handle
+                        .clone()
+                        .wait_for_block_height(*required_block_height)


You can make wait_for_block_height to work with &self to avoid cloning here

xgreenx · 2025-02-11T04:16:34Z

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

+                // If the node is lagging behind w.r.t. the required fuel block height,
+                // but within an acceptable interval, wait until the current block height
+                // matches the required fuel block height.
+                // TODO: Add a timeout to prevent the node from waiting indefinitely.


I think you can add a timeout in 1 minute + number of blocks(in seconds) that you need to wait

xgreenx · 2025-02-11T04:19:32Z

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

+        height: BlockHeight,
+    ) -> anyhow::Result<()> {
+        loop {
+            let current_height = *self.rx_block_height.borrow_and_update();


You can do just borrow, it still should work

happy to do that, but that will introduce busy waiting as we cannot await on the value being changed on the sender side. Is that okay?

xgreenx · 2025-02-11T04:29:07Z

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

+        &mut self,
+        height: BlockHeight,
+    ) -> anyhow::Result<()> {
+        loop {


While it is very simple to implement it with the tokio::watch, it is still expensive from an execution perspective.

Let's imagine the current height is 10. The required height for 10k requests is 20.

10 times, each task related to the request will wake up; it is 100k wakeups that check the state(it is read of the RWLock) of the watch and wait again.

If you have the list of subscribers for each block height, we can avoid unnecessary wakeups. And it should be very easy to maintain this list via one mutex. You can have BTreeMap<BlockHeight, Vec<tokio::sync::oneshot::Sender<()>>>, where you register all listeners, and on each block you just process most left listeners.

xgreenx · 2025-02-11T04:29:41Z

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

+    pub async fn wait_for_block_height(
+        &mut self,
+        height: BlockHeight,
+    ) -> anyhow::Result<()> {


You can return the new block height from wait_for_block_height. In this case you don't need latest_seen_block_height

xgreenx · 2025-02-11T04:30:53Z

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

@@ -745,5 +779,7 @@ where
        da_compression_config,
        continue_on_error,
        base_asset_id: *consensus_parameters.base_asset_id(),
+        // TODO: Should we recover the block height from the DB?


Yes, we need to get the block height from the off_chain_database

xgreenx · 2025-02-14T05:10:21Z

bin/fuel-core/src/cli/run/graphql.rs

+    /// The minimum time that the node will wait to catch up to the required block height
+    /// of a graphql request. The actual time may be longer and proportional to how
+    /// much the node is behind the required block height.
+    #[clap(
+        long = "graphql-required-block-height-min-timeout-seconds",
+        default_value = "10",
+        env
+    )]
+    pub required_fuel_block_height_min_timeout: u32,


I think this CLI argument is overkill, it is very hard to understand what it is doing, if you don't look into the code. And even then, it is very hard to trigger this timeout

xgreenx · 2025-02-14T05:10:55Z

bin/fuel-core/src/cli/run/graphql.rs

+    /// The minimum time that the node will wait to catch up to the required block height
+    /// of a graphql request. The actual time may be longer and proportional to how
+    /// much the node is behind the required block height.
+    #[clap(
+        long = "graphql-required-block-height-min-timeout-seconds",
+        default_value = "10",
+        env
+    )]
+    pub required_fuel_block_height_min_timeout: u32,


Suggested change

/// The minimum time that the node will wait to catch up to the required block height

/// of a graphql request. The actual time may be longer and proportional to how

/// much the node is behind the required block height.

#[clap(

long = "graphql-required-block-height-min-timeout-seconds",

default_value = "10",

env

)]

pub required_fuel_block_height_min_timeout: u32,

/// The minimum time that the node will wait to catch up to the required block height

/// of a graphql request. The actual time may be longer and proportional to how

/// much the node is behind the required block height.

#[clap(

long = "graphql-required-block-height-min-timeout-seconds",

default_value = "30s",

env

)]

pub required_fuel_block_height_min_timeout: humantime::Duration,

xgreenx · 2025-02-14T05:12:45Z

crates/fuel-core/src/graphql_api.rs

+    /// Minimum time to wait before dropping the request if the node is lagging behind the required
+    /// fuel block height. The actual timeout in seconds is `required_fuel_block_height_min_timeout + lag`,
+    /// where `lag` is the number of blocks the node is lagging behind the required fuel block height.
+    pub required_fuel_block_height_min_timeout: Duration,


I proposed to add lag to avoid making the required_fuel_block_height_min_timeout configurable via the CLI=D If you plan to have CLI argument, then we need to remove lag for simplicity of the logic. If you remove the CLI argument, then we need to have a lag + some random timeout like 1 minute for example.

Happy to have lag + random timeout, wondering if 1 minute is too high (the default timeout for all RPC requests is 30 seconds IIRC)

I think we can stick with CLI argument without lag, it allows us to test timeout logic(it is what you did and I think it makes sense):)

ok, just to confirm we remove the lag altogether and we always wait for a timeout?

xgreenx · 2025-02-14T05:43:38Z

Sorry, clicked wrong button=D Didn't plan to close it=)

xgreenx

Thank you! The change looks very nice

… height

…n executed

…ting the request

…, removed usage of calling of second `view`. Changed a little bit the test, to be sure that behavior right

rafal-ch

Partial review, will continue later.
No issues so far.

rafal-ch · 2025-02-14T14:35:42Z

CHANGELOG.md

@@ -12,12 +12,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

 ### Added
 - [2150](https://github.com/FuelLabs/fuel-core/pull/2150): Upgraded `libp2p` to `0.54.1` and introduced `ConnectionLimiter` to limit pending incoming/outgoing connections.
+- [2666](https://github.com/FuelLabs/fuel-core/pull/2666) Added `--graphql-block-height-tolerance` (default: `10`) and `--graphql-block-height-min-timeout-secs` (default: `30`) CLI args. Requests with `required_fuel_block_height` set will wait if within tolerance of `current_fuel_block_height` instead of failing. The number of seconds that the node will wait is proportional to the number of blocks `lag-behind-blocks` that the node is lagging behind the required fuel block height, and equal to `graphql-block-height-min-timeout-secs + lag-behind-blocks.


Maybe we can be more concise, because we don't need to treat changelog as a feature documentation :)

Suggested change

- [2666](https://github.com/FuelLabs/fuel-core/pull/2666) Added `--graphql-block-height-tolerance` (default: `10`) and `--graphql-block-height-min-timeout-secs` (default: `30`) CLI args. Requests with `required_fuel_block_height` set will wait if within tolerance of `current_fuel_block_height` instead of failing. The number of seconds that the node will wait is proportional to the number of blocks `lag-behind-blocks` that the node is lagging behind the required fuel block height, and equal to `graphql-block-height-min-timeout-secs + lag-behind-blocks.

Added two new CLI arguments to control the GraphQL queries consistency: `--graphql-block-height-tolerance` (default: `10`) and `--graphql-block-height-min-timeout-secs` (default: `30`). If a request requires a specific block height and the node is slightly behind, it will wait instead of failing. The wait time increases with the block lag, starting at the minimum timeout.

rafal-ch · 2025-02-14T14:45:12Z

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

+use tokio::sync::oneshot;
+
+#[derive(Default)]
+


Suggested change

netrome

Looks generally good to me. I think the block height subscription can be simplified.

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

… in configuration before failing (#2666) ## Linked Issues/PRs  Follow up on #1897 This PR implements a mechansim that allows a node operator to configure a tolerance level, in number of blocks, for the required fuel_block_height extension. When checking that the `current_fuel_block_height` is at least the `required_fuel_block_height`, the node will check first whether the `current_fuel_block_height` is at most `required_fuel_block_height_tolerance` "too far away" from the `required_fuel_block_height`. If that is the case, the node will check every second if the precondition has been met before fulfilling the request. Otherwise, the request will fail because the precondition has not been met. Because the HTTP server of the node has a timeout layer, requests by a node that has lost synchronisation with the network will eventually fail. ## Description  ## Checklist - [ ] Breaking changes are clearly marked as such in the PR description and changelog - [ ] New behavior is reflected in tests - [ ] [The specification](https://github.com/FuelLabs/fuel-specs/) 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](https://github.com/FuelLabs/fuels-rs/) - [ ] [Sway compiler](https://github.com/FuelLabs/sway/) - [ ] [Platform documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+) (for out-of-organization contributors, the person merging the PR will do this) - [ ] Someone else? --------- Co-authored-by: green <[email protected]>

acerone85 requested review from xgreenx, Dentosal, MitchTurner and netrome as code owners February 4, 2025 14:19

acerone85 force-pushed the ac@rpc-consistency-proposal-wait-for-block branch from cf942ca to ebe0ffa Compare February 4, 2025 15:12

acerone85 requested review from rafal-ch, rymnc and AurelienFT as code owners February 4, 2025 15:12

xgreenx reviewed Feb 7, 2025

View reviewed changes

xgreenx reviewed Feb 11, 2025

View reviewed changes

acerone85 force-pushed the ac@rpc-consistency-proposal-wait-for-block branch 2 times, most recently from a3890ba to 7496006 Compare February 11, 2025 13:54

Base automatically changed from 1897-rpc-consistency-proposal to master February 12, 2025 20:43

acerone85 mentioned this pull request Feb 13, 2025

Optimize how ReadViews are fetched #2713

Open

acerone85 force-pushed the ac@rpc-consistency-proposal-wait-for-block branch from 9821876 to 331976b Compare February 13, 2025 13:10

Torres-ssf mentioned this pull request Feb 13, 2025

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

Draft

4 tasks

xgreenx reviewed Feb 14, 2025

View reviewed changes

xgreenx closed this Feb 14, 2025

xgreenx deleted the ac@rpc-consistency-proposal-wait-for-block branch February 14, 2025 05:16

xgreenx restored the ac@rpc-consistency-proposal-wait-for-block branch February 14, 2025 05:42

xgreenx reopened this Feb 14, 2025

xgreenx previously approved these changes Feb 14, 2025

View reviewed changes

xgreenx dismissed their stale review via 98b0ca7 February 14, 2025 06:30

acerone85 added 6 commits February 14, 2025 12:58

Add configuration argument for required fuel block height threshold

0f8bbe9

Wait until the required fuel block height is reached

ad0abd1

Changelog

439670f

Use a subscription handler to get notified about changes to the block…

64a21b8

… height

Get rid of mutex, clone receiver instead

7a14849

Fix

bb9d169

acerone85 and others added 19 commits February 14, 2025 12:58

Update CHANGELOG

3dd65dc

Refactor to use BTreeMap shared BTreeMap of senders

78cbca2

Minor improvements

f2e5639

Actually send notifications to block height subscribers

9dac629

Timeout when waiting for too long

8791206

Make min timeout configurable

bd9a4de

Fix compilation

41a7c55

Force ReadViewProvider to always fetch the latest view

01d5cde

Bring back fetching up to date height from view after request has bee…

4e74924

…n executed

Rework block subscription handlers in separate module

2d306b5

Plug new module into extension

c015269

Avoid fetching the current height from the database view before execu…

f4d4d3d

…ting the request

Remove view extension

75adc78

Remove gas price analysis Cargo.lock

ce3479c

Fix compilation

61a7166

Fmt

9226ca3

Tests

1c478ac

Applied some comments from the PR. Simplified some code. Removed todo…

3c09e8c

…, removed usage of calling of second `view`. Changed a little bit the test, to be sure that behavior right

During manual block production wait for off chain database to catch up

6329c4f

rafal-ch reviewed Feb 14, 2025

View reviewed changes

acerone85 force-pushed the ac@rpc-consistency-proposal-wait-for-block branch from 98b0ca7 to 6329c4f Compare February 14, 2025 17:05

acerone85 added 3 commits February 14, 2025 17:20

Remove newline

5280457

Rewrite changelog

98adddf

Update Changelog entry

b08a5b4

xgreenx approved these changes Feb 14, 2025

View reviewed changes

netrome approved these changes Feb 16, 2025

View reviewed changes

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

netrome mentioned this pull request Feb 16, 2025

refactor: Use tokio::watch in block height subscription #2721

Closed

acerone85 merged commit c0975a0 into master Feb 17, 2025
33 checks passed

acerone85 deleted the ac@rpc-consistency-proposal-wait-for-block branch February 17, 2025 00:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rpc consistency proposal: Wait for up to a number of blocks specified in configuration before failing #2666

Rpc consistency proposal: Wait for up to a number of blocks specified in configuration before failing #2666

acerone85 commented Feb 4, 2025 •

edited

Loading

xgreenx Feb 7, 2025

xgreenx Feb 7, 2025

acerone85 Feb 10, 2025

xgreenx Feb 7, 2025

acerone85 Feb 10, 2025

xgreenx Feb 11, 2025

acerone85 Feb 11, 2025

xgreenx Feb 11, 2025

acerone85 Feb 11, 2025

xgreenx Feb 11, 2025

xgreenx Feb 11, 2025

xgreenx Feb 11, 2025

xgreenx Feb 11, 2025

xgreenx Feb 11, 2025

xgreenx Feb 11, 2025

acerone85 Feb 11, 2025

xgreenx Feb 11, 2025

xgreenx Feb 11, 2025

xgreenx Feb 11, 2025

xgreenx Feb 14, 2025

xgreenx Feb 14, 2025

xgreenx Feb 14, 2025

acerone85 Feb 14, 2025

xgreenx Feb 14, 2025

acerone85 Feb 14, 2025

xgreenx Feb 14, 2025

xgreenx commented Feb 14, 2025

xgreenx left a comment

rafal-ch left a comment

rafal-ch Feb 14, 2025

rafal-ch Feb 14, 2025

netrome left a comment

	long = "required-fuel-block-height-tolerance",
	long = "graphql_required-fuel-block-height-tolerance",

Rpc consistency proposal: Wait for up to a number of blocks specified in configuration before failing #2666

Rpc consistency proposal: Wait for up to a number of blocks specified in configuration before failing #2666

Conversation

acerone85 commented Feb 4, 2025 • edited Loading

I am using a rebase heavy workflow to handle stacked PR. If you want to push updates to this PR and make sure that the whole PR is consistent with the target branch, please do the following instead of git merge 1897-rpc-consistency-proposal:

Linked Issues/PRs

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

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

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 commented Feb 14, 2025

xgreenx left a comment

Choose a reason for hiding this comment

rafal-ch left a 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

acerone85 commented Feb 4, 2025 •

edited

Loading

I am using a rebase heavy workflow to handle stacked PR. If you want to push updates to this PR and make sure that the whole PR is consistent with the target branch, please do the following instead of `git merge 1897-rpc-consistency-proposal`: