multi: Remove agenda flags from DetermineTxType. #2922
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Upcoming changes constitute breaking public API changes to the gcs module, therefore, this follows the process for introducing major API breaks which consists of: - Bump the major version in the go.mod of the affected module if not already done since the last release tag - Add a replacement to the go.mod in the main module if not already done since the last release tag - Update all imports in the repo to use the new major version as necessary - Make necessary modifications to allow all other modules to use the new version in the same commit - Repeat the process for any other modules the require a new major as a result of consuming the new major(s)
Upcoming changes constitute breaking public API changes to the blockchain/stake module, therefore, this follows the process for introducing major API breaks which consists of: - Bump the major version in the go.mod of the affected module if not already done since the last release tag - Add a replacement to the go.mod in the main module if not already done since the last release tag - Update all imports in the repo to use the new major version as necessary - Make necessary modifications to allow all other modules to use the new version in the same commit - Repeat the process for any other modules the require a new major as a result of consuming the new major(s)
davecgh
added
the
waiting for consensus change
davecgh
reviewed
Apr 12, 2022
There was a problem hiding this comment.
This is a nice change and I appreciate the logical commit structure that makes it so much easier to review for correctness.
I've thoroughly reviewed this and only spotted one thing in the tests that I called out inline. Once the necessary changes activate, I'll run a sync on mainnet and testnet with --nocheckpoints --assumevalid=0 as well to serve as an additional double check.
This updates several test vote transactions to be version 1, since that is the actual version that is currently used.
This updates the stake.CheckSSRtx function to use the transaction version rather than the auto revocations agenda flag to determine whether to apply additional checks. This is now safe since: - The automatic revocations agenda has activated - Consensus rules enforce that the revocation transaction version MUST be 2 if the automatic revocations agenda is active - There are no revocations with a transaction version greater than or equal to 2 in any blocks prior to the agenda activating on mainnet
This removes the auto revocations agenda flag from the stake.IsSSRtx function since it is now unused due to it being removed from the stake.CheckSSRtx function.
This updates the stake.CheckSSGenVotes function to remove the treasury agenda check, but still retain the logic to reject vote transactions that contain a null data script in the last output but don't have a transaction version equal to the treasury transaction version. This is now safe since: - The treasury agenda has activated - There are no transactions that violate this rule in any blocks prior to the agenda activating
This removes the treasury agenda flag from the stake.CheckSSGen function since it is now unused due to it being removed from the stake.CheckSSGenVotes function.
This removes the treasury agenda flag from the stake.IsSSGen function since it is now unused due to it being removed from the stake.CheckSSGen function.
This removes the treasury agenda flag from the several functions since it is now unused due to it being removed from the stake.IsSSGen function.
This updates stake.DetermineTxType to use the tx version to check if the treasury agenda is active rather than relying on the agenda flag. This is safe since the treasury agenda has activated and there are no transactions with a transaction version greater than or equal to the treasury transaction version in any blocks prior to the agenda activating.
This removes the treasury and auto revocations agenda flags from the stake.DetermineTxType since they are no longer used.
This removes the treasury and auto revocations agenda flags from the several functions since it is now unused due to it being removed from the stake.DetermineTxType function.
rstaudt2
force-pushed
the
check-auto-revocation-tx-version
branch
from
6b1eb41
to
a415a35
Compare
davecgh
approved these changes
Apr 12, 2022
dnldd
approved these changes
Apr 19, 2022
|
I've now run the aforementioned sync on mainnet and testnet with |
davecgh
removed
the
waiting for consensus change
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTE: This is ready for review, however, it can't be merged until the auto revocations consensus change agenda becomes active.
--
This removes the auto revocations and treasury agenda flags from
stake.DetermineTxType,stake.IsSSGen,stake.CheckSSGen,stake.CheckSSGenVotes,stake.CheckSSRtx,stake.CheckSSRtx, and several other functions where the flags became unused due to them being removed from these functions.Instead of using the agenda flags, this updates the logic to use the transaction version as a proxy for the agendas. This is desirable because callers need global state to properly specify the agenda flags, whereas the transaction version can be determined from the transaction itself.
This is broken down into several commits to ease the review process. The major changes to call out are:
stake.CheckSSRtxstake.CheckSSRtxfunction to use the transaction version rather than the auto revocations agenda flag to determine whether to apply additional checksstake.CheckSSGenVotesstake.CheckSSGenVotesfunction to remove the treasury agenda check, but still retain the logic to reject vote transactions that contain a null data script in the last output but don't have a transaction version equal to the treasury transaction versionstake.DetermineTxTypestake.DetermineTxTypeto use the tx version to check if the treasury agenda is active rather than relying on the agenda flag