multi: Cleanup and optimize tx input check code. by davecgh · Pull Request #1468 · decred/dcrd

davecgh · 2018-09-22T00:26:53Z

This optimizes and cleans up significant portions of the CheckTransactionInputs function to avoid a lot extra allocations, remove redundant checks, better document its semantics, and make the code easier to reason about. In addition, it renames a lot of the error constants involved in said function to use the ticket/vote/revocation terminology and improve their readability. Even though several of the checks have been rearranged for efficiency and readability purposes, all consensus semantics have been retained.

One of the primary changes is to reduce the reliance on the stake package for consensus validation. Not only is more desirable to have the bulk of the validation related to the blockchain in the blockchain package, but it also allows the code to be more specific, which enables better optimization opportunities as well as eliminates the need for a lot of redundant checks that are simply unnecessary.

The stake identification functions are also part of consensus and have not been modified, however, the actual validation related to all of their inputs, such as ensuring commitments are observed, are now in the
blockchain package itself.

Since the only thing using the related verification functions is now done in blockchain, this also removes the VerifyStakingPkhsAndAmounts, VerifySStxAmounts, and related tests from the stake package.

Another significant change is the addition of new functions for efficiently and specifically identifying the form of the scripts required by stake transactions in order to reduce the dependence on the standard code in txscript. Standard code should NOT be used in consensus code as the two are not the same thing.

It should be noted that these changes break compatibility with the current v1 blockchain and stake modules, so they will need a major version bump prior to the next release.

Finally, all tests in the repository have been updated for the error name changes as well as change in expected error in some cases due to the reordering of the validation checks.

This is work towards #1145.

dnldd · 2018-09-23T13:52:30Z

+	return extractPubKeyHash(script) != nil
+}
+
+// extractPubKeyHash extracts a script hash that is being paid from the passed


s/extractPubKeyHash/extractScriptHash

dnldd · 2018-09-23T14:10:09Z

-	ErrSSGenPayeeOuts
+	// ErrBadPayeeScriptVersion indicates that either a vote or revocation
+	// transaction output that corresponds to a ticket commitment does not have
+	// use a supported script version.


... does not have use a supported script version. should be ... does not use a supported script version.

dnldd · 2018-09-23T14:15:19Z

+	// ErrBadPayeeScriptType indicates that either a vote or revocation
+	// transaction output that corresponds to a ticket commitment does not pay
+	// to the same script type required by the commitment.
+	ErrMismatchedPayeeHash


ErrMismatchedPayeeHash currently has ErrBadPayeeScriptType's description.

dnldd · 2018-09-23T14:16:09Z

+	// to the same script type required by the commitment.
+	ErrMismatchedPayeeHash
+
+	// ErrBadPayeeValue indicates the either a vote or revocation transaction


dnldd · 2018-09-23T14:26:09Z

-	// ErrTxSStxOutSpend indicates that a non SSGen or SSRtx tx attempted to spend
-	// an OP_SSTX tagged output from an SStx.
+	// ErrTxSStxOutSpend indicates that a non SSGen or SSRtx tx attempted to
+	// spend an OP_SSTX tagged output from an SStx.
 	ErrTxSStxOutSpend


Perhaps ErrTxSStxOutSpend can now become ErrTxTicketOutSpend ?

Rename SStx

See previous comment. I agree with the rename, but I don't think it should be done in this commit since it's not directly used by the code being modified.

matheusd

Tried as much as possible to figure out whether there were any changes breaking consensus and couldn't find any.

Added a few points for further clarification.

Also performed a functional test in simnet with a mix of nodes using the current master and this PR applied and didn't hit any snags.

matheusd · 2018-09-24T11:06:29Z


 	// ErrSSRtxPayees indicates that the SSRtx failed to pay out to the committed
 	// addresses or amounts from the originating SStx.
 	ErrSSRtxPayees

