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

Skip empty Script transaction #284

Merged
merged 3 commits into from
Dec 9, 2022
Merged

Conversation

xgreenx
Copy link
Collaborator

@xgreenx xgreenx commented Dec 9, 2022

Fixes FuelLabs/fuel-core#794

It is fast but not an optimal solution to unblock users. With the right solution, we need to skip execution totally without allocating any memory and Interpreter. But it requires some refactoring to support transactions which not affect VM during execution(like empty Script or Create transactions).

@xgreenx xgreenx added the bug Something isn't working label Dec 9, 2022
@xgreenx xgreenx requested a review from a team December 9, 2022 00:55
@xgreenx xgreenx self-assigned this Dec 9, 2022
@Voxelot
Copy link
Member

Voxelot commented Dec 9, 2022

fixes: FuelLabs/fuel-core#794

@ControlCplusControlV
Copy link
Contributor

From a client side as well we should probably filter these out from the mempool as well right? (separate pr and issue ofc)

{
self.run_program()
} else {
Ok(ProgramState::Skipped)
Copy link
Member

Choose a reason for hiding this comment

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

While this is a better variant, I think we should just do ProgramState::Return(1) like create txs did before to avoid extra breaking changes downstream.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, let's introduce ProgramState::Skipped together with an optimal solution.

@xgreenx
Copy link
Collaborator Author

xgreenx commented Dec 9, 2022

Why do we need to filter them?

@Voxelot
Copy link
Member

Voxelot commented Dec 9, 2022

From a client side as well we should probably filter these out from the mempool as well right? (separate pr and issue ofc)

we still want to process the change output for empty script txs. We need to skip the vm's run_program since it expects a real script that ends in either ret or rvrt. However, empty scripts is a use-case we want to support for basic transfer txs.

@mitchmindtree mitchmindtree added the fuel-vm Related to the `fuel-vm` crate. label Dec 9, 2022
@ControlCplusControlV
Copy link
Contributor

Why do we need to filter them?

Is there any reason the node should process empty tx's? Seems like bloat from a larger perspective, and just adds history without doing anything

@xgreenx
Copy link
Collaborator Author

xgreenx commented Dec 9, 2022

Is there any reason the node should process empty tx's? Seems like bloat from a larger perspective, and just adds history without doing anything

It can be useful for transfer like in Bitcoin, you can consume inputs and produce outputs

@Voxelot
Copy link
Member

Voxelot commented Dec 9, 2022

The utxo updates can still make a lot of meaningful change to the blockchain state without any scripts or contracts.

@ControlCplusControlV
Copy link
Contributor

Is there any reason the node should process empty tx's? Seems like bloat from a larger perspective, and just adds history without doing anything

It can be useful for transfer like in Bitcoin, you can consume inputs and produce outputs

Oh nvrm I had thought that everything was a script by default, kept thinking it was analogous to an empty tx

@Voxelot
Copy link
Member

Voxelot commented Dec 9, 2022

It still needs to be a valid script tx type, but only the script field is empty.

@xgreenx xgreenx merged commit 2842da2 into master Dec 9, 2022
@xgreenx xgreenx deleted the bugfix/skip-empty-transaction branch December 9, 2022 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fuel-vm Related to the `fuel-vm` crate.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support script txs with empty script
4 participants