Skip to content

chore: fix serde std flags for no-std build #987

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.

Sign up for GitHub

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

Merged
rakita merged 7 commits into from
Jan 18, 2024
Merged

chore: fix serde std flags for no-std build #987

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cbbfe9c
use serde as no-std
Nov 24, 2023
15c51fa
optional serde std
CeciliaZ030 Jan 18, 2024
6f95113
delete comment #"std"
CeciliaZ030 Jan 18, 2024
cf4da2f
"preserve_order"
CeciliaZ030 Jan 18, 2024
9a03c7e
Merge branch 'main' into serde-no-std
CeciliaZ030 Jan 18, 2024
2f0f19f
revme revert
CeciliaZ030 Jan 18, 2024
53d6e8b
Update bins/revme/Cargo.toml
rakita Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions bins/revme/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,20 @@ indicatif = "0.17"
plain_hasher = "0.2"
revm = { path = "../../crates/revm", version = "3.5.0", default-features = false, features = [
"ethersdb",
"std",
"serde",
"c-kzg",
Copy link
Copy Markdown
Member

@rakita rakita Jan 18, 2024

Choose a reason for hiding this comment

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

c-kzg is hard to build in no_std environment, it is c lib. But c-kzg is needed for the test so it needs to be included.

I am fine with making revme std only as it is binary and more like a tool/utility.

Copy link
Copy Markdown

@mratsim mratsim Jan 18, 2024

Choose a reason for hiding this comment

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

Maybe it can be a test only / bench only dependencies?

Copy link
Copy Markdown
Contributor Author

@CeciliaZ030 CeciliaZ030 Jan 18, 2024

Choose a reason for hiding this comment

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

I looked again, and it does make sense to keep this on std cuz it's a command-line app. I'm gonna revert changes for revme

] }
alloy-rlp = { version = "0.3", default-features = false, features = [
"arrayvec",
"derive",
] }
serde = { version = "1.0", features = ["derive", "rc"] }
serde_json = { version = "1.0", features = ["preserve_order"] }
serde = { version = "1.0", default-features = false, features = ["derive", "rc"] }
serde_json = { version = "1.0", default-features = false, features = ["alloc"]}
Copy link
Copy Markdown
Member

@rakita rakita Jan 18, 2024

Choose a reason for hiding this comment

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

preserve_order was a nice feature, does it work with no_std?

Copy link
Copy Markdown
Contributor Author

@CeciliaZ030 CeciliaZ030 Jan 18, 2024

Choose a reason for hiding this comment

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

it works, sry i added back at one place for miss the other :)

Copy link
Copy Markdown

@leonardoalt leonardoalt Jan 18, 2024

Choose a reason for hiding this comment

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

I think preserve_order doesn't really work with no-std because of https://github.com/serde-rs/json/blob/master/Cargo.toml#L58 . That's why I had to remove it in cbbfe9c in order to make revme no-std. But that should be fine in this repo because as pointed out it's only required by revme which is a separate tool, also I don't think it would be possible to fully make revme no-std because of c-kzg.

Copy link
Copy Markdown
Member

@rakita rakita Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

They have PR for it: serde-rs/json#1101 still not merged
Will answer about its usefulness here #987 (comment)

structopt = "0.3"
thiserror = "1.0"
triehash = "0.8"
walkdir = "2.4"

[features]
default = ["std"]
std = ["serde/std", "serde_json/std", "alloy-rlp/std", "revm/std"]
4 changes: 2 additions & 2 deletions crates/interpreter/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ rustdoc-args = ["--cfg", "docsrs"]
revm-primitives = { path = "../primitives", version = "1.3.0", default-features = false }

# optional
serde = { version = "1.0", features = ["derive", "rc"], optional = true }
serde = { version = "1.0", default-features = false, features = ["derive", "rc"], optional = true }

