From 37315c030bef937f34960bf7e4f3169b36b3cf81 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Thu, 13 Sep 2018 01:00:18 -0500 Subject: [PATCH] blockchain: Ensure no stake opcodes in tx sanity. This moves the check for validating that output scripts in non-stake transactions do not contain any of the opcodes only allowed in the stake tree to the CheckTransactionSanity function where it more naturally belongs since it does not require access to transaction inputs as its location in CheckTransactionInputs would otherwise indicate. It should be noted that the check just before the one being moved in this change is removed because it checked that specific patterns involving stake opcodes where not being used in non-stake transactions which is a strict subset of the more general check which ensures there are no stake opcodes at all in the output scripts. Also, to improve efficiency, the check is added to the existing loop which already iterates the outputs and the entire block is moved after some other faster checks. Finally, since the transaction sanity code previously did not determine if the transaction is a stake transaction which is required for the check in question, this adds code to relatively efficiently determine that. --- blockchain/fullblocktests/generate.go | 6 +- blockchain/validate.go | 140 ++++++++++++-------------- 2 files changed, 65 insertions(+), 81 deletions(-) diff --git a/blockchain/fullblocktests/generate.go b/blockchain/fullblocktests/generate.go index eadc1d584c..351798a53f 100644 --- a/blockchain/fullblocktests/generate.go +++ b/blockchain/fullblocktests/generate.go @@ -2176,7 +2176,7 @@ func Generate(includeLargeReorg bool) (tests [][]TestInstance, err error) { b.AddSTransaction(ticket) b.Header.FreshStake++ }) - rejected(blockchain.ErrRegTxInStakeTree) + rejected(blockchain.ErrRegTxCreateStakeOut) // Create block with scripts that do not involve p2pkh or // p2sh addresses for a ticket purchase. @@ -2193,7 +2193,7 @@ func Generate(includeLargeReorg bool) (tests [][]TestInstance, err error) { b.AddSTransaction(ticket) b.Header.FreshStake++ }) - rejected(blockchain.ErrRegTxInStakeTree) + rejected(blockchain.ErrRegTxCreateStakeOut) // --------------------------------------------------------------------- // Block header median time tests. @@ -2708,7 +2708,7 @@ func Generate(includeLargeReorg bool) (tests [][]TestInstance, err error) { g.NextBlock("bsc2", outs[19], ticketOuts[19], replaceSpendScript(tooManySigOps)) g.AssertTipBlockSigOpsCount(maxBlockSigOps + 1) - rejected(blockchain.ErrTooManySigOps) + rejected(blockchain.ErrScriptMalformed) // --------------------------------------------------------------------- // Dead execution path tests. diff --git a/blockchain/validate.go b/blockchain/validate.go index a2ee380ea4..cfdc2a4d0a 100644 --- a/blockchain/validate.go +++ b/blockchain/validate.go @@ -197,57 +197,17 @@ func CheckTransactionSanity(tx *wire.MsgTx, params *chaincfg.Params) error { return ruleError(ErrTxTooBig, str) } - // Ensure the transaction amounts are in range. Each transaction - // output must not be negative or more than the max allowed per - // transaction. Also, the total of all outputs must abide by the same - // restrictions. All amounts in a transaction are in a unit value - // known as an atom. One Decred is a quantity of atoms as defined by - // the AtomsPerCoin constant. - var totalAtom int64 - for _, txOut := range tx.TxOut { - atom := txOut.Value - if atom < 0 { - str := fmt.Sprintf("transaction output has negative "+ - "value of %v", atom) - return ruleError(ErrBadTxOutValue, str) - } - if atom > dcrutil.MaxAmount { - str := fmt.Sprintf("transaction output value of %v is "+ - "higher than max allowed value of %v", atom, - dcrutil.MaxAmount) - return ruleError(ErrBadTxOutValue, str) - } - - // Two's complement int64 overflow guarantees that any overflow - // is detected and reported. This is impossible for Decred, - // but perhaps possible if an alt increases the total money - // supply. - totalAtom += atom - if totalAtom < 0 { - str := fmt.Sprintf("total value of all transaction "+ - "outputs exceeds max allowed value of %v", - dcrutil.MaxAmount) - return ruleError(ErrBadTxOutValue, str) - } - if totalAtom > dcrutil.MaxAmount { - str := fmt.Sprintf("total value of all transaction "+ - "outputs is %v which is higher than max "+ - "allowed value of %v", totalAtom, - dcrutil.MaxAmount) - return ruleError(ErrBadTxOutValue, str) - } - } - // Coinbase script length must be between min and max length. + isVote := stake.IsSSGen(tx) if IsCoinBaseTx(tx) { - // The referenced outpoint should be null. + // The referenced outpoint must be null. if !isNullOutpoint(&tx.TxIn[0].PreviousOutPoint) { str := fmt.Sprintf("coinbase transaction did not use " + "a null outpoint") return ruleError(ErrBadCoinbaseOutpoint, str) } - // The fraud proof should also be null. + // The fraud proof must also be null. if !isNullFraudProof(tx.TxIn[0]) { str := fmt.Sprintf("coinbase transaction fraud proof " + "was non-null") @@ -262,7 +222,7 @@ func CheckTransactionSanity(tx *wire.MsgTx, params *chaincfg.Params) error { MaxCoinbaseScriptLen) return ruleError(ErrBadCoinbaseScriptLen, str) } - } else if stake.IsSSGen(tx) { + } else if isVote { // Check script length of stake base signature. slen := len(tx.TxIn[0].SignatureScript) if slen < MinCoinbaseScriptLen || slen > MaxCoinbaseScriptLen { @@ -293,7 +253,7 @@ func CheckTransactionSanity(tx *wire.MsgTx, params *chaincfg.Params) error { } else { // Previous transaction outputs referenced by the inputs to // this transaction must not be null except in the case of - // stake bases for SSGen tx. + // stakebases for votes. for _, txIn := range tx.TxIn { prevOut := &txIn.PreviousOutPoint if isNullOutpoint(prevOut) { @@ -304,6 +264,62 @@ func CheckTransactionSanity(tx *wire.MsgTx, params *chaincfg.Params) error { } } + // Ensure the transaction amounts are in range. Each transaction output + // must not be negative or more than the max allowed per transaction. Also, + // the total of all outputs must abide by the same restrictions. All + // amounts in a transaction are in a unit value known as an atom. One + // Decred is a quantity of atoms as defined by the AtomsPerCoin constant. + // + // Also ensure that non-stake transaction output scripts do not contain any + // stake opcodes. + isTicket := !isVote && stake.IsSStx(tx) + isRevocation := !isVote && !isTicket && stake.IsSSRtx(tx) + isStakeTx := isVote || isTicket || isRevocation + var totalAtom int64 + for txOutIdx, txOut := range tx.TxOut { + atom := txOut.Value + if atom < 0 { + str := fmt.Sprintf("transaction output has negative value of %v", + atom) + return ruleError(ErrBadTxOutValue, str) + } + if atom > dcrutil.MaxAmount { + str := fmt.Sprintf("transaction output value of %v is higher than "+ + "max allowed value of %v", atom, dcrutil.MaxAmount) + return ruleError(ErrBadTxOutValue, str) + } + + // Two's complement int64 overflow guarantees that any overflow is + // detected and reported. This is impossible for Decred, but perhaps + // possible if an alt increases the total money supply. + totalAtom += atom + if totalAtom < 0 { + str := fmt.Sprintf("total value of all transaction outputs "+ + "exceeds max allowed value of %v", dcrutil.MaxAmount) + return ruleError(ErrBadTxOutValue, str) + } + if totalAtom > dcrutil.MaxAmount { + str := fmt.Sprintf("total value of all transaction outputs is %v "+ + "which is higher than max allowed value of %v", totalAtom, + dcrutil.MaxAmount) + return ruleError(ErrBadTxOutValue, str) + } + + // Ensure that non-stake transactions have no outputs with opcodes + // OP_SSTX, OP_SSRTX, OP_SSGEN, or OP_SSTX_CHANGE. + if !isStakeTx { + hasOp, err := txscript.ContainsStakeOpCodes(txOut.PkScript) + if err != nil { + return ruleError(ErrScriptMalformed, err.Error()) + } + if hasOp { + str := fmt.Sprintf("non-stake transaction output %d contains "+ + "stake opcode", txOutIdx) + return ruleError(ErrRegTxCreateStakeOut, str) + } + } + } + // Check for duplicate transaction inputs. existingTxOut := make(map[wire.OutPoint]struct{}) for _, txIn := range tx.TxIn { @@ -1812,40 +1828,8 @@ func CheckTransactionInputs(subsidyCache *SubsidyCache, tx *dcrutil.Tx, txHeight // to ignore overflow and out of range errors here because those error // conditions would have already been caught by checkTransactionSanity. var totalAtomOut int64 - for i, txOut := range tx.MsgTx().TxOut { + for _, txOut := range tx.MsgTx().TxOut { totalAtomOut += txOut.Value - - // Double check and make sure that, if this is not a stake - // transaction, that no outputs have OP code tags OP_SSTX, - // OP_SSRTX, OP_SSGEN, or OP_SSTX_CHANGE. - if !isSStx && !isSSGen && !isSSRtx { - scriptClass := txscript.GetScriptClass(txOut.Version, txOut.PkScript) - if (scriptClass == txscript.StakeSubmissionTy) || - (scriptClass == txscript.StakeGenTy) || - (scriptClass == txscript.StakeRevocationTy) || - (scriptClass == txscript.StakeSubChangeTy) { - errStr := fmt.Sprintf("Non-stake tx %v "+ - "included stake output type %v at in "+ - "txout at position %v", txHash, - scriptClass, i) - return 0, ruleError(ErrRegTxCreateStakeOut, errStr) - } - - // Check to make sure that non-stake transactions also - // are not using stake tagging OP codes anywhere else - // in their output pkScripts. - op, err := txscript.ContainsStakeOpCodes(txOut.PkScript) - if err != nil { - return 0, ruleError(ErrScriptMalformed, - err.Error()) - } - if op { - errStr := fmt.Sprintf("Non-stake tx %v "+ - "included stake OP code in txout at "+ - "position %v", txHash, i) - return 0, ruleError(ErrScriptMalformed, errStr) - } - } } // Ensure the transaction does not spend more than its inputs.