reth: 0.2.0-beta.6 -> 1.0.1

[scottbot95]

Reth recently had their 1.0, production-read release. This PR updates reth to the latest 1.0.1 release.

Considerations

I had to make several potentially controversial changes in order to get reth to build. Specifically two new flake inputs where added, rust-overlay and crane

rust-overlay

Reth requires Rust 1.79.0, this required pulling in the rust-overlay flake as even nixpkgs-unstable only has 1.78.0.

crane

The nixpkgs native rustPlatform.buildRustPackage is fairly rudimentary and doesn't play nicely with Rusts incremental compilation (it will re-download and rebuild your ENTIRE dependency closure every time the nix derivation is built). This is partially Rust's fault (since a package can change how it's dependencies are compiled), however the crane flake was designed to help improve this situation and reduce re-compile times. Crane also turned out to be required for testing as described below.

Testing

Reth has an extensive testing suite. These tests don't require nextest, however we only want to run the unit tests which is difficult to do with cargo test (would require a significant number of --skip args), but can easily be accomplished by providing -E '!kind(test)' to the nextest invocation. Additionally some "unit" tests require I/O access that is not compatible with the nix sandbox requiring us to skip several specific tests. In nextest this looks like -E '!kind(test) - test(tests::test_exex)'.

Unfortunately, when passing arguments via rustPlatform.buildRustPackage, nix tries to be "help" and will shell escape all arguments passed in. This completely breaks the -E args because it contains special characters that need to be included as-is to the actual call to cargo nextest run. There would be a workaround for this, simply have preCheck script that directly modifies cargoTestFlags, however unfortunately there is a bug in nixpkgs here where despite cargoTestFlags being an array, only the first element of that array is included in the final call (${cargoTestFlags} instead of the correct ${cargoTestFlags[@}}.

I have opted to use nextest since that is the officially supported testing platform with Reth and also executes >x3 faster than cargo test. This forced me to use crane due to the inability to properly pass in args to the cargo nextest run call with rustPlatform.buildRustPackage

[scottbot95]

There has been >1000 commits since 0.2.0-beta.6. It is entirely possible this is no longer required, however I have no way to test x86_64-darwin.

[scottbot95]

If we don't like the the addition of the crane dependency, I think once NixOS/nixpkgs#326365 is merged I should be able to refactor this to use buildRustPackage instead.

[aldoborrero]

@scottbot95 I would prefer to wait instead to use buildRustPackage if possible rather than adding more external inputs (and it seems it's going to be merged soon).

[scottbot95]

Sure that sounds reasonable to me. I will update once my change has been merged to nixpkgs.

That will also require I update the nixpkgs-unstable input. Would it be preferrable to update that as a separate PR or just include it in this one?

[aldoborrero]

That will also require I update the nixpkgs-unstable input. Would it be preferrable to update that as a separate PR or just include it in this one?

@scottbot95 just include it on this one as it's tied to this specific work.

[aldoborrero]

@scottbot95 given the upstream PR for nixpkgs is taking more time than expected to be merged. Let's proceed and merge this work into master. We can refactor it on a separate PR and remove the extra inputs.

[scottbot95]

Unfortunate, but that sounds reasonable to me. FWIW I have a local branch where I'm targeting my fork of nixpkgs and I have confirmed that it builds cleanly using buildRustPackage after my changes to nixpkgs

[aldoborrero]

@scottbot95 is there anything else to account for or can I proceed with merging the PR?

[scottbot95]

@aldoborrero I successfully used this commit to sync a holesky node so I think we should be good unless you care about bottoming out whatever the darwin issues are. Those might just go away when we switch back to buildRustPackage though. I don't have any darwin machines so it's rather difficult for me to test.

[aldoborrero]

@scottbot95 would you mind fixing the conflict?

[scottbot95]

Rebased against main

[aldoborrero]

@scottbot95 thanks! Have a look at the following error.

[scottbot95]

Sorry for taking a while to look at this. I think the issue is the rustPlatform.bindgenHook needs to be in commonArgs. I have pushed a potential fix but don't have a darwin machine so I have no easy to way test it locally.

[scottbot95]

Hmm still failing... I think this will require someone who actually understands darwin at all to resolve.

[aldoborrero]

@brianmcgee would you be kind to test it on your Mac?

[jhvst]

Can we consider dropping darwin support for now for reth? If someone actually needs it, maybe they can also figure out what is wrong with the build command.

[brianmcgee]

Can we consider dropping darwin support for now for reth? If someone actually needs it, maybe they can also figure out what is wrong with the build command.

I think this makes sense, @aldoborrero @selfuryon ?

[aldoborrero]

It does. Let's proceed without darwin support.