Skip to content

Make primitive.h functions indirect #292

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

Merged
merged 1 commit into from
May 16, 2025
Merged

Conversation

roconnor-blockstream
Copy link
Collaborator

The original plan here was to build different Simplicity primitives (e.g. Elements and Bitcoin), which are selected at link time.

However, we would like to link multiple sets of primitives into Haskell for testing purposes. This means we will need different names for the Bitcoin and Elements to bind them.

To support this we turn the primitive.h functions into callbacks for the various specific primitive implementations.

Various functions are rearranged to support the removal of primitive.h and make the pairing of .c file implementing .h header files more consistent. It should now be the case that each .c files implement exactly the declairations of one .h file.

The original plan here was to build different Simplicity primitives (e.g.
Elements and Bitcoin), which are selected at link time.

However, we would like to link multiple sets of primitives into Haskell for
testing purposes.  This means we will need different names for the Bitcoin and
Elements to bind them.

To support this we turn the primitive.h functions into callbacks for the various
specific primitive implementations.

Various functions are rearranged to support the removal of primitive.h and make
the pairing of .c file implementing .h header files more consistent.  It should
now be the case that each .c files implement exactly the declairations of one
.h file.
Copy link
Contributor

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 1a4006c; successfully ran local tests; though without seeing the bitcoin code it is hard to tell what exactly needs to be pulled into callbacks

@roconnor-blockstream
Copy link
Collaborator Author

I pushed an update to my WIP bitcoin branch you can take a peek at. Notably simplicity_bitcoin_decodeJet and simplicity_elements_decodeJet are both used in the Haskell FFI as part of testing, and thus they need to have different names in order for them to link. You can sort of see part of this aspect in this PR in that Haskell/Tests/Simplicity/Elements/FFI/Primitive.hs has been updated to use the new simplicity_elements_decodeJet.

The old name for this function was simplicity_decodeJet and it was part of the interface where we "use link-time to change the Simplicity application from Elements, to Bitcoin, (to potentially alternative Simplicity applications)" idea. But if the function name is change to simplicity_elements_decodeJet to allow multiple applications to be linked simultaneously in Haskell and Rust bindings, then something has to be done about the old simplicity_decodeJet.

And that something is going to be using function pointers for simplicity_elements_decodeJet in some way, either by assigning them to some sort of const global variable (an alternative approach that I tried), or by fishing the function pointers through the call stack, this approach, which I'd argue is a bit better than using global variables.

@apoelstra
Copy link
Contributor

Ok, perfect. So I understand the motivation here, and it's obvious that decodeJets needs to be different for bitcoin and elements. My confusion was that you're also splitting mallocBoundVars which doesn't obviously have a bitcoin/elements dependence. So I worry that there are other "nonobvious" cases.

@apoelstra
Copy link
Contributor

Ok, out of band you told me that the "things that need to split" are "the things that were in primitives.h" which is straightforward enough.

@roconnor-blockstream
Copy link
Collaborator Author

Yep. mallocBoundVars also pre-allocates all types which are used in jets, which is why it is application dependent.

There might be a better design surrounding that bit of code, but this design had the best tradeoffs that I could think of at the time.

@roconnor-blockstream roconnor-blockstream merged commit 1a4006c into master May 16, 2025
2 checks passed
@roconnor-blockstream roconnor-blockstream deleted the primitive-pointer branch May 16, 2025 15:33
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.

2 participants