Simplicity bindings #22

uncomputable · 2022-04-13T19:45:32Z

Exposes part of Simplicity's C API in Rust. At the moment, one can decode Simplicity DAGs from bytes, and one can access test programs with hardcoded bytes and their correct Merkle roots.

Todo

Discussion: use minimal set of necessary C files instead of entire library?
Discussion: use C test files?
~~test CMRs and IMRs against C~~ [postponed to later PR due to outdated jets]
Make sure that FFI is safe and that pointers are freed

uncomputable · 2022-04-13T19:57:06Z

Please check if this is the way we should expose C bindings, @apoelstra, @sanket1729. I mimicked secp256k1-sys with important differences:

There is no renaming / tagging of functions.
The crate is called simplicity_sys, since hyphens are not allowed in crate identifiers.

apoelstra · 2022-04-20T20:15:32Z

simplicity-sys/build.rs

+    .into_iter()
+    .map(|x| simplicity_path.join(x))
+    .collect();
+    let test_files: Vec<_> = vec![


In cdee8cb:

I don't think we need this array (or these files, for that matter).

Ah -- you have unit tests in -sys that use them. In this case I think you could gate this array with #[cfg(test)] so that it's not built when simplicity-sys is used as a dependency.

We could reduce the built files to a minimum. For instance, in order to construct C-DAGs from binary, we definitely need decodeMallocDag() from dag.c.

The point of this PR is to expose (1) a way to decode DAGs from bytes in C, and (2) test programs with hardcoded bytes and Merkle roots. This is why we need test_files. My original plan was to test Rust against C, but this has to be done in a future PR due to outdated jets.

apoelstra · 2022-04-20T20:18:06Z

simplicity-sys/src/bitstream.rs

+}
+
+impl From<&[u8]> for Bitstream {
+    fn from(bytes: &[u8]) -> Self {


In 8e45d76:

This entire function needs to be marked unsafe, or better, you need to tie the lifetime of bytes to the Bitstream type (perhaps with one of the types in std::marker). As written it takes an array bytes and then returns an object pointing into bytes' data which might outlive bytes.

I updated Bitstream to Bitstream<'a> where 'a is the lifetime of &bytes.

apoelstra · 2022-04-20T20:20:23Z

simplicity-sys/src/dag.rs

+    jet: *const c_void,
+    cmr: [u32; 8],
+    // Assume largest union member
+    union_aux_types: [size_t; 2],


In 8e45d76:

This makes me a little nervous. Rust has a union keyword. Could you use that?

DagNode now uses unions, namely AuxTypes [1] and IndicesChildrenWitness [2].

apoelstra · 2022-04-20T20:24:22Z

Regarding "should we try to minimize the set of C", I vote no. The whole C library is super small; better to use the whole thing to make review easier. Link-time optimization will strip unused symbols from the final binaries.

Done reviewing 8e45d76. Overall this looks great!

uncomputable · 2022-04-26T17:07:55Z

simplicity-sys/src/dag.rs

+    /// Returns the DAG and its length on success.
+    pub fn decode(bytes: &[u8]) -> Result<(&mut DagNode, usize), Error> {
+        let mut bitstream = Bitstream::from(bytes);
+        let mut dag: *mut DagNode = std::ptr::null_mut();


Do we have to explicitly free the DAG in the destructor of DagNode? In the C code, the DAG is freed after use.

Yep, you need to free dag in the Drop impl of DagNode. Good catch.

apoelstra · 2022-04-30T15:54:32Z

simplicity-sys/src/bitstream.rs

+        unsafe {
+            let mode = "r";
+            // `file` does not need to be freed as
+            // `bytes` will free itself at the end of its lifetime


In 213c697:

This comment doesn't make sense to me. At some point you do need to fclose file, don't you?

Bitstream<'a> lives at most as long as &'a bytes. According to man pages, fmemopen does not open a file descriptor, nor does a read-only stream have any buffers that need to be flushed. fclose simply closes file descriptors and flushes. If I understand correctly, this means that the FILE pointer in the stream can safely be dropped without freeing it.

That being said, not freeing a FILE pointer is probably bad practice. I will add a destructor.

I'm sure that you need to actually call fclose at some point in this source file.

Otherwise, the lifetime stuff looks like it's been fixed.

Actually, on further reading I don't know. I can't find a citation either way. It is surprising that there is no descriptor and that fileno will just fail.

apoelstra · 2022-04-30T15:56:37Z

Briefly reviewed 213c697. (Only changes from the last review were in the final commit; I think we'll need a bit more iteration on this one.)

uncomputable · 2022-05-19T13:51:08Z

I updated how DAGs are represented, and added free and fclose for each allocation and opened stream.

Because Drop now tries to free a DAG when it goes out of scope, we cannot do pointer addition to index the DAG and return a struct of the same type. The fact that DAGs are not their root node is confusing in the first place, so I decided to hide that from the user completely: A DAG has exactly one CMR, namely its root CMR. In future PRs, we can add the notion of DAG nodes, whose lifetime will depend on that of their parent DAG.

uncomputable · 2022-05-19T13:51:33Z

@apoelstra This should be ready to be merged. Sorry for the many force pushes.

apoelstra · 2022-05-19T14:06:26Z

simplicity-sys/src/dag.rs

+    len: usize,
+}
+
+impl<'a> fmt::Debug for Dag {


In a0faaf2:

nit: this 'a shouldn't be there. I'm not sure why the compiler doesn't warn about this.

apoelstra · 2022-05-19T14:13:07Z

Could you also rebase onto master? This currently doesn't have #23 which is making it hard for me to test locally.

apoelstra

ACK 190a669

sanket1729

high level review ACK. Left a couple questions

sanket1729 · 2022-05-25T23:09:26Z

simplicity-sys/src/test.rs

+}
+
+/// Simplicity program with bytes, CMR and IMR for testing.
+pub struct TestProgram {


Where are we testing the decoding works?

I assume that the values from C are correct (within C's context, using its functions). Testing Rust against C is currently not feasible, due to outdated jets, because these tests will fail.

sanket1729 · 2022-05-25T23:15:11Z

simplicity-sys/src/dag.rs

+    fn decode_cmr() {
+        let bytes = SCHNORR0.bytes;
+        let dag = Dag::decode(bytes).expect("decoding");
+        assert_eq!(SCHNORR0.cmr(), dag.cmr());


We should also test that calculation for cmr from SCHNORR0 from rust-simplicity is same as C simplicity. What we are testing here is that is the value exported from C is correctly calculated from C.

I agree; this was my original intention of this PR. Unfortunately, outdated jets etc. make it so the Rust CMR is almost always different than the C CMR (for the same program). Rust's SCHNORR0 and C's SCHNORR0 are also very different programs, the later using jets and being much smaller. In a later PR, I will replace src/test_progs in Rust with tests that use the FFI once there is a prospect that these tests will succeed.

- sub-crate contains C code - copied from ElementsProject/simplicity/commit/35627fc49ad96fcb844cca72ffbc69b6e934cb4c

apoelstra

ACK 66135ce

sanket1729

utACK 66135ce. The build script seems fragile. We should have some vendoring-like script that updates the underlying library when anything changes.

apoelstra · 2022-08-25T14:38:18Z

Let's merge this for now. We can improve things later to write a revendoring script that will do codegen and maybe symbol renaming, but for now this is something we've wanted for a while.

apoelstra reviewed Apr 20, 2022

View reviewed changes

uncomputable commented Apr 26, 2022

View reviewed changes

uncomputable force-pushed the simplicity-sys branch from 8e45d76 to 213c697 Compare April 26, 2022 17:19

apoelstra reviewed Apr 30, 2022

View reviewed changes

uncomputable force-pushed the simplicity-sys branch from 213c697 to a0faaf2 Compare May 19, 2022 13:38

apoelstra reviewed May 19, 2022

View reviewed changes

uncomputable force-pushed the simplicity-sys branch from a0faaf2 to 190a669 Compare May 20, 2022 16:57

apoelstra previously approved these changes May 23, 2022

View reviewed changes

sanket1729 reviewed May 25, 2022

View reviewed changes

uncomputable added 3 commits June 6, 2022 13:43

Add simplicity-sys

2f0d52c

- sub-crate contains C code - copied from ElementsProject/simplicity/commit/35627fc49ad96fcb844cca72ffbc69b6e934cb4c

Update .gitignore

33994ea

Add bindings for DAGs and tests

66135ce

uncomputable dismissed apoelstra’s stale review via 66135ce June 6, 2022 18:44

uncomputable force-pushed the simplicity-sys branch from 190a669 to 66135ce Compare June 6, 2022 18:44

uncomputable requested a review from apoelstra June 11, 2022 15:23

apoelstra approved these changes Jun 15, 2022

View reviewed changes

uncomputable mentioned this pull request Jul 9, 2022

Call FFI during jet execution #29

Closed

3 tasks

sanket1729 approved these changes Aug 23, 2022

View reviewed changes

apoelstra merged commit 3c12828 into BlockstreamResearch:master Aug 25, 2022

uncomputable deleted the simplicity-sys branch August 29, 2022 18:56

Simplicity bindings #22

Simplicity bindings #22

Uh oh!

Conversation

uncomputable commented Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Todo

Uh oh!

uncomputable commented Apr 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uncomputable May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Apr 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Apr 30, 2022

Uh oh!

uncomputable commented May 19, 2022

Uh oh!

uncomputable commented May 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented May 19, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Aug 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

uncomputable commented Apr 13, 2022 •

edited

Loading

uncomputable May 17, 2022 •

edited

Loading