multi: Add isAutoRevocationsEnabled to CheckSSRtx.

[rstaudt2]

Requires #2718.

---

Note: This modifies the following exported functions from the stake package:

• func CheckSSRtx(tx *wire.MsgTx, isAutoRevocationsEnabled bool) error
• func IsSSRtx(tx *wire.MsgTx, isAutoRevocationsEnabled bool) bool
• func DetermineTxType(tx *wire.MsgTx, isTreasuryEnabled bool, isAutoRevocationsEnabled bool) TxType

---

This adds the isAutoRevocationsEnabled flag to stake.CheckSSRtx, which in turn requires it to be added to stake.DetermineTxType and updating all callers.

This is required for stake.CheckSSRtx since it will be updated to return errors if revocation transactions are not valid when the automatic ticket revocations agenda is enabled.

This is not ideal since it requires passing around the context-based isAutoRevocationsEnabled flag in many places.

If the automatic ticket revocations agenda activates and there are no existing version 2 revocation transactions in blocks, then this could retroactively be removed and the updated checks can be based on the transaction version instead.

This does not contain any consensus rule changes. The consensus rule changes for DCP0009 will come in a separate pull request that is dependent on this one.

[matheusd]

Have you considered simply adding the specific contextual check for the new revocations within checkBlockContext() (e.g. through adding a CheckRRTxContext()) instead?

This would reduce the code churn due to doing all this then (assuming the agenda passes) moving everything back into place.

Granted, this would make CheckRRTx incomplete (in the sense that it would be insufficient to fully determine whether a tx is in fact a revocation once the autorevocation agenda actives) and we'd probably have to verify the assumptions in some other functions (that rely on a prior CheckSSRtx being applied), but splitting the checks would better match the actual activation process, given that all the checks in the original CheckSSRtx are still applied.

[rstaudt2]

Have you considered simply adding the specific contextual check for the new revocations within checkBlockContext() (e.g. through adding a CheckRRTxContext()) instead?

This would reduce the code churn due to doing all this then (assuming the agenda passes) moving everything back into place.

Granted, this would make CheckRRTx incomplete (in the sense that it would be insufficient to fully determine whether a tx is in fact a revocation once the autorevocation agenda actives) and we'd probably have to verify the assumptions in some other functions (that rely on a prior CheckSSRtx being applied), but splitting the checks would better match the actual activation process, given that all the checks in the original CheckSSRtx are still applied.

Yeah, I did consider something along those lines. Unfortunately, as you noted, if we don't update CheckSSRtx (which in turn requires updating IsSSRtx and DetermineTxType), then invalid transactions under the new rules may be incorrectly identified as revocation transactions. Specifically, transactions that would otherwise be identified as revocations but have a signature script or a non-zero fee, which violate the new rules, would still be identified as revocation transactions.

Since stake.DetermineTxType is used very widely (including externally in dcrwallet, dcrdata, etc.) I opted to go the route of making that function as accurate as possible. However, this is also why I put this change in a separate PR, as I figured it would be worth discussing this tradeoff.

[matheusd]

Specifically, transactions that would otherwise be identified as revocations but have a signature script or a non-zero fee, which violate the new rules, would still be identified as revocation transactions.

Sure. But in practice, would something break specifically?

Enforcement of the consensus rules for the additional conditions under the new rules would still be happening as appropriate (as an additional contextual check that would be added). The wallet (which iirc is the only other software which deals with revocations) could be modified to also verify the correct condition if we really wanted to make sure not extraneous data could make its way into the db.

BTW, I'm not necessarily opposed to doing the check in one go based on the isAutoRevocationsEnabled arg (specially since you've already went ahead and wrote it and modified all call sites) just trying to get a feel for the rationale of this way of making this change.

[rstaudt2]

Specifically, transactions that would otherwise be identified as revocations but have a signature script or a non-zero fee, which violate the new rules, would still be identified as revocation transactions.

Sure. But in practice, would something break specifically?

Enforcement of the consensus rules for the additional conditions under the new rules would still be happening as appropriate (as an additional contextual check that would be added). The wallet (which iirc is the only other software which deals with revocations) could be modified to also verify the correct condition if we really wanted to make sure not extraneous data could make its way into the db.

BTW, I'm not necessarily opposed to doing the check in one go based on the isAutoRevocationsEnabled arg (specially since you've already went ahead and wrote it and modified all call sites) just trying to get a feel for the rationale of this way of making this change.

Nothing would break. It would still be enforced by consensus, as you stated.

The reasons I am slightly in favor of the changes in this PR are:

• It is consistent with the existing behavior of stake.DetermineTxType in that a transaction will not be considered a given type if the transaction itself is not structured appropriately, as opposed to using the minimum criteria needed to identify a transaction type
  • For example, if a treasury base has a signature script in its zeroeth input, it will not be considered a treasury base transaction (source), which is equivalent to one of the checks being added for revocations in this PR
• stake.DetermineTxType already requires context and makes decisions based on the context with the isTreasuryEnabled flag being passed

In short, I don't think it would necessarily cause any issues not to do this, but I went in this direction because it is more technically correct and consistent with the existing code, which makes the behavior of the function easier to reason about. Other than making the updates in this PR, I don't think there is much of a downside since callers already have to pass isTreasuryEnabled anyway.

Either way, we ultimately want to remove both the isTreasuryEnabled and isAutoRevocationsEnabled flags in favor of using the transaction version for these types of checks, which will be much nicer. I'm hoping we can retroactively update this to be based on the version instead as long as there are no violations prior to the agenda activating. And it will be nice that we shouldn't have to deal with this in the future once the explicit versions consensus change activates.

[JoeGruffins]

There are a lot of places I am compelled to whine about args having bool two or three times, but I guess DCP0008 will allow those bools to be removed completely? Anyway good work.

[rstaudt2]

There are a lot of places I am compelled to whine about args having bool two or three times

I went ahead and updated those since it doesn't involve any functional changes and is arguably the more idiomatic approach.

but I guess DCP0008 will allow those bools to be removed completely? Anyway good work.

If there are no version 2 revocation transactions in blocks prior to the agenda activating, then the agenda flag can be removed here, and instead the transaction version can be used as a proxy for the agenda being active. If a version 2 revocation transaction does exist in a block prior to the agenda activating (which would be fairly unlikely), we'd need another consensus change after DCP0008 is active to bump the revocation transaction version to something higher than the max allowed tx version in order to use the tx version as a proxy for the agenda activating.