[features]
default = ["std"]
std = ["revm-primitives/std"]
std = ["serde?/std", "revm-primitives/std"]
serde = ["dep:serde", "revm-primitives/serde"]
arbitrary = ["std", "revm-primitives/arbitrary"]
asm-keccak = ["revm-primitives/asm-keccak"]
Expand Down
5 changes: 3 additions & 2 deletions crates/primitives/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,15 @@ enumn = "0.1"
derive_more = { version = "0.99", optional = true }

# optional
serde = { version = "1.0", features = ["derive", "rc"], optional = true }
serde = { version = "1.0", default-features = false, features = ["derive", "rc"], optional = true }

[build-dependencies]
hex = "0.4"
hex = { version = "0.4", default-features = false }

[features]
default = ["std", "c-kzg"]
std = [
"serde?/std",
"alloy-primitives/std",
"hex/std",
"bitvec/std",
Expand Down
6 changes: 3 additions & 3 deletions crates/revm/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ revm-precompile = { path = "../precompile", version = "2.2.0", default-features
auto_impl = { version = "1.1", default-features = false }

# Optional
serde = { version = "1.0", features = ["derive", "rc"], optional = true }
serde_json = { version = "1.0", features = ["preserve_order"], optional = true }
serde = { version = "1.0", default-features = false, features = ["derive", "rc"], optional = true }
serde_json = { version = "1.0", default-features = false, features = ["alloc", "preserve_order"], optional = true }
Copy link
Copy Markdown

@leonardoalt leonardoalt Jan 19, 2024

Choose a reason for hiding this comment

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

@rakita how critical is having preserve_order here?

Copy link
Copy Markdown
Member

@rakita rakita Jan 19, 2024

Choose a reason for hiding this comment

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

It seems not at all critical, I thought it is there to format output, but after checking, this is not the case. I am okay with removing it.

Copy link
Copy Markdown

@leonardoalt leonardoalt Jan 19, 2024

Choose a reason for hiding this comment

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

Nice, thanks!

Copy link
Copy Markdown
Member

@rakita rakita Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Just curious, why would you need binary to be no_std compatible?

Copy link
Copy Markdown

@leonardoalt leonardoalt Jan 19, 2024

Choose a reason for hiding this comment

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

My need (and in this case @CeciliaZ030 's as well) is not the binary per se, but rather revm+serde. We want no-std revm to make ZK proofs for it with powdr , which works fine. However it would be great to also be able to enable the serde feature in revm, in order to have better UX in message passing between the host and the guest when making ZK proofs.

Copy link
Copy Markdown
Member

@rakita rakita Jan 19, 2024

Choose a reason for hiding this comment

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

Makes sense, I don't focus a lot of my attention on revme (binary) as it is used only for testing, was curious to see if I need to revise that view.

Okay, I just noticed that preserve_order is in both revm and revme. preserved_order is nice in revm as it formats trace json output that is easier to read, still okay with removing it.

Can you make a PR?

Copy link
Copy Markdown

@leonardoalt leonardoalt Jan 19, 2024

Choose a reason for hiding this comment

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

Sure! We can also wait to see if serde-rs/json#1101 gets merged, then we don't need to remove it.

Copy link
Copy Markdown
Member

@rakita rakita Jan 19, 2024

Choose a reason for hiding this comment

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

Your call, if this blocks you or you can use this right now it would be best to make PR


# ethersdb
tokio = { version = "1.35", features = [
Expand All @@ -42,7 +42,7 @@ indicatif = "0.17"

[features]
default = ["std", "c-kzg", "secp256k1"]
std = ["revm-interpreter/std", "revm-precompile/std"]
std = ["serde?/std", "serde_json?/std", "revm-interpreter/std", "revm-precompile/std"]
serde = ["dep:serde", "dep:serde_json", "revm-interpreter/serde"]
arbitrary = ["revm-interpreter/arbitrary"]
asm-keccak = ["revm-interpreter/asm-keccak", "revm-precompile/asm-keccak"]
Expand Down