Skip to content

Conversation

@sanket1729
Copy link
Member

This PR aims to settle on a structure for automatically generated files for jets execution. As per the current design, we would need the files:

  1. src/jet/init/core.rs
  2. src/jet/init/elements.rs
  3. simplicity-sys/src/c_jets/jets_wrapper.rs
  4. simplicity-sys/src/c_jets/jets_ffi.rs

The tests here would fail because we are updating simplicity and a lot of jets are implemented. This should PR should pass once we have all the generated files. The goal is to get an ACK on the design.

Ealier this was a dev dependency because it was only used
for testing
This module is assuming bitcoin jets that are not
implemented/unspecified. Comment this out, this will be renabled later
in a separate PR after we have all the jets
Add a new function to_type that outputs Type
This commit also does a couple other things.
1) Makes modification to bitmachine that allows to skip assertions in
test cfg. Useful for creating states of bitmachine wihtout creating a
program.
2) Removes all existing jets that were implemented in rust. This is
   really not a good idea because the definitions of certain jets are
hard to emulate and have special corner/wierd cases. We are best to
directly use the FFI bindings.
3) Adds test for checking FFI with/without env
@uncomputable
Copy link
Collaborator

I like the design. Adding c_jet_ptr to the Jet trait and default-implementing exec based on that is elegant because if I want then I can still implement everything in Rust by overriding exec. I have only skimmed the FFI, but clearly a lot of work has flown into it.

My only real remark is that we should separate the C jet pointers in simplicity-sys::c_jets::jets_wrapper into Core, Bitcoin and Elements. I get it, there is hardly a separation in the C library, but if we can separate the jets somehow, then this would avoid future confusion. Maybe this also answers the question how the jets are separated in the C library in the first place.

Besides that, maybe we can remove the unsafe code in Jet::exec and jet::elements::c_env? I don't know. According to @apoelstra , some unsafe code cannot be avoided. (I just read an article how UB breaks the entire binary, so I'm sensitive.)

Rename CTxEnv -> CElementsTxEnv
Add CTxEnv as a enum to capture all environments
@sanket1729 sanket1729 mentioned this pull request Dec 3, 2022
3 tasks
@apoelstra
Copy link
Collaborator

Overall this looks great. I think we should break it apart into a couple smaller PRs. In particular the 1st and 3rd and 5th commits (update underlying Simplicity library; add cargo:rerun-if-changed=depend to build.rs; make Simplicity a non-dev-dependency could be pulled out and made into an uncontroversial PR, and reduce this one in size from 9 to 6 commits.

Then 479472b and 8de5acc and c33c3dc (which add C getters/setters for the structs; adds the actual jet FFI bindings; tweak the enviroment pointers) could be a separate PR which we could iterate on which does the actual FFI binding.

Then the remaining PRs would be a "actually use the FFI bindings in this library" PR, which initially we'd just use as a reference while we debated the other ones.

I've only briefly skimmed this. I have a few comments but my overall impression is that this PR is way too big to effectively review :).

@sanket1729
Copy link
Member Author

sanket1729 commented Dec 14, 2022

@apoelstra. Cleaned some code, Fixed CI, and split into #48, #49, and #50

@apoelstra
Copy link
Collaborator

Thank you!! Let's close this one then.

@apoelstra apoelstra closed this Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants