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.

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

change(consensus): Optimize checks for coinbase transactions #9126

Merged
merged 3 commits into from
Jan 15, 2025

Conversation

upbqdn
Copy link
Member

@upbqdn upbqdn commented Jan 15, 2025

Motivation

The original motivation was to address #9098 (comment) and #9098 (comment). Then I noticed that the coinbase_outputs_are_decryptable fn currently converts each post-Heartwood coinbase tx to check if its shielded outputs are decryptable, even if the tx has no shielded outputs. This conversion involves a serialization and deserialization cycle. We can skip this conversion for txs with no shielded outputs.

Solution

  • Return early if the tx has no shielded outputs.
  • Return a new Transaction::NotCoinbase error if the tx is not coinbase but should be.
  • Update docs.

Tests

  • Improve tests so they check more scenarios.

PR Author's Checklist

  • The PR name will make sense to users.
  • The PR provides a CHANGELOG summary.
  • The solution is tested.
  • The documentation is up to date.
  • The PR has a priority label.

@upbqdn upbqdn added A-consensus Area: Consensus rule updates P-Low ❄️ labels Jan 15, 2025
@upbqdn upbqdn self-assigned this Jan 15, 2025
@upbqdn upbqdn requested a review from a team as a code owner January 15, 2025 17:36
@upbqdn upbqdn requested review from arya2 and removed request for a team January 15, 2025 17:36
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jan 15, 2025
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@upbqdn upbqdn removed the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jan 15, 2025
mergify bot added a commit that referenced this pull request Jan 15, 2025
mergify bot added a commit that referenced this pull request Jan 15, 2025
@mergify mergify bot merged commit 522d955 into main Jan 15, 2025
145 checks passed
@mergify mergify bot deleted the fix-coinbase-decrypt branch January 15, 2025 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates P-Low ❄️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants