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

Set Latest Recorded Height on startup #2603

Merged
merged 22 commits into from
Jan 22, 2025

Conversation

MitchTurner
Copy link
Member

@MitchTurner MitchTurner commented Jan 20, 2025

Linked Issues/PRs

closes #2613

Description

This improves the behavior for debugging the code--for long-running sessions it won't change anything. If this isn't set on. startup, it will be delayed each time you restart the node unless it runs long enough to receive DA costs. These changes persists the expectations from the first run.

Checklist

  • New behavior is reflected in tests

@MitchTurner MitchTurner marked this pull request as ready for review January 20, 2025 19:07
@MitchTurner MitchTurner self-assigned this Jan 20, 2025
let starting_recorded_height = match self.gas_price_db.get_recorded_height()? {
Some(height) => height,
None => {
let mut storage_tx = self.gas_price_db.begin_transaction()?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where do you commit this transaction? Also, maybe it makes sense to allow customization of the gas price height from CLI? For example: you want to sync with the network from genesis block, but you only want to calculate the gas price starting the latest block height, to avoid huge debt because data was not posted during previous blocks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Where do you commit this transaction?

Hmm. I'll double check that it makes sense. I got the test to pass so I assumed it was okay but you're right. Might be able to move this to the sync section.

Also, maybe it makes sense to allow customization of the gas price height from CLI?

Yeah. I think that's a good idea.

@MitchTurner MitchTurner changed the base branch from master to fix-url-scheme January 20, 2025 22:59
Copy link
Collaborator

@xgreenx xgreenx left a comment

Choose a reason for hiding this comment

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

I meant: Maybe we want to have a new CLI argument to allow control of it from outside=D But is can be a separate PR, if you think you don't need this feature for testing

Base automatically changed from fix-url-scheme to master January 21, 2025 09:08
@MitchTurner
Copy link
Member Author

I meant: Maybe we want to have a new CLI argument to allow control of it from outside=D But is can be a separate PR, if you think you don't need this feature for testing

We can add this too. I like the idea. It just wasn't blocking.

@MitchTurner
Copy link
Member Author

I meant: Maybe we want to have a new CLI argument to allow control of it from outside=D But is can be a separate PR, if you think you don't need this feature for testing

#2613

@MitchTurner
Copy link
Member Author

Just went ahead and did #2613 too while waiting.

bin/fuel-core/src/cli/run.rs Outdated Show resolved Hide resolved
tests/tests/gas_price.rs Outdated Show resolved Hide resolved
@@ -85,6 +85,7 @@ pub struct Config {
/// The size of the memory pool in number of `MemoryInstance`s.
pub memory_pool_size: usize,
pub da_gas_price_factor: NonZeroU64,
pub starting_recorded_height: Option<u32>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can work directly with BlockHeight

Comment on lines +40 to +43
type Wrapper<'a>
= FuelStorageUnrecordedBlocks<&'a mut Self>
where
Self: 'a;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious about this. Caused by an update to fmt?

Copy link
Contributor

@rafal-ch rafal-ch left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@MitchTurner MitchTurner enabled auto-merge (squash) January 22, 2025 14:51
@MitchTurner MitchTurner merged commit fbe2f87 into master Jan 22, 2025
30 checks passed
@MitchTurner MitchTurner deleted the set-latest-recorded-height-on-startup branch January 22, 2025 15:13
@MitchTurner MitchTurner mentioned this pull request Jan 22, 2025
MitchTurner added a commit that referenced this pull request Jan 22, 2025
## Version v0.41.1

* fault_proving(compression): include block_id in da compressed block
headers by @rymnc in #2551
* chore: Add myself and Andrea as codeowner for graphql API + related
crates by @netrome in #2570
* fix(integration_tests): remove flake from
produce_block__l1_committed_block_affects_gas_price by @rymnc in
#2566
* bugfix: Improve the `BlockCommitterHttpApi` client to use `url` apis
better by @MitchTurner in
#2599
* Fix version compatibility error by @AurelienFT in
#2608
* Improve error messages for responses from committer by @MitchTurner in
#2609
* Update async processor tests by @rafal-ch in
#2577
* The amount of returned dust coins is limited by factor relative to the
amount of selected big coins by @rafal-ch in
#2610
* fix(da_compression): invalid decompression of utxo id and CoinConfig
fix by @rymnc in #2593
* Use latest gas price to estimate next price for tx pool checks by
@MitchTurner in #2612
* Set Latest Recorded Height on startup by @MitchTurner in
#2603
* Use latest gas price to estimate next block gas price during dry runs
by @MitchTurner in #2615
* Check that fuel-core lib builds correctly without default features by
@rafal-ch in #2594
* Expose indexation status in `NodeInfo` endpoint by @rafal-ch in
#2595


**Full Changelog**:
v0.41.0...v0.41.1
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.

Add cli flag for setting starting DA record block height
3 participants