multi: Implement reject new script and tx vers.

[davecgh]

This requires #2713, #2715, and #2721.

This implements the agenda for voting on rejecting new transaction and script versions until they have explicitly been enabled by a consensus rule change as defined in DCP0008 along with consensus tests.

In particular, once the vote has passed and is active, transaction versions greater than 3 and script versions greater than 0 will no longer be accepted by consensus.

This PR also updates the mempool policy when considering candidate transactions for acceptance to the mempool, relaying, and inclusion into block templates to enforce the new version behavior at a consensus level versus via the standardness policy. In practice, this change means the following:

• For mainnet, there will be no externally observable difference since standardness rules are already enforced on mainnet and they already prevent transactions which violate the new rules
• For testnet (where non-standard transactions are allowed) the mempool policy will now match the existing mainnet policy in terms of version handling
  • Namely, any transaction versions greater than 3 and script versions greater than 0 will no longer be accepted

Note that this does not implement block version bumps in the mining and validation code that will ultimately be needed since another consensus vote is expected to land before voting begins.

The following is an overview of the changes:

• Introduce a convenience function for determining if the vote passed and is now active
• Modify block validation to enforce the rejection of newer transaction versions in accordance with the state of the vote
• Modify block validation to enforce the rejection of newer script versions in accordance with the state of the vote
• Add tests for determining if the agenda is active for both mainnet and testnet
• Add tests to ensure proper behavior for new transaction versions as follows:
  • New regular and stake transaction versions are rejected once the agenda is active
  • Existing regular and stake outputs created with newer tx versions are spendable before the agenda is active and remain spendable after it is active
• Add tests to ensure proper behavior for new script versions as follows:
  • Ensure stake transactions still require version 0 scripts regardless of the state of the agenda
  • Regular transactions with script versions newer than 0 are rejected once the agenda is active
  • Transaction outputs that have a non-zero output and script that would make the output unspendable if it were version 0 are spendable with a nil signature script both before the agenda is active and remain spendable after it is active
• Modify the mempool policy to enforce the explicit version upgrade requirements via the consensus rules versus via the standardness rules
• Remove the mempool MaxTxVersion field from the Policy struct
• Remove the mempool max tx version checking logic from checkTransactionStandard
• Remove the mempool max tx version param from checkTransactionStandard
• Update the mempool transaction standardness tests accordingly
• Update all callers to account for the aforementioned mempool changes accordingly

[matheusd]

Reviewing first relevant commit.

mempool.go needs to also be added the AFExplicitVerUpgrades flag here though I guess you might wanna to deal with mempool in a separate PR, so as to remove the MaxTxVersion config knob.

This is also probably inconsequential, but I notice that miners running with --acceptnonstd that happen to have a mempool tx with an incompatible tx version, might end up in a state where they keep trying to generate a block that can't be accepted anymore (once this upgrade activates). This can be dealt with in the template generation code or by just instructing miners to restart their nodes if this happens to them.

[matheusd]

I double checked, and the only places that are calling checkTransactionContext are indeed checkBlockContext() (where the flags are correctly set according to the results of the vote) and on the exported version CheckTransaction() where it's up to the caller to set the correct flags.

[matheusd]

Reviewed second relevant commit and only have the same considerations re: script version in mempool and miners if they have txs in their mempool that become invalid when the new agenda activates.

[davecgh]

mempool.go needs to also be added the AFExplicitVerUpgrades flag here though I guess you might wanna to deal with mempool in a separate PR, so as to remove the MaxTxVersion config knob.

This is also probably inconsequential, but I notice that miners running with --acceptnonstd that happen to have a mempool tx with an incompatible tx version, might end up in a state where they keep trying to generate a block that can't be accepted anymore (once this upgrade activates). This can be dealt with in the template generation code or by just instructing miners to restart their nodes if this happens to them.

