Skip to content

Remove redundant sighashes#281

Merged
ValarDragon merged 17 commits into
mainfrom
adam/remove-redundant-sighashes
Jul 20, 2026
Merged

Remove redundant sighashes#281
ValarDragon merged 17 commits into
mainfrom
adam/remove-redundant-sighashes

Conversation

@czarcas7ic

@czarcas7ic czarcas7ic commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Motivation

Zakura reuses one SigHasher per transaction, but signature checks still rebuilt transaction-wide ZIP 243 components for each V4 signature and ZIP 244 amounts and scriptPubKeys digests for each non-ANYONECANPAY V5/V6 signature. With a signature on each transparent input, that repeated work grows quadratically.

Solution

Cache the shared signature-hash components inside zakura-chain. V5/V6 extend Zakura's native ZIP 244 digest tree with amounts, scriptPubKeys, per-input, and SIGHASH_SINGLE output digests. V4 uses a private ZIP 243 cache for prevouts, sequences, outputs, JoinSplits, and Sapling components. V3 and earlier transactions continue to use the existing librustzcash fallback.

Input index, runtime hash type, SIGHASH_SINGLE selection, and legacy scriptCode remain finalized for each requested digest. The interpreter callback and is_valid API stay unchanged, and Zakura continues to use released librustzcash crates.

The published HashType bitflags and sighash_v4_raw return type remain unchanged. Cached paths convert to a private canonical type, while V5/V6 script validation continues to accept only the six defined hash-type bytes.

Specifications & References

Testing

  • cargo fmt --all -- --check
  • cargo test -p zakura-chain --locked
  • cargo test -p zakura-script --locked
  • cargo test -p zakura-consensus --locked script::tests
  • cargo clippy -p zakura-chain -p zakura-script --all-targets --locked -- -D warnings
  • markdownlint zakura-chain/CHANGELOG.md --config .trunk/configs/.markdownlint.yaml

Keep transparent sighashes input- and signature-specific while reusing transaction-wide ZIP 143, ZIP 243, and ZIP 244 components through the librustzcash fork's owned precomputation context.
@czarcas7ic

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9071a6d481

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread Cargo.toml Outdated
Comment thread Cargo.toml Outdated
@czarcas7ic

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: 5542f397cb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@czarcas7ic
czarcas7ic marked this pull request as ready for review July 19, 2026 22:16
Comment thread zakura-chain/src/transaction/zip244.rs
Comment thread zakura-chain/src/transaction/sighash.rs Outdated
@ValarDragon

This comment was marked as resolved.

Comment thread zakura-chain/src/transaction/sighash/legacy.rs Outdated
@ValarDragon

Copy link
Copy Markdown
Contributor

@v12 please review

@evan-forbes

Copy link
Copy Markdown
Contributor

@v12-auditor

@v12-auditor

v12-auditor Bot commented Jul 19, 2026

Copy link
Copy Markdown

Note

Complete: Audit complete. V12 did not find any issues that need review.

Open the full results here.

Analyzed three files, diff 54bf3b6...0004cd4.

@ValarDragon

ValarDragon commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

committed one type change to use LRZ constants instead of re-defining, and then an extra test

Comment thread zakura-chain/src/transaction/sighash.rs

@evan-forbes evan-forbes left a comment

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.

ran a benchmark locally and saw the expected speed improvements

I, fable, and sol couldn't find any consensus breaking changes, but we're running a fuzzer with librustzcash to make sure

very nice

@ValarDragon ValarDragon left a comment

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.

Lgtm after multiple review passes

I still would like to update to not pay allocation or wasted hashing overhead when it’s not needed though

@ValarDragon
ValarDragon merged commit b393b05 into main Jul 20, 2026
54 checks passed
@ValarDragon
ValarDragon deleted the adam/remove-redundant-sighashes branch July 20, 2026 13:10
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