Page MenuHomePhabricator

[Chronik] Add minimal bitcoinsuite-core Rust crate + CMake to `cargo test` it
ClosedPublic

Authored by tobias_ruck on Jun 24 2022, 21:00.

Details

Summary
  • Add empty bitcoinsuite-core crate. We keep it empty as we don't have any lints yet, which will be added next.
  • Add CMake files to enable Chronik via BUILD_BITCOIN_CHRONIK CMake flag
  • Find Rust via Corrosion
  • Add check-bitcoinsuite target to run cargo test on all the rust crates starting with bitcoinsuite-.
Test Plan
  1. Install Rust 1.61 and Corrosion 0.2, see D11663
  2. cmake .. -GNinja -DBUILD_BITCOIN_CHRONIK=on
  3. ninja check-bitcoinsuite
  4. Optionally (will be automated next): in src/rust: None of cargo check, cargo doc and cargo clippy raise warnings

Diff Detail

Event Timeline

tobias_ruck retitled this revision from Add minimal bitcoinsuite-core Rust crate + CMake to `cargo test` it to [Chronik] Add minimal bitcoinsuite-core Rust crate + CMake to `cargo test` it.Jun 25 2022, 16:10
Fabien requested changes to this revision.Jun 25 2022, 16:36

There are several copyright headers missing.

What test is the target running ? None for the moment ?

Also the linter should be added to our linters running from arc lint if possible.