Aye, I have a separate PR I was working on to deal with mempool by removing MaxTxVersion and making it always enforce the new semantics from mempool ahead of the vote since mempool already rejects such transactions as non-standard, and anything more restrictive in mempool is purely policy as opposed to consensus.

That said, it's a fair point it should probably be a part of this PR too in order to avoid what looks like missed handling. I'll get a commit added accordingly.

[davecgh]

I've updated the PR to include two new commits which add the aforementioned mempool enforcement semantics and updated the PR description accordingly.

[JoeGruffins]

The commit message in b979fd1 mentions renaming a test function but looks cut off.

[davecgh]

The commit message in b979fd1 mentions renaming a test function but looks cut off.

That is actually in #2721 where it has already been updated.

EDIT: I rebased over it now, so the correct message should be incorporated.

[matheusd]

Tested several combinations of the standard two-node simnet setup using current master/this PR applied/this PR + non-voted-agenda (to test an upgraded node while the agenda hasn't activated yet).

In particular, I tested scenarios involving sending (via rpc) and relaying txs across nodes running different versions, using tx version > 3 and script version > 0 (and spending from them). All scenarios worked as expected.

All in all, looking pretty good :P

I'll probably do a second pass review in a couple of days after other ppl have had a chance to also look at this, but it seems pretty close to ready.

[rstaudt2]

This looks great. I can definitely appreciate the benefits this will bring after working on revocation transaction changes for DCP0009. I’ll do another pass over it in the next few days in addition to some testing, but so far everything looks proper to me.

[rstaudt2]

Thinking through the process of a future consensus change once this is in place:

• Let's say we are introducing a consensus change that makes modifications to vote transactions
• With the explicit upgrades agenda active, we would know that transactions with a version greater than 3 are not being accepted
• Therefore, we would want to bump the vote transaction version from 1 to 4, and maxAllowedTxVer would be set to 4 here if this new agenda to change vote transactions was active
• This would allow us to have this single agenda check here, and most other transaction-level checks could rely on checking that the vote transaction version is 4 to apply the new rules (which will be super nice!)

A couple of things felt a bit odd to me about this at first:

• Even though in the above example the vote transaction version is bumped to 4, other transaction types will still actively use version 1 - 3. And for those transaction types, version 4 is also now valid, even though it may not be used and does not stipulate any changes to those transaction types.
• The version always needs to be bumped from wherever it is at to beyond the upper limit to take advantage of this (e.g. in the example above vote transactions skip from version 1 to 4, even though there are no changes in any of the intermediate versions for vote transactions).

This effectively makes the max transaction version really just an update to the overall rule set for transactions, which will typically only be set on the transaction types that were impacted by the rule updates. That makes sense, and I think capturing these updates here in a single place for any new agendas will make it clear and easy to follow.

No suggested changes here just capturing my thought process to make sure I’m not missing anything and in case it is helpful for anyone else reviewing!

[davecgh]

I should note that I did consider applying max versions individually, but I decided against that because stake transactions are themselves just transactions that must be of a very specific form to be categorized as that "type" of transaction. Due to that, any changes that affect non-stake transactions will also affect stake transactions (most of the time anyway), so, having an overall rule set for transactions (which encompasses both stake and non-stake) makes the most sense.

[rstaudt2]

I double-checked as well that the script version is being enforced for all stake transaction types.

[davecgh]

For reference, I added a separate prep commit that refactors the construction logic of the flags to use when checking transactions based on the active agendas into a single func and updated the code to use those flags instead of the local agenda determination funcs. That way, adding the new agenda in one place automatically applies it to all. The subsequent commits have been updated accordingly.

The net effect is that now all places where the transaction check flags are constructed in blockchain ensure all of the active agendas are set and thus all notifications will also indicate all active agendas regardless of the path leading up to the notification.

[rstaudt2]

This looks good to go to me! I did another pass of reviewing and testing and only noticed a few comment typos.

[dnldd]

Nice validation tests, all issues look to have been addressed as well.