Skip to content

Add DefaultTracker #184

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
Merged
KyrylR merged 5 commits into from
Dec 18, 2025
Merged

Add DefaultTracker #184

KyrylR merged 5 commits into from
Dec 18, 2025

Conversation

@KyrylR
Copy link
Collaborator

@KyrylR KyrylR commented Dec 12, 2025

No description provided.

@KyrylR KyrylR self-assigned this Dec 12, 2025
@KyrylR KyrylR requested a review from imaginator as a code owner December 12, 2025 14:19
KyrylR
KyrylR commented Dec 12, 2025
View reviewed changes
@KyrylR KyrylR force-pushed the feature/tracker branch 3 times, most recently from f0b52f7 to a07ee03 Compare December 12, 2025 15:04
KyrylR
KyrylR commented Dec 12, 2025
View reviewed changes
src/tracker.rs
Comment on lines +168 to +229
// Skip the leading bit when the frame has extra padding.
// This occurs because jets are wrapped in AssertL (a Case combinator),
// which adds structure to the output frame for some jets.
if output_frame.len() > target_ty.bit_width() {
let _ = output_frame.next();
}
Copy link
Collaborator Author

@KyrylR KyrylR Dec 12, 2025

Choose a reason for hiding this comment

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

Though, I still have doubts regarding this

@apoelstra
Copy link
Contributor

apoelstra commented Dec 15, 2025

Overall this looks great. I have two comments in 1f7ef82:

  • You use both custom sinks and eprintln calls. Maybe you should only call the sinks, and let the user make one that calls eprintln if they want stderr?
  • You only handle the "successful jet" case. But I think the user probably cares more than anything about failed jets.

@KyrylR KyrylR force-pushed the feature/tracker branch 2 times, most recently from b0882fb to b5f573d Compare December 15, 2025 16:24
@KyrylR
Copy link
Collaborator Author

KyrylR commented Dec 15, 2025

Overall this looks great. I have two comments in 1f7ef82:

* You use both custom sinks and `eprintln` calls. Maybe you should only call the sinks, and let the user make one that calls `eprintln` if they want stderr?

* You only handle the "successful jet" case. But I think the user probably cares more than anything about failed jets.

Good point, migrated to sinks, and here how it looks when jet fails:

image

apoelstra
apoelstra approved these changes Dec 17, 2025
View reviewed changes
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 74f8c06; successfully ran local tests

@KyrylR
Copy link
Collaborator Author

KyrylR commented Dec 18, 2025

@apoelstra sorry, forgot to bump version

@apoelstra
Copy link
Contributor

apoelstra commented Dec 18, 2025

@KyrylR do you think I should cut a release immediately? If so, can you update CHANGELOG.md?

Based on

git log master --not simplicityhl-0.3.0 --first-parent --oneline

it looks like this is the only user-visible change to SimplicityHL.

apoelstra
apoelstra approved these changes Dec 18, 2025
View reviewed changes
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 813140b; successfully ran local tests

@KyrylR KyrylR merged commit 31783fe into master Dec 18, 2025
10 checks passed
@KyrylR
Copy link
Collaborator Author

KyrylR commented Dec 18, 2025

Merged and published. Thanks!

@apoelstra apoelstra deleted the feature/tracker branch December 18, 2025 16:09
@apoelstra
Copy link
Contributor

apoelstra commented Dec 19, 2025 •
edited
Loading

@KyrylR can you also make a git tag (git tag -as). See git tag for the current list and git show <tag> for the text added to each one. (Though the text doesn't matter, I think. I've never read this text, but git makes you put it in, so I usually just repeat the crate name.)

@KyrylR
Copy link
Collaborator Author

KyrylR commented Dec 19, 2025

@apoelstra, Done: https://github.com/BlockstreamResearch/SimplicityHL/tags

@apoelstra
Copy link
Contributor

apoelstra commented Dec 19, 2025

Great, thanks!

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

@uncomputable uncomputable Awaiting requested review from uncomputable

@imaginator imaginator Awaiting requested review from imaginator imaginator is a code owner

Assignees

@KyrylR KyrylR

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@KyrylR @apoelstra