-	// ErrTxSStxOutSpend indicates that a non SSGen or SSRtx tx attempted to spend
-	// an OP_SSTX tagged output from an SStx.
+	// ErrTxSStxOutSpend indicates that a non SSGen or SSRtx tx attempted to


Rename SSGen and SSRtx

This error is not used by anything being touched in these changes, so I wanted to leave that for another commit. I only touched the comment because I noticed it was not following coding style. I should probably revert the change for this commit and leave it for another that does a full blown rename of errors.

matheusd · 2018-09-24T11:06:54Z

-	// ErrTxSStxOutSpend indicates that a non SSGen or SSRtx tx attempted to spend
-	// an OP_SSTX tagged output from an SStx.
+	// ErrTxSStxOutSpend indicates that a non SSGen or SSRtx tx attempted to
+	// spend an OP_SSTX tagged output from an SStx.
 	ErrTxSStxOutSpend


Rename SStx

matheusd · 2018-09-24T11:35:25Z

+	// Assert there are two outputs for each input to the ticket as well as the
+	// additional voting rights output.
+	if (len(msgTx.TxIn)*2 + 1) != len(msgTx.TxOut) {
+		panicf("attempt to check ticket purchase inputs on tx %s which does "+


Why is this a panic instead of an error (possibly an ErrTicketCommitment or something else)? I get that this ordinarily should never be exercised, given that checkTicketPurchaseInputs explicitly notes that it should only be called on transactions previously determined to be ticket purchases (which implicitly implies this check has been performed).

So given the current requirement for checkTicketPruchaseInputs, this seems like a redundant check. Do you plan on removing the requirement that isSSTx be performed prior to calling this? Or is this meant more as a smoke test?

It's a smoke test to assert the requirement in the comment that this function is only called with a transaction that has already been determined to be a ticket purchase in order to help prevent potential misuse of the function in the future.

Calling this function without this condition holding is a logic error and could ultimately lead to many insidious issues such as database "corruption" in terms of improper inclusion of a transaction as if it were a ticket when it's not that wouldn't be detected until the "ticket" finally got called to vote way too late. Also, returning an error here would just lead to it being rejected, which could cause an unexpected fork in consensus due to a logic bug. Instead, the panic just takes the node down since there is an internal logic bug.

It is especially salient because this assumption depends on the stake identifications functions which live in a different module, so it isn't too hard to imagine that being changed by someone who missed this assumption.

You are correct to note that it is slightly redundant when called properly, but it's a very fast assertion that helps ensure the assumptions are valid. Since consensus is so important, and mistakes in consensus can have disastrous consequences, I have been slowly adding more quick assertions like this throughout to try and help prevent future changes from inadvertently making breaking changes.

matheusd · 2018-09-24T11:39:31Z

+			msgTx.TxHash(), len(msgTx.TxIn), len(msgTx.TxOut))
+	}
+
+	for txInIdx := 0; txInIdx < len(msgTx.TxIn); txInIdx++ {


Just curious, since this seems more prevalent in the code in validate.go: why do you prefer the for i:= 0; i < len(); i++ vs the for i, in := range ... construct?

The range statement in Go is rather wonky. It has non-obvious semantics and often leads to issues, because of the way it behaves. For example, I'm sure you'll notice in some places that copies of things are made with comments about preventing issues that are a direct result of the terrible range semantics.

Ultimately, I would like to change the arrays of most things from being arrays of pointers to elements to arrays of elements themselves to significantly improve cache locality. However, the range statement copies the entire element in that case, and that is, in fact, the original primary driving reason why most of the existing data structures ended up being []*Foo instead of []Foo so they could be used with the range statement and behave as one would expect. I personally don't think the use of range justifies significantly slower code.

matheusd · 2018-09-24T12:17:06Z

+//
+// The vote subsidy must be 0 for revocations since, unlike votes, they do not
+// produce any additional subsidy.
+func calcReturnAmount(contribAmount, ticketPurchaseAmount, voteSubsidy int64, contributionSumBig *big.Int) int64 {


Very small suggestion: I'd name this calcStakeReturnAmount (or something along those lines) to make it obvious this is related to the stake proportional return as opposed to some generic return amount. This was previously calculated in stake.CalculateRewards which more clearly showed this is a reward related to staking.

Good suggestion. Renamed to calcTicketReturnAmount.

matheusd · 2018-09-24T13:29:40Z

+		// tagged outputs are votes and revocations.  So, check all the inputs
+		// from non votes and revocations and make sure that they spend no
+		// OP_SSTX tagged outputs.
+		if !(isVote || isRevocation) {
 			if txscript.GetScriptClass(


Do we want to eventually remove this (and the next) GetScriptClass and replace by more specific checks as well?

Yeah, I think so. I might look at that before merging this one, but it already has a lot of testing on it, so it might make sense to do that in a separate PR.

dajohi

ok

This optimizes and cleans up significant portions of the CheckTransactionInputs function to avoid a lot extra allocations, remove redundant checks, better document its semantics, and make the code easier to reason about. In addition, it renames a lot of the error constants involved in said function to use the ticket/vote/revocation terminology and improve their readability. Even though several of the checks have been rearranged for efficiency and readability purposes, all consensus semantics have been retained. One of the primary changes is to reduce the reliance on the stake package for consensus validation. Not only is more desirable to have the bulk of the validation related to the blockchain in the blockchain package, but it also allows the code to be more specific, which enables better optimization opportunities as well as eliminates the need for a lot of redundant checks that are simply unnecessary. The stake identification functions are also part of consensus and have not been modified, however, the actual validation related to all of their inputs, such as ensuring commitments are observed, are now in the blockchain package itself. Since the only thing using the related verification functions is now done in blockchain, this also removes the VerifyStakingPkhsAndAmounts, VerifySStxAmounts, and related tests from the stake package. Another significant change is the addition of new functions for efficiently and specifically identifying the form of the scripts required by stake transactions in order to reduce the dependence on the standard code in txscript. Standard code should _NOT_ be used in consensus code as the two are not the same thing. It should be noted that these changes break compatibility with the current v1 blockchain and stake modules, so they will need a major version bump prior to the next release. Finally, all tests in the repository have been updated for the error name changes as well as change in expected error in some cases due to the reordering of the validation checks.

davecgh added this to the 1.4.0 milestone Sep 22, 2018

davecgh mentioned this pull request Sep 22, 2018

Multi-peer Checklist #1145

Open

29 of 33 tasks

dnldd reviewed Sep 23, 2018

View reviewed changes

matheusd reviewed Sep 24, 2018

View reviewed changes

davecgh force-pushed the blockchain_checktxinputs_cleanup branch 2 times, most recently from fb125e8 to 8026a2b Compare September 24, 2018 21:04

matheusd approved these changes Sep 25, 2018

View reviewed changes

dajohi approved these changes Sep 26, 2018

View reviewed changes

davecgh force-pushed the blockchain_checktxinputs_cleanup branch from 8026a2b to 42bc847 Compare September 26, 2018 14:23

davecgh merged commit 42bc847 into decred:master Sep 26, 2018

davecgh deleted the blockchain_checktxinputs_cleanup branch September 26, 2018 15:20

dnldd mentioned this pull request Jul 10, 2020

Block validation rules impossible error paths refactor #1182

Open

matthawkins90 mentioned this pull request Aug 30, 2026

fullblocktests: Add ErrInvalidRevokeInput test. #3786

Merged

This was referenced Sep 13, 2026

fullblocktests: Add ErrTicketInputScript tests. #3796

Merged

blockchain: Test immature ticket spend. #3797

Merged

blockchain: Test invalid ticket submission input. #3800

Merged

Conversation

davecgh commented Sep 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matheusd left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dajohi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

davecgh commented Sep 22, 2018 •

edited

Loading

matheusd left a comment •

edited

Loading