Skip to content

Count sigops using generics - #9670

Merged
mergify[bot] merged 24 commits into
mainfrom
refactor-sigops
Jul 29, 2025
Merged

Count sigops using generics#9670
mergify[bot] merged 24 commits into
mainfrom
refactor-sigops

Conversation

@upbqdn

@upbqdn upbqdn commented Jul 1, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Motivation

Solution

  • Define a new Sigops trait.
  • Impl the trait for relevant tx types.
  • Clean-ups:
    • Use u32 instead of u64 for the sigops count since that's what the script interpreter, which returns the counts for tx inputs and outputs, uses.
    • Move tests to their own files.

PR Checklist

  • The PR name is suitable for the release notes.
  • The solution is tested.
  • The documentation is up to date.

@upbqdn
upbqdn requested a review from arya2 July 1, 2025 13:09
@upbqdn upbqdn self-assigned this Jul 1, 2025
@upbqdn
upbqdn marked this pull request as ready for review July 14, 2025 14:47
@upbqdn
upbqdn requested review from a team as code owners July 14, 2025 14:47
@upbqdn
upbqdn requested review from conradoplg and removed request for a team July 14, 2025 14:47
@upbqdn

upbqdn commented Jul 15, 2025

Copy link
Copy Markdown
Member Author

@gustavovalverde, could you take a look at the timeouts? They don't seem related to the PR.

upbqdn
upbqdn commented Jul 16, 2025
View reviewed changes
Comment thread zebra-script/src/lib.rs
arya2
arya2 previously approved these changes Jul 17, 2025
View reviewed changes

@arya2 arya2 left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Look good to me as long as there's a test for the Sigops impl for the zcash_primitives transaction type.

It would be nice to have Sigops implemented in zcash-script, but not necessary.

I particularly like the use of try_fold().

Comment thread zebra-script/src/lib.rs
@conradoplg conradoplg added the do-not-merge Tells Mergify not to merge this PR label Jul 22, 2025
conradoplg
conradoplg reviewed Jul 22, 2025
View reviewed changes

@conradoplg conradoplg left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good, left some fixes and suggestions

Comment thread zebra-script/Cargo.toml Outdated
Comment thread zebra-script/src/lib.rs
Comment thread zebra-script/src/lib.rs Outdated
Comment thread zebrad/tests/acceptance.rs
conradoplg
conradoplg requested changes Jul 22, 2025
View reviewed changes

@conradoplg conradoplg left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

(blocking on the import fix)

@github-project-automation github-project-automation Bot moved this from In progress to Review/QA in Zebra Jul 22, 2025
upbqdn and others added 2 commits July 28, 2025 11:15
upbqdn added 2 commits July 28, 2025 11:43
@upbqdn
Clarify use of dummy values in sigops counting
39cdd62
@upbqdn
Filter coinbase inputs from script iterator in Sigops implementation
11dd9b1 
Previously, coinbase inputs yielded empty byte slices; now they are
excluded. This simplifies downstream consumers and avoids passing empty
scripts.
@mergify mergify Bot mentioned this pull request Jul 29, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@conradoplg conradoplg conradoplg approved these changes
@arya2 arya2 arya2 left review comments

Assignees

@upbqdn upbqdn

Labels

P-Medium ⚡

Projects

No open projects
Zebra
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Count sigops for multiple tx types

4 participants

@upbqdn @conradoplg @arya2 @natalieesk