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

Dry run in the past #2661

Merged
merged 14 commits into from
Feb 10, 2025
Merged

Dry run in the past #2661

merged 14 commits into from
Feb 10, 2025

Conversation

Dentosal
Copy link
Member

@Dentosal Dentosal commented Feb 3, 2025

Closes #2062

Description

Support dry running in past blocks. It's behind the --historical-execution flag.

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

@Dentosal Dentosal self-assigned this Feb 3, 2025
@Dentosal Dentosal marked this pull request as ready for review February 3, 2025 15:11
@Dentosal
Copy link
Member Author

Dentosal commented Feb 6, 2025

One test is failing due to a pthread issue (that I cannot reproduce locally). I'll have to setup a linux env to figure this one out, unless someone already knows what's the issue.

@netrome
Copy link
Contributor

netrome commented Feb 6, 2025

One test is failing due to a pthread issue (that I cannot reproduce locally). I'll have to setup a linux env to figure this one out, unless someone already knows what's the issue.

Let me see if I can reproduce it on one of my Linux machines.

@netrome
Copy link
Contributor

netrome commented Feb 6, 2025

One test is failing due to a pthread issue (that I cannot reproduce locally). I'll have to setup a linux env to figure this one out, unless someone already knows what's the issue.

Which test? All I can see is fuel-core-e2e-client::integration_tests works_in_multinode_local_env which I think is flaky.

@Dentosal
Copy link
Member Author

Dentosal commented Feb 7, 2025

One test is failing due to a pthread issue (that I cannot reproduce locally). I'll have to setup a linux env to figure this one out, unless someone already knows what's the issue.

Which test? All I can see is fuel-core-e2e-client::integration_tests works_in_multinode_local_env which I think is flaky.

dry_run__correct_contract_state_in_past_blocks (https://github.com/FuelLabs/fuel-core/actions/runs/13166313215/job/36747248348?pr=2661)

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.

Looks good to me=) Small things to fix and good to go form my side=D

Comment on lines 311 to 316
if block_height.is_some() && !config.debug {
return Err(anyhow::anyhow!(
"The `blockHeight` parameter requires the `--debug` option"
)
.into());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It still uses --debug and .debug.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 2245cc1

Comment on lines 106 to 108
let allow_historical_execution = config.combined_db_config.database_type
== DbType::RocksDb
&& config.historical_execution;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need allow it on config level for the whole fuel-core, including the GraphQL and other services.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Dentosal Dentosal requested a review from xgreenx February 7, 2025 21:35
@netrome
Copy link
Contributor

netrome commented Feb 10, 2025

dry_run__correct_contract_state_in_past_blocks (https://github.com/FuelLabs/fuel-core/actions/runs/13166313215/job/36747248348?pr=2661)

Okay I cannot reproduce this locally either. I tried both the latest version, on 52a981a and on 07013c7.

I'll proceed with reviewing and assume this was caused cosmic background radiation.

Copy link
Contributor

@netrome netrome left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

}

#[tokio::test(flavor = "multi_thread")]
async fn dry_run__correct_utxoid_state_in_past_blocks() -> anyhow::Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: It would be nice to add "Given" "When" "Then" sections to this test.

@@ -760,7 +760,7 @@ type Mutation {
"""
Execute a dry-run of multiple transactions using a fork of current state, no changes are committed.
"""
dryRun(txs: [HexString!]!, utxoValidation: Boolean, gasPrice: U64): [DryRunTransactionExecutionStatus!]!
dryRun(txs: [HexString!]!, utxoValidation: Boolean, gasPrice: U64, blockHeight: U32): [DryRunTransactionExecutionStatus!]!
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should consider combining the utxoValidation, gasPrice and blockHeight args into a single input struct. This would allow us to add more fields in the future without breaking the API (though currently it's still breaking if we use the new arg in the client until we resolve #2676).

If you think this sounds sensible, we could create a follow-up issue for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

The extra args are already optional, isn't that already good enough? But it's good to consider the forward compatibility. I'm not sure if a struct would help in this case?

@Dentosal Dentosal merged commit ed6aa39 into master Feb 10, 2025
33 checks passed
@Dentosal Dentosal deleted the dento/2062_dry_run_in_the_past branch February 10, 2025 09:59
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.

A couple of post-merge questions and comments.

tests/test-helpers/src/counter_contract.rs Show resolved Hide resolved
tests/tests/state_rewind.rs Show resolved Hide resolved
Dentosal added a commit that referenced this pull request Feb 10, 2025
Dentosal added a commit that referenced this pull request Feb 10, 2025
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.

Support dry running of transactions in the past
4 participants