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

no_std support for fuel-tx and fuel-vm #582

Merged
merged 49 commits into from
Sep 22, 2023
Merged

no_std support for fuel-tx and fuel-vm #582

merged 49 commits into from
Sep 22, 2023

Conversation

Dentosal
Copy link
Member

@Dentosal Dentosal commented Sep 7, 2023

Makes fuel-vm crate compile without stdlib, but still requireing alloc. This is a breaking change, and the corresponding updates to fuel-core are done in FuelLabs/fuel-core#1367. Most of the breakage comes from removing std::io::Error, especially from storage traits which now use concrete error types instead and only allow std::io::Error on some variants available with std feature enabled.

Currently passes with the following command for RiscV:

cargo build --target riscv32imac-unknown-none-elf -p fuel-vm --no-default-features --features alloc

and check passes for freestanding x86-64, but building doesn't, since it requires soft-float configuration:

cargo check --target x86_64-unknown-none -p fuel-vm --no-default-features --features alloc

Closes #230 #536

@Dentosal Dentosal added breaking A breaking api change no changelog Skips the CI changelog check labels Sep 7, 2023
@Dentosal Dentosal self-assigned this Sep 7, 2023
@Dentosal Dentosal mentioned this pull request Sep 11, 2023
@Dentosal Dentosal added fuel-vm Related to the `fuel-vm` crate. and removed no changelog Skips the CI changelog check labels Sep 13, 2023
fuel-crypto/src/secp256/backend/r1/p256.rs Show resolved Hide resolved
fuel-merkle/src/binary/merkle_tree.rs Outdated Show resolved Hide resolved
fuel-merkle/src/common/storage_map.rs Outdated Show resolved Hide resolved
fuel-merkle/src/storage.rs Outdated Show resolved Hide resolved
}

#[cfg(feature = "std")]
impl From<StorageError> for std::io::Error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there still instances where we use std::io::Error? Or can we get rid of this conversion now?

fuel-vm/src/arith.rs Show resolved Hide resolved
@Dentosal
Copy link
Member Author

Dentosal commented Sep 20, 2023

Migrating to a single concrete StorageError type seems like an disliked approach, and while I still like it more myself, I can see that it has downsides.

I've reversed the direction on this, and instead parametrized InterpreterError with a generic parameter carrying the storage error type, so that we don't have to use a custom error trait. Now the resulting error type is directly available to the users of fuel-vm, and isn't hidden behind std::io::Error. This means minor changes on fuel-core side, but it shouldn't be too hard, especially since we only used type-erased errors before.

Let me know your opinions on the new approach.

fuel-vm/src/interpreter/blockchain.rs Outdated Show resolved Hide resolved
Comment on lines +11 to 12
#[cfg(feature = "std")]
use thiserror::Error;
Copy link
Collaborator

@xgreenx xgreenx Sep 21, 2023

Choose a reason for hiding this comment

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

I would prefer to remove usage of thiserror and replace it with similar Display implementation. Maybe we can use another macro(no_std compatible) to derive exact the same implementation of the Display trait.

fuel-vm/src/interpreter/contract.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bvrooman bvrooman left a comment

Choose a reason for hiding this comment

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

LGTM. Personally, I like the approach to parameterize the InterpreterError because it allows us to maintain separation between discrete levels of logic.

re: thiserror - If we want to remove thiserror in favour of writing custom Display traits, I think that we can do that in a separate PR.

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.

It would be nice to have it in this PR because it can remove some std features from some crates, but I'm also okay with doing that in a separate PR=)

@@ -95,7 +97,7 @@ jobs:
- uses: dtolnay/rust-toolchain@master
with:
toolchain: ${{ env.RUST_VERSION }}
targets: "thumbv6m-none-eabi,wasm32-unknown-unknown"
targets: "thumbv6m-none-eabi,riscv32imac-unknown-none-elf,wasm32-unknown-unknown"
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 - did you chose the IMAC extensions for any particular reason, or is that just a safe default?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's closes to the internal target used by risc-zero

@Dentosal
Copy link
Member Author

I'm leaving the thiserror -> display impl change for a follow-up PR, as I feel like it's nicer to review separately.

@Dentosal Dentosal added this pull request to the merge queue Sep 22, 2023
Merged via the queue into master with commit 76eb3e0 Sep 22, 2023
37 checks passed
@Dentosal Dentosal deleted the dento/nostd branch September 22, 2023 02:49
This was referenced Sep 27, 2023
Dentosal added a commit to FuelLabs/fuel-core that referenced this pull request Sep 27, 2023
Addresses breaking fuel-vm changes from
FuelLabs/fuel-vm#582,
FuelLabs/fuel-vm#578,
FuelLabs/fuel-vm#588 and
FuelLabs/fuel-vm#587.

Waiting for a new fuel-vm release.

---------

Co-authored-by: Green Baneling <XgreenX9999@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A breaking api change fuel-storage Related to the `fuel-storage` crate. fuel-tx Related to the `fuel-tx` crate. fuel-vm Related to the `fuel-vm` crate.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error propagation
3 participants