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): Allow transactions spending coinbase outputs to have transparent outputs on Regtest #9085

Merged
merged 6 commits into from
Dec 20, 2024

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Dec 13, 2024

Motivation

We want Zebra's behaviour on Regtest to match zcashd as closely as possible, and zcashd allows transactions spending from coinbase inputs to have transparent outputs on Regtest.

Closes #8478.

Solution

  • Adds a should_allow_unshielded_coinbase_spends field to testnet::Parameters
    • Sets it as true on Regtest
  • Updates the coinbase_spend_restriction() method to always return OnlyShieldedOutputs on Regtest

Tests

Added a unit test to check that the transaction verifier returns Ok for a transaction that spends coinbase inputs and has transparent outputs.

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.

PR Reviewer's Checklist

  • The PR Author's checklist is complete.
  • The PR resolves the issue.

@arya2 arya2 added A-consensus Area: Consensus rule updates C-testing Category: These are tests P-Medium ⚡ labels Dec 13, 2024
@arya2 arya2 self-assigned this Dec 13, 2024
@arya2 arya2 requested a review from a team as a code owner December 13, 2024 03:03
@arya2 arya2 requested review from oxarbitrage and removed request for a team December 13, 2024 03:03
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Dec 13, 2024
@mpguerra mpguerra requested review from upbqdn and removed request for oxarbitrage December 16, 2024 15:45
@arya2
Copy link
Contributor Author

arya2 commented Dec 18, 2024

@mergify update

Copy link
Contributor

mergify bot commented Dec 18, 2024

update

✅ Branch has been successfully updated

Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

I think it would be better to change the name of the PR name so that it mentions spending coinbase outputs instead of inputs.

zebra-chain/src/parameters/network/testnet.rs Show resolved Hide resolved
zebra-chain/src/transaction.rs Outdated Show resolved Hide resolved
zebra-chain/src/parameters/network/testnet.rs Outdated Show resolved Hide resolved
zebra-chain/src/parameters/network/testnet.rs Outdated Show resolved Hide resolved
zebra-chain/src/transaction.rs Outdated Show resolved Hide resolved
zebra-chain/src/parameters/network/testnet.rs Outdated Show resolved Hide resolved
zebra-chain/src/parameters/network/testnet.rs Outdated Show resolved Hide resolved
zebra-chain/src/parameters/network/testnet.rs Outdated Show resolved Hide resolved
zebra-chain/src/parameters/network/testnet.rs Outdated Show resolved Hide resolved
zebra-consensus/src/transaction/tests.rs Show resolved Hide resolved
zebra-consensus/src/transaction/tests.rs Outdated Show resolved Hide resolved
zebra-consensus/src/transaction/tests.rs Outdated Show resolved Hide resolved
zebra-consensus/src/transaction/tests.rs Outdated Show resolved Hide resolved
zebra-consensus/src/transaction/tests.rs Outdated Show resolved Hide resolved
upbqdn
upbqdn previously approved these changes Dec 19, 2024
zebra-consensus/src/transaction/tests.rs Outdated Show resolved Hide resolved
zebra-consensus/src/transaction/tests.rs Outdated Show resolved Hide resolved
zebra-consensus/src/transaction/tests.rs Show resolved Hide resolved
zebra-consensus/src/transaction/tests.rs Outdated Show resolved Hide resolved
@arya2 arya2 changed the title change(consensus): Allow transactions spending from coinbase inputs to have transparent outputs on Regtest change(consensus): Allow transactions spending coinbase outputs to have transparent outputs on Regtest Dec 19, 2024
Co-authored-by: Marek <mail@marek.onl>
@arya2 arya2 requested a review from a team as a code owner December 19, 2024 20:25
@arya2 arya2 requested a review from upbqdn December 19, 2024 21:55
@arya2 arya2 removed the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Dec 19, 2024
mergify bot added a commit that referenced this pull request Dec 20, 2024
@mergify mergify bot merged commit 7597cf1 into main Dec 20, 2024
147 checks passed
@mergify mergify bot deleted the regtest-allow-transparent-coinbase-spends branch December 20, 2024 01:29
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 C-testing Category: These are tests P-Medium ⚡
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow for coinbases to be spent to transparent outputs on regtest
2 participants