src/CMakeLists.txt
686 ↗(On Diff #34138)

I don't expect to build rust. You probably want to call this director chronik instead. That also removes the need for the above comment

src/rust/.gitignore
2 ↗(On Diff #34138)

We usually use out of tree build, is this possible ? So there is no file generated in the source tree

src/rust/CMakeLists.txt
1 ↗(On Diff #34138)

Copyright is missing

3 ↗(On Diff #34138)

Are we checking rust ?

8 ↗(On Diff #34138)

Same question, why forcing in tree build ?

This revision now requires changes to proceed.Jun 25 2022, 16:36

There are several copyright headers missing.

Will fix, my bad.

What test is the target running ? None for the moment ?

Yes, it just runs the test suite, which finds no tests and succeeds. I wanted to keep things small and simple.

Also the linter should be added to our linters running from arc lint if possible.

There's some question marks on how to lint exactly, especially with Phabricator integration, I'd prefer to do it in a later diff?

I don't expect to build rust. You probably want to call this director chronik instead. That also removes the need for the above comment

I thought this too yet went for rust, but this might be a bigger discussion. Several things:

  1. What if we add non-Chronik Rust code, like adding our Rust BIP70 server (@Mengerian showed interest in this), or SLP Post Office, or a SWaP server? Then we could have this (idiomatic) folder structure in src/rust, each being one Rust crate:
    1. bip70-server
    2. bitcoinsuite-core
    3. bitcoinsuite-slp
    4. ...
    5. chronik-http
    6. chronik-indexer
    7. chronik-rocksdb
    8. ...
    9. chronik-rocksdb
    10. post-office-pool
    11. post-office-server
    12. swap-server
    13. ...
  2. I expect this folder to evolve over time into a more and more sophisticated library, which might add crates for tx construction, signing etc. at some point, basically a set of general-purpose Rust crates. Cargo makes it very easy to reuse these, especially if we extract the src/rust folder automatically to GitHub like we do for secp256k1, then people can reference any crate in their dependencies, like via bitcoinsuite-core = { git = "https://github.com/Bitcoin-ABC/ecash-rust" } (see here). In that case, it makes more sense to just call the folder rust and keep all things written in Rust in there.
  3. Cargo has one workspace file (called Cargo.toml) and one lockfile (called Cargo.lock). The latter makes it such that all crates have a consistent set of (outside) dependencies, so there should be only one workspace and lockfile. We could have src/chronik, src/bitcoinsuite, src/post-office etc., but then where to put the workspace & lockfiles?
  4. corrosion_import_crate seems to work best if there's just one workspace file, and I'd like to maintain only one bridge to bitcoind. If there's just one rust folder, all the future stuff we could add could depend on that one bridge.

What is unclear to me is how to do the build flag. Say we have BUILD_BITCOIN_CHRONIK and BUILD_BITCOIN_BIP70_SERVER. Should there also be a BUILD_BITCOIN_RUST?. Both would always imply that. I think it will be that there will be some kind of this down the line:

set(BUILD_BITCOIN_RUST, BUILD_BITCOIN_CHRONIK || BUILD_BITCOIN_BIP70_SERVER) (don't know how to do logical OR in CMake, doesn't matter)

with this in src/CMakeLists.txt

if(BUILD_BITCOIN_RUST)
	add_subdirectory(rust)
endif()

and then in src/rust/CMakeLists.txt we configure Chronik and the BIP70 server depending on the states of BUILD_BITCOIN_CHRONIK and BUILD_BITCOIN_BIP70_SERVER.

We usually use out of tree build, is this possible ? So there is no file generated in the source tree

Yes, good catch. Will fix. There's one thing though: Using my IDE, it generates some artefacts into src/rust/target. I think it would be nice to add an ignore for that for convenience so people don't have to do crazy configs just to move their IDE's target into the build folder.

Are we checking rust ?

We're testing everything in the src/rust folder. Similar reasoning as in the directory question—although this could easily be split up by project (check-chronik, check-bitcoinsuite, check-bip70-server, check-post-office) etc. I think that would be a good idea.

I'll keep the discussion on the directory name for another time as I need to learn about the tools first.

Yes, it just runs the test suite, which finds no tests and succeeds. I wanted to keep things small and simple.

OK

There's some question marks on how to lint exactly, especially with Phabricator integration, I'd prefer to do it in a later diff?

Sure, and I can help with that

Yes, good catch. Will fix. There's one thing though: Using my IDE, it generates some artefacts into src/rust/target. I think it would be nice to add an ignore for that for convenience so people don't have to do crazy configs just to move their IDE's target into the build folder.

We don't add code/configs related to IDE in the source tree as they are different for every developer and as such will never be maintained properly. See https://github.com/Bitcoin-ABC/bitcoin-abc/blob/master/doc/developer-notes.md#ignoring-ideeditor-files

We're testing everything in the src/rust folder. Similar reasoning as in the directory question—although this could easily be split up by project (check-chronik, check-bitcoinsuite, check-bip70-server, check-post-office) etc. I think that would be a good idea.

Yes that makes more sense to check by project. You can eventually add it to some magic targets like check or check-all at a later point in time if needed.

Resolve issues raised:

  • Added copyright
  • Moved /src/rust to /chronik, moved workspace file and lockfile to /
  • Made crate empty instead of some useless hash structs
  • Removed #[warn] lints for now
  • Move test artefacts into ${CMAKE_CURRENT_BINARY_DIR}/cargo/build
  • Rename test target to check-bitcoinsuite (will add check-chronik, ... later)
tobias_ruck edited the test plan for this revision. (Show Details)
Fabien requested changes to this revision.Jun 27 2022, 08:44
Fabien added inline comments.
chronik/CMakeLists.txt
5–26 ↗(On Diff #34144)

The whole file can be much simplified, and ninja clean now works as expected

7 ↗(On Diff #34144)

Can you require a min version of rustc and cargo from corrosion ? I tried the target but got this:

The package requires the Cargo feature called `edition2021`, but that feature is not stabilized in this version of Cargo (1.55.0 (32da73ab1 2021-08-23)).
Consider trying a newer version of Cargo (this may require the nightly release).

Updating to 1.61 fixed it but it's better UX to fail early with a nicer error message

13 ↗(On Diff #34144)

You can avoid all this get_property calls by using generator expressions, see below

19 ↗(On Diff #34144)

You need to use double quotes to make it work for paths with space, and add this dir to the clean list so the magic clean target can remove it.

This revision now requires changes to proceed.Jun 27 2022, 08:44
tobias_ruck edited the summary of this revision. (Show Details)
tobias_ruck edited the test plan for this revision. (Show Details)
  • Check for minimum Rust version
  • Simplify CMake following Fabiens diff
  • Removed ALGO_NAME associated const
  • Added copyright
  • Fixed + tested Debug impl

something bizzare happened with my git, adding missing files

This revision is now accepted and ready to land.Jun 27 2022, 12:38
This revision was landed with ongoing or failed builds.Jun 27 2022, 12:43
This revision was automatically updated to reflect the committed changes.