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

Cross-check XDR file hashes between Rust and C++. #3452

Merged
merged 5 commits into from
Sep 3, 2022

Conversation

graydon
Copy link
Contributor

@graydon graydon commented Jun 18, 2022

This adds the necessary machinery to core for cross-checking that the XDR files converted to C++ via xdrc are the same as those converted to rust using xdrgen (and coming in via dependency on rs-stellar-xdr). In other words it is a companion to PR stellar/xdrgen#89

It also includes a fix for some test breakage that occurred when stellar/rs-soroban-env#120 landed on the env crate.

cc @leighmcculloch @jonjove

@leighmcculloch
Copy link
Member

leighmcculloch commented Jun 18, 2022

@graydon graydon force-pushed the check-sha256-of-xdr-files branch 2 times, most recently from 5509566 to b0f1e51 Compare June 21, 2022 06:11
@graydon graydon marked this pull request as ready for review June 21, 2022 07:22
@graydon graydon changed the title First step of cross-checking XDR file hashes between Rust and C++. Cross-check XDR file hashes between Rust and C++. Jun 21, 2022
@MonsieurNicolas
Copy link
Contributor

do we need this to be enforced in code or we could just use github actions for this and catch the problem even earlier?

@MonsieurNicolas
Copy link
Contributor

note: you have a merge commit in there

@graydon
Copy link
Contributor Author

graydon commented Jun 29, 2022

do we need this to be enforced in code or we could just use github actions for this and catch the problem even earlier?

Github is actually a bit late -- the way this is now we will catch it on our own workstations when running make check.

@MonsieurNicolas
Copy link
Contributor

I was commenting on the core binary doing that check. As you're looking at moving the xdr around it looks to be a build problem not a runtime problem. The "hash of xdr" is a small subset of the kind of things that can go wrong if we mix incompatible SDK and core. It seems to me that we need to think about dependencies a bit more.

@graydon graydon mentioned this pull request Jul 5, 2022
39 tasks
@graydon
Copy link
Contributor Author

graydon commented Jul 12, 2022

@MonsieurNicolas I would love to do this at compile time but it's really not obvious how to do it, especially portably (using the very limited build-staging room available in MSBuild rules).

One set of hashes is compiled into rust code and the other C++ code. There's no const-function-evaluation phase in either clang or rustc that has access to user-defined constants in both languages simultaneously -- I tried for a couple days and could not make it work, the cxx.rs bridge system just wasn't designed for it.

So if we wanted it to be a "build time" action we'd need to make the build system have a new phase where it compiles two separate programs -- one rust and one C++ -- each with their own main, each of which print out their compiled-in hashes, and then run those at compile time and compare those. I decided this was not worth the pain at this point.

We also can't just fish it out of version control, because the hashes are at some "distance" from one another: the rust hashes for example are from a secondary repository that's generated by a ruby script applied to another set of input files. There's no question we can ask git about files in the output repo that tell us the hashes of the inputs.

@MonsieurNicolas MonsieurNicolas added this to In progress in v19.4.0 Jul 28, 2022
@graydon
Copy link
Contributor Author

graydon commented Aug 24, 2022

Note: we had another instance of divergence between the rust XDR inputs and C++ XDR inputs today, based on mistaken commits to the wrong repo. I only noticed when something failed to build: stellar/stellar-xdr#21

I appreciate that we need to think about dependencies quite a bit, but "having the same XDR on either side of an FFI" is a baseline part of any correct solution.

@MonsieurNicolas
Copy link
Contributor

ok let's merge this then. I was hoping that by now we'd be in a better spot wrt dependencies but whatever helps.

you need to rebase this though.

Also, I made a comment some time ago > "note: you have a merge commit in there".

@graydon
Copy link
Contributor Author

graydon commented Sep 2, 2022

@MonsieurNicolas rebased, updated, cleaned out spurious merge commit, and actually synchronized the input rust and C++ XDRs (they had become out of sync on trunk again, and this change caught it). This should land ASAP.

@MonsieurNicolas
Copy link
Contributor

r+ d89c499

@latobarita latobarita merged commit 364b2f7 into stellar:master Sep 3, 2022
v19.4.0 automation moved this from In progress to Done Sep 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants