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.

Sign up for GitHub

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

Jump to bottom

Remove serde dependency #251

Merged
merged 2 commits into from
Oct 10, 2024
Merged

Remove serde dependency #251

merged 2 commits into from
Oct 10, 2024

Conversation

uncomputable
Copy link
Collaborator

@uncomputable uncomputable commented Oct 7, 2024 •
edited
Loading

Remove the serde_json dependency when the serde feature is disabled. Update libsimplicity.

apoelstra
apoelstra reviewed Oct 7, 2024
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
@uncomputable uncomputable force-pushed the 2024-10-disable-serde branch from 2a2e7fb to 9ff40ba Compare October 7, 2024 20:57
@apoelstra
Copy link
Collaborator

Looks like the lockfiles need to be updated.

@uncomputable uncomputable force-pushed the 2024-10-disable-serde branch from 9ff40ba to 505e219 Compare October 8, 2024 15:56
@uncomputable
Copy link
Collaborator Author

uncomputable commented Oct 8, 2024 •
edited
Loading

Lockfiles are updated. CI seems to be happy

@apoelstra
Copy link
Collaborator

I apparently can't test this until we update to ghc 9.8, because my script can't verify that the generated files are correct.

@apoelstra
Copy link
Collaborator

@uncomputable can you open a new PR which updates libsimplicity?

@uncomputable
Copy link
Collaborator Author

I am on it. I was thinking of pushing to this PR.

@uncomputable uncomputable marked this pull request as draft October 8, 2024 22:36
@uncomputable
Copy link
Collaborator Author

I updated the Merkle roots, but for some reason C and Rust still don't agree on them. It is getting late, so I will take this up again tomorrow. Drafting the PR.

@apoelstra
Copy link
Collaborator

@uncomputable I patched this up locally to create a test transaction. There are two changes you need to make:

  • The Cmr::BITS array needs to be updated (you can just change them to the value that makes the unit tests pass; and you can check against the C code in precomputed.h if you want to be sure)
  • A couple more unit tests have fixed vectors in them, which need to be updated.

@uncomputable
chore: Update libsimplicity
c85c6f1 
Update name of elementsJets.c. Update Merkle root IVs based on change
from `Simplicity-Draft` to `Simplicity`. Add tooling to upgrade Merkle
roots faster in the future. Update Merkle roots in unit tests.
@uncomputable
chore: Remove serde dependency
905bf9b 
rust-elements has the default feature "json-contract" which depends on
serde_json. We don't need this feature in rust-simplicity.
@uncomputable uncomputable force-pushed the 2024-10-disable-serde branch from 0c10f52 to 905bf9b Compare October 10, 2024 17:25
@uncomputable
Copy link
Collaborator Author

@apoelstra Thanks for pointing to Cmr::BITS. I was unsure which hardcoded Merkle root constants were wrong and I missed Cmr::BITS. I added a unit test with print statements for future updates.

@uncomputable uncomputable marked this pull request as ready for review October 10, 2024 17:29
@apoelstra
Copy link
Collaborator

utack c85c6f1 except that we should leave the names of the Tmr IVs alone, and keep exporting them as pub

@uncomputable
Copy link
Collaborator Author

uncomputable commented Oct 10, 2024 •
edited
Loading

I can add getters to the Tmr constants if we need them. The names of the Tmr constants changed due to the code that generates them. The new names are consistent with the names of the Cmr constants.

apoelstra
apoelstra approved these changes Oct 10, 2024
View reviewed changes
Copy link
Collaborator

@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 905bf9b successfully ran local tests

@apoelstra apoelstra merged commit 4ec0aba into BlockstreamResearch:master Oct 10, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@uncomputable @apoelstra