Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify block producer to take into account the total gas used by the L1 transactions #1785

Merged
merged 36 commits into from
Apr 3, 2024

Conversation

MitchTurner
Copy link
Member

closes #1750

@MitchTurner MitchTurner force-pushed the take-into-account-gas-when-including-L1-txs branch from 6b80189 to 2112149 Compare March 29, 2024 00:42
@MitchTurner MitchTurner changed the title The block producer should take into account the total gas used by the L1 transactions The block producer takes into account the total gas used by the L1 transactions Mar 29, 2024
@MitchTurner MitchTurner changed the title The block producer takes into account the total gas used by the L1 transactions Modify block producer to take into account the total gas used by the L1 transactions Mar 29, 2024
@MitchTurner MitchTurner requested review from Voxelot and a team March 29, 2024 23:45
@MitchTurner MitchTurner marked this pull request as ready for review March 30, 2024 00:03
.unwrap_or_default()
.iter()
.map(|event| event.cost())
.sum();
Copy link
Member

@Voxelot Voxelot Mar 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While probably very unlikely (and should be prevented by the contract on L1), I wonder if we need to do anything here to handle an overflow in case the cumulative max_gas is somehow too large.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change it to a saturated add and fold.

long = "max-database-cache-size",
default_value_t = DEFAULT_DATABASE_CACHE_SIZE,
env
long = "max-database-cache-size",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you revert back indent changes in this file, please?=)

Comment on lines 135 to 136
(starting_from.0..=highest.0)
.map(|height| get_gas_cost_for_height(height, sync))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid that the current usage of the Genesis da block height will lead to iterations from 0..5M.

It may not be a huge problem in the long term, but it may slow down the start of the fuel core for local development.

&self,
height: &primitives::DaBlockHeight,
) -> anyhow::Result<primitives::DaBlockHeight> {
starting_from: &DaBlockHeight,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice, maybe, to move this logic to block producer.

In this case, the block producer will have control over the number of iterations. Because we had a long delay/finalization and we have 100 blocks with a maxed gas limit, you will do 100 iterations while you need only one.

We could use something like:

#[async_trait::async_trait]
pub trait Relayer: Send + Sync {
    /// Wait for the relayer to reach at least this height and return the
    /// latest height (which is guaranteed to be >= height).
    async fn wait_for_at_least(
        &self,
        height: &DaBlockHeight,
    ) -> anyhow::Result<DaBlockHeight>;

    /// Returns the gas cost of events at the given height.
    async fn gas_cost_at_height(
        &self,
        height: &DaBlockHeight,
    ) -> anyhow::Result<Word>;
}

.database()
.storage::<fuel_core_relayer::storage::EventsHistory>()
.get(&da_height)?
.map(|cow| cow.into_owned())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to call into_owned here. It is the place where we want to use references if possible.

#[test_case(Some(0), Some(0) => false; "if something updated with same thing then stop waiting")]
#[test_case(Some(0), None => true; "if something updated with nothing then keep waiting")]
#[test_case(Some(0), Some(1) => false; "if something updated with something new then stop waiting")]
fn can_update_sync(previous_state: Option<u64>, new_state: Option<u64>) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we want to lose these tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some tests on Task for the same coverage.

@MitchTurner MitchTurner requested a review from xgreenx April 1, 2024 20:41
@MitchTurner MitchTurner requested a review from a team April 3, 2024 00:21
.into())
.into());
}
for height in previous_da_height.0..=heighest.0 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for height in previous_da_height.0..=heighest.0 {
for height in previous_da_height.0 + 1..=heighest.0 {

We've already included events at previous_da_height, so we need to skip it. Otherwise, we will stuck on that height forever.

I think it also would be nice to add a test to test that case where each da height contains transactions to fill block_gas_limit

@MitchTurner MitchTurner enabled auto-merge (squash) April 3, 2024 17:17
@MitchTurner MitchTurner merged commit dba4b4a into master Apr 3, 2024
27 of 29 checks passed
@MitchTurner MitchTurner deleted the take-into-account-gas-when-including-L1-txs branch April 3, 2024 17:58
@xgreenx xgreenx mentioned this pull request Apr 4, 2024
xgreenx added a commit that referenced this pull request Apr 5, 2024
## Version v0.24.2

### Changed

#### Breaking
- [#1798](#1798): add nonce to
relayed transactions and also hash full messages in the inbox root.

### Fixed

- [#1802](#1802): Fixed a
runtime panic that occurred when restarting a node. The panic was caused
by an invalid database commit while loading an existing off-chain
database. The invalid commit is removed in this PR.
- [#1803](#1803): Produce
block when da height haven't changed.
- [#1795](#1795): Fixed the
building of the `fuel-core-wasm-executor` to work outside of the
`fuel-core` context. The change uses the path to the manifest file of
the `fuel-core-upgradable-executor` to build the
`fuel-core-wasm-executor` instead of relying on the workspace.

## What's Changed
* Weekly `cargo update` by @github-actions in
#1794
* Improvements for the `fuel-core-upgradable-executor` build script by
@xgreenx in #1795
* Use full hash for messages in inbox_root and add nonce to relayed
transactions by @Voxelot in
#1798
* Modify block producer to take into account the total gas used by the
L1 transactions by @MitchTurner in
#1785
* Fix: Produce block when da height haven't changed by @xgreenx in
#1803
* fix: Fix commit error on GraphQL service startup by @bvrooman in
#1802


**Full Changelog**:
v0.24.1...v0.24.2
crypto523 pushed a commit to crypto523/fuel-core that referenced this pull request Oct 7, 2024
## Version v0.24.2

### Changed

#### Breaking
- [#1798](FuelLabs/fuel-core#1798): add nonce to
relayed transactions and also hash full messages in the inbox root.

### Fixed

- [#1802](FuelLabs/fuel-core#1802): Fixed a
runtime panic that occurred when restarting a node. The panic was caused
by an invalid database commit while loading an existing off-chain
database. The invalid commit is removed in this PR.
- [#1803](FuelLabs/fuel-core#1803): Produce
block when da height haven't changed.
- [#1795](FuelLabs/fuel-core#1795): Fixed the
building of the `fuel-core-wasm-executor` to work outside of the
`fuel-core` context. The change uses the path to the manifest file of
the `fuel-core-upgradable-executor` to build the
`fuel-core-wasm-executor` instead of relying on the workspace.

## What's Changed
* Weekly `cargo update` by @github-actions in
FuelLabs/fuel-core#1794
* Improvements for the `fuel-core-upgradable-executor` build script by
@xgreenx in FuelLabs/fuel-core#1795
* Use full hash for messages in inbox_root and add nonce to relayed
transactions by @Voxelot in
FuelLabs/fuel-core#1798
* Modify block producer to take into account the total gas used by the
L1 transactions by @MitchTurner in
FuelLabs/fuel-core#1785
* Fix: Produce block when da height haven't changed by @xgreenx in
FuelLabs/fuel-core#1803
* fix: Fix commit error on GraphQL service startup by @bvrooman in
FuelLabs/fuel-core#1802


**Full Changelog**:
FuelLabs/fuel-core@v0.24.1...v0.24.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The block producer should take into account the total gas used by the L1 transactions
3 participants