Rust protobuf API #525

benma · 2020-07-02T09:30:10Z

This is a working demo of how to use Prost to handle our protobuf api in an async manner.

I am using the prost deps from github as the no_std fixes are not released yet, see https://github.com/danburkert/prost/issues/329. We can discuss if we should move ahead anyway or wait for the release.

bitbox02_rust::hww::api::process corresponds to the C commander().

Still missing: commander_states_can_call() check from commander() - I didn't add it yet to keep the code smaller for review, will add as soon as the concept is approved.

benma · 2020-07-02T09:30:48Z

I know, you don't need to tell me that there are too many vendored files that we don't need :( No idea how to improve that.

Edit: this might be the culprit:

tokio-rs/bytes#400 (comment)

x1ddos · 2020-07-02T11:14:14Z

I know, you don't need to tell me that there are too many vendored files that we don't need :( No idea how to improve that.

Edit: this might be the culprit:

tokio-rs/bytes#400 (comment)

Did you try with a cargo +nightly vendor -Z avoid-dev-deps?

x1ddos

Very cool! Actually, amazing. A couple items to discuss:

would it make sense work on this in a separate "prost" branch?
I'd personally prefer to wait for prost release; we're not in a hurry, are we
try cargo +nightly vendor -Z avoid-dev-deps or something like that

This is a working demo

It's not really a demo anymore. It does process a valid API call, even though it's just one for now.

src/CMakeLists.txt

tools/prost-build/.gitignore

tools/prost-build/.cargo/config

x1ddos · 2020-07-02T11:25:41Z

tools/prost-build/src/main.rs

+// limitations under the License.
+
+fn main() {
+    let messages_dir = std::env::var("MESSAGES_DIR").expect("MESSAGES_DIR env var not set");


Why not use a command line flag?

My issue with env vars is I see this:

MESSAGES_DIR=${CMAKE_CURRENT_SOURCE_DIR} cargo run --manifest-path=${CMAKE_SOURCE_DIR}/tools/prost-build/Cargo.toml

and I have no idea whether it was a mistake, typo or MESSAGES_DIR is really used in the next command.

idk, seemed consistent with OUT_DIR imposed by prost-build. And no CLI parsing needed ;)

I will look into it.

yeah, that's even more confusing imho:

prost_build::compile_protos(&["hww.proto"], &[&messages_dir]).unwrap();

So, compile_protos takes in what to compile in function arguments, but somehow it wants an env var for the output... that looks very inconsistent, wouldn't you agree?

i think OUT_DIR is imposed by the rust build.rs architecture 🤷

I'm just saying that the reason that prost-build uses OUT_DIR is because cargo sets that environment variable when it runs a build.rs file. You should, on the other hand, use command line arguments to pass input files and output dirs.

I think build.rs files are useful for rust-only projects, but they are kind of evil in that they can do arbitrary things at compile time (like fetching stuff from ze internets). So I try to avoid them. In your case it is better to create a separate tool (like you've done) and run it separately from cargo. This reduces the number of deps on your main project and (when you move it to docker) reduces the overall build time.

I'm split regarding if the generated sources should be committed. One benefit of committing them is that you notice if you by mistake use a tool that generates new/different output. (Maybe you locally have the wrong version installed for some reason..)

Like @benma, I'd prefer to check in generated sources. We already do it in py/bitbox02 here and in api-go, api-js. Also, similar with vendoring Go pkgs in the wallet app. It's always nice to rely on git and see what's changed.

@x1ddos done

hm.. I still see no args parsing in main.rs. @benma am I looking at the old code or wrong place?

Hmm somehow I didn't push properly. Pushed again now.

tools/prost-build/src/main.rs

src/rust/bitbox02-rust/src/hww/api/api.rs

benma · 2020-07-02T15:53:13Z

Did you try with a cargo +nightly vendor -Z avoid-dev-deps?

it's not a dev dependency though.

x1ddos · 2020-07-02T15:55:32Z

Did you try with a cargo +nightly vendor -Z avoid-dev-deps?

it's not a dev dependency though.

Ah right. Didn't pay enough attention.
What if we temporary fork tokio-rs/bytes and move loom to dev dependencies?

benma · 2020-07-02T16:27:08Z

fork

That helped. 3755 files down to 185 👏

Fork here: https://github.com/digitalbitbox/bytes/tree/bitbox02-firmware-20200702

x1ddos · 2020-07-02T19:32:03Z

Almost forgot: add clippy to pass over tools/prost-build/ or tools/...? I know it's just 2 lines for code but maybe we'll have more with time.

NickeZ · 2020-07-02T20:40:10Z

To reiterate what I said in a thread. I think it makes sense to compile the tool when you create the docker image instead of every time you compile the sources. So I think the tool should take "OUT_DIR" and "MESSAGES_DIR" as the two input arguments. You don't need especially sophisticated arg parsing:

fn main() {
    let mut args = std::env::args().into_iter();
    let _ = args.next().unwrap(); // executable name
    let out_dir = args.next().unwrap();
    let messages_dir = args.next().unwrap();
    println!("{} {}", out_dir, messages_dir);
}

NickeZ · 2020-07-06T11:24:01Z

I think clap takes a long time to compile (minutes), so I suggest to not use clap in case you don't need all the bells and whistles. You should also pass --release to cargo run to probably increase execution speed some orders of magnitude.

benma · 2020-07-06T12:50:07Z

I think clap takes a long time to compile (minutes),

Hmm I just recompiled and almost didn't notice the compilation, was very quick, definitely nowhere near a minute.

so I suggest to not use clap in case you don't need all the bells and whistles.

But I really much prefer named args over positional args, and the code is much nicer too :O

--release: can add, though execution speed here does not matter much, feels like nanoseconds anyway.

Edit: tried with --release, actually that seems to slow it down some.

NickeZ · 2020-07-06T13:46:28Z

Edit: tried with --release, actually that seems to slow it down some.

WAT.. haha

edit: or maybe it slowed down the overall thing because it added more time to the compilation of the tool than it removed from the run time?

benma · 2020-07-06T13:55:59Z

Edit: tried with --release, actually that seems to slow it down some.

WAT.. haha

edit: or maybe it slowed down the overall thing because it added more time to the compilation of the tool than it removed from the run time?

Yeah I meant the compile time, in response to

I think clap takes a long time to compile (minutes),

Run time is probably better, but compiling those handful of proto files is fast no matter what.

x1ddos · 2020-07-06T14:02:55Z

I think clap takes a long time to compile (minutes)

@NickeZ didn't you also suggest to add the compiled tool to the docker image? :)

Also, @benma @NickeZ is clap like a top package cool kids use to parse cmd line args in Rust these days? Why is it taking long to compile anyway? Are there alternatives?

x1ddos · 2020-07-06T14:04:57Z

Re: clap. At the end of the day, how often are we going to regenerate proto sources. I think quite rarely if they're checked in.

x1ddos

Cool! One other very minor thing:

Almost forgot: add clippy to pass over tools/prost-build/ or tools/...? I know it's just 2 lines for code but maybe we'll have more with time.

x1ddos · 2020-07-07T10:00:17Z

tools/prost-build/Cargo.toml

+license = "Apache-2.0"
+
+[dependencies]
+clap = "3.0.0-beta.1"


Why not the latest non-beta release 2.33?

Why non-beta? beta is working.

stability? otherwise, why call things "beta".
git master/HEAD is also working. why not always pull from latest git all the deps then :)

api stability is out anyway, as 2.33 -> 3.0.0 is breaking. I expect less work to upgrade to 3.0.0 from the beta than from 2.33. Probably no work, would be a surprise if they changed the api that breaks our 4 line use of it.

If you insist I can do it, but imho it's working okay ;)

I expect less work to upgrade to 3.0.0 from the beta than from 2.33

That's the explanation I expected! Makes sense. Good as is then, considering it's 4 lines like you said. 👍

Well, not expected really but it makes sense either way.

benma · 2020-07-09T10:06:43Z

how often

CI will do it everytime, right, as to the CI, the .proto files are always new. So probably worth it to put it into the Docker image eventually to shorten CI time.

x1ddos · 2020-07-09T11:10:49Z

how often

CI will do it everytime, right, as to the CI, the .proto files are always new. So probably worth it to put it into the Docker image eventually to shorten CI time.

Exactly, so that should make long compile time of clap less annoying.

x1ddos · 2020-07-09T11:12:13Z

So, what's left here, this?

Still missing: commander_states_can_call() check from commander() - I didn't add it yet to keep the code smaller

and clippy to run over tools/...

benma · 2020-07-09T21:49:53Z

and clippy to run over tools/...

I am not sure my cmake skills are up to the task. The only thing I can come up is to make another custom target like rust-clippy-tools, which kinda sucks, but it is in a different WORKING_DIRECTORY. i am inclined to just skip it, seeing that the tool is totally trivial.

x1ddos · 2020-07-10T08:11:16Z

Sounds good without clippy!

…

On 7/9/20 11:50 PM, benma wrote: > and clippy to run over `tools/...` I am not sure my cmake skills are up to the task. The only thing I can come up is to make another custom target like `rust-clippy-tools`, which kinda sucks, but it is in a different `WORKING_DIRECTORY`. i am inclined to just skip it, seeing that the tool is totally trivial.

benma · 2020-08-17T16:09:18Z

Rebased.

We make a small cli tool, used in CMakeLists, similar to nanopb. The generated file lands in the source folder and is committed. I prefer this over generating into the build folder. The advantages are: - Less reliance on the tool to work when you check out and old commit - The rust crates do not depend on CMake in this way, which should make it easier to use standard Rust tooling (though more work is - needed there).

Used in bitbox02-rust to decode and encode protobuf messages.

benma · 2020-08-22T17:35:55Z

So, what's left here, this?

Still missing: commander_states_can_call() check from commander() - I didn't add it yet to keep the code smaller

Added a commit resolving this part too, PTAL.

Imho we shouldn't wait longer for the prost release, and use the GitHub dependency as is in this PR. The release will come when it will come. In the meantime, we shouldn't be blocked from going further with Rust api call processing. WDYT?

x1ddos · 2020-08-24T08:22:09Z

Imho we shouldn't wait longer for the prost release, and use the GitHub dependency as is in this PR. The release will come when it will come. In the meantime, we shouldn't be blocked from going further with Rust api call processing. WDYT?

👍 let me try this on a dev device...

x1ddos

It appears clippy isn't running and a tiny bit unhappy. We should add it to the CI.

x1ddos · 2020-08-24T08:43:50Z

test/unit-test/test_commander_set_device_name.c

-    return mock();
-}
-
-static void _test_api_set_device_name(void** state)


Too bad this commit deletes a unit test without an equivalent replacement, or there one in rust that I don't see?

If not, any chance to export commander_api_set_device_name from rust to use here in the test?

Imho we should start thinking about how to properly unit test Rust code that references C code, and then add some tests for all code. Propably will involve some extra traits, but not quite clear. This has been a blocker to write good unit tests in Rust in general. Happy to maximize coverage then.

There is no big loss in removing this test until we have that.

x1ddos · 2020-08-24T08:45:27Z

src/rust/bitbox02-rust/src/hww/api/api.rs

+/// `input` is a hww.proto Request message, protobuf encoded.
+/// Returns a protobuf encoded hww.proto Response message.
+pub async fn process(input: Vec<u8>) -> Vec<u8> {
+    use prost::Message;


I thought we agreed to use global imports with an exception of enums where makes sense, or did I misunderstood?

Yeah this line preceeds this discussion.

I plan to split the individual api calls into individual files after this is merged, so I would move the use to the top level then, ok?

Actually I thought we were in the device name api function. Fixed it for prost::Message, which will stay in this file even when I move the api calls to other files.

x1ddos · 2020-08-24T08:54:10Z

test/unit-test/test_commander_set_device_name.c

-    // All A-Okay.
-    will_return(__wrap_workflow_confirm_blocking, true);
-    will_return(__wrap_memory_set_device_name, true);
-    assert_int_equal(COMMANDER_OK, commander_api_set_device_name(&request));


The func declaration is still in test/unit-test/framework/includes/test_commander.h. That should be removed too, right.

Good catch, fixed!

benma · 2020-08-24T21:24:43Z

It appears clippy isn't running and a tiny bit unhappy. We should add it to the CI.

fixed and added to CI. PTAL

Moving the commander states check calls from C to Rust makes sure that API calls handled in Rust obey the same rules, e.g. that SetDeviceName cannot be called in the middle of a Bitcoin tx signing session.

api::api is bad (clippy), so we move it to mod.rs.

x1ddos · 2020-08-25T05:55:30Z

LGTM. Hope to see more tests soon!

benma requested a review from x1ddos July 2, 2020 09:30

benma force-pushed the rust-prost-2 branch from accbfb0 to 522bf12 Compare July 2, 2020 09:42

x1ddos reviewed Jul 2, 2020

View reviewed changes

benma force-pushed the rust-prost-2 branch from 522bf12 to 29cd960 Compare July 2, 2020 16:26

benma force-pushed the rust-prost-2 branch 2 times, most recently from 334f641 to 3b2c083 Compare July 2, 2020 16:32

x1ddos changed the title ~~Rust protobuf api~~ Rust protobuf API Jul 3, 2020

benma force-pushed the rust-prost-2 branch from 3b2c083 to 07085c9 Compare July 6, 2020 07:51

x1ddos reviewed Jul 7, 2020

View reviewed changes

benma force-pushed the rust-prost-2 branch from 07085c9 to 2265d08 Compare August 17, 2020 16:03

rust: vendor prost

b581845

Used in bitbox02-rust to decode and encode protobuf messages.

benma force-pushed the rust-prost-2 branch from 2265d08 to 99374bb Compare August 22, 2020 17:34

benma force-pushed the rust-prost-2 branch 3 times, most recently from 5907bd5 to 523c79c Compare August 22, 2020 22:03

x1ddos reviewed Aug 24, 2020

View reviewed changes

benma force-pushed the rust-prost-2 branch from 42598fe to 8c15dc3 Compare August 24, 2020 21:26

benma added 4 commits August 25, 2020 00:06

rust/hww: protobuf plumbing and set device name call in Rust

084068a

rust/hww: disallow api calls according to commander_states

41f22ff

Moving the commander states check calls from C to Rust makes sure that API calls handled in Rust obey the same rules, e.g. that SetDeviceName cannot be called in the middle of a Bitcoin tx signing session.

rust/hww: fix module-inception

2c08ada

api::api is bad (clippy), so we move it to mod.rs.

rust: fix remaning clippy issues and add clippy to CI

7b3f5f1

benma force-pushed the rust-prost-2 branch from 8c15dc3 to 7b3f5f1 Compare August 24, 2020 22:07

x1ddos approved these changes Aug 25, 2020

View reviewed changes

benma merged commit e9802c2 into BitBoxSwiss:master Aug 25, 2020

benma deleted the rust-prost-2 branch August 25, 2020 08:57

Rust protobuf API #525

Rust protobuf API #525

Conversation

benma commented Jul 2, 2020

benma commented Jul 2, 2020 • edited Loading

x1ddos commented Jul 2, 2020

x1ddos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

x1ddos Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma commented Jul 2, 2020

x1ddos commented Jul 2, 2020 • edited Loading

benma commented Jul 2, 2020

x1ddos commented Jul 2, 2020

NickeZ commented Jul 2, 2020

NickeZ commented Jul 6, 2020

benma commented Jul 6, 2020 • edited Loading

NickeZ commented Jul 6, 2020 • edited Loading

benma commented Jul 6, 2020

x1ddos commented Jul 6, 2020 • edited Loading

x1ddos commented Jul 6, 2020 • edited Loading

x1ddos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma commented Jul 9, 2020

x1ddos commented Jul 9, 2020

x1ddos commented Jul 9, 2020

benma commented Jul 9, 2020

x1ddos commented Jul 10, 2020 via email

benma commented Aug 17, 2020

benma commented Aug 22, 2020

x1ddos commented Aug 24, 2020

x1ddos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma commented Aug 24, 2020

x1ddos commented Aug 25, 2020

benma commented Jul 2, 2020 •

edited

Loading

x1ddos Jul 6, 2020 •

edited

Loading

x1ddos commented Jul 2, 2020 •

edited

Loading

benma commented Jul 6, 2020 •

edited

Loading

NickeZ commented Jul 6, 2020 •

edited

Loading

x1ddos commented Jul 6, 2020 •

edited

Loading

x1ddos commented Jul 6, 2020 •

edited

Loading

benma Jul 7, 2020 •

edited

Loading

benma Aug 24, 2020 •

edited

Loading