From c9de6b067e570b6923b0eb95385e53699c52d620 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Wed, 6 May 2026 01:18:38 -0500 Subject: [PATCH 01/10] blockchain: Cleanup proof of stake checks. This performs some light cleanup of the checkProofOfStake function to make it more consistent with the rest of the code make the error messages more accurate and useful. --- internal/blockchain/validate.go | 48 ++++++++++++++++----------------- 1 file changed, 23 insertions(+), 25 deletions(-) diff --git a/internal/blockchain/validate.go b/internal/blockchain/validate.go index 216377cb9..cca5ad7a8 100644 --- a/internal/blockchain/validate.go +++ b/internal/blockchain/validate.go @@ -629,32 +629,30 @@ func CheckTransaction(tx *wire.MsgTx, params *chaincfg.Params, flags AgendaFlags // checkProofOfStake ensures that all ticket purchases in the block pay at least // the amount required by the block header stake bits which indicate the target // stake difficulty (aka ticket price) as claimed. -func checkProofOfStake(block *dcrutil.Block, posLimit int64) error { - msgBlock := block.MsgBlock() - for _, staketx := range block.STransactions() { - msgTx := staketx.MsgTx() - if stake.IsSStx(msgTx) { - commitValue := msgTx.TxOut[0].Value - - // Check for underflow block sbits. - if commitValue < msgBlock.Header.SBits { - errStr := fmt.Sprintf("Stake tx %v has a "+ - "commitment value less than the "+ - "minimum stake difficulty specified in"+ - " the block (%v)", staketx.Hash(), - msgBlock.Header.SBits) - return ruleError(ErrNotEnoughStake, errStr) - } +func checkProofOfStake(block *dcrutil.Block, minStakeDiff int64) error { + header := &block.MsgBlock().Header + for _, stx := range block.STransactions() { + msgTx := stx.MsgTx() + if !stake.IsSStx(msgTx) { + continue + } - // Check if it's above the PoS limit. - if commitValue < posLimit { - errStr := fmt.Sprintf("Stake tx %v has a "+ - "commitment value less than the "+ - "minimum stake difficulty for the "+ - "network (%v)", staketx.Hash(), - posLimit) - return ruleError(ErrStakeBelowMinimum, errStr) - } + // The ticket price must not be below the stake difficulty claimed by + // the block header. + ticketPaidAmt := msgTx.TxOut[submissionOutputIdx].Value + if ticketPaidAmt < header.SBits { + str := fmt.Sprintf("ticket %v pays %v below the stake difficulty "+ + "%v committed to by the block header", stx.Hash(), + ticketPaidAmt, header.SBits) + return ruleError(ErrNotEnoughStake, str) + } + + // The ticket price must not be below the network minimum. + if ticketPaidAmt < minStakeDiff { + str := fmt.Sprintf("ticket %v pays %v below the network minimum "+ + "stake difficulty of %v", stx.Hash(), ticketPaidAmt, + minStakeDiff) + return ruleError(ErrStakeBelowMinimum, str) } } From 7ef641bde08c541233be25ce6d04e40970338ec9 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Wed, 6 May 2026 07:07:30 -0500 Subject: [PATCH 02/10] blockchain: New add funcs with overflow detection. Currently, all overflow detection is done inline in multiple places throughout the blockchain code. It would be more ergonomic, consistent, and less error prone to instead use well-tested funcs dedicated to that purpose. To pave the way, this adds two new functions for adding arguments with a returned flag that indicates whether the result is safe to use (that is no overflow or underflow occurred). One variant is for unsigned ints (uint16, uint32, and uint64) and the other is for signed ints (int16, int32, and int64). It only introduces the funcs and does not modify any code to use them. --- internal/blockchain/checkedmath.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 internal/blockchain/checkedmath.go diff --git a/internal/blockchain/checkedmath.go b/internal/blockchain/checkedmath.go new file mode 100644 index 000000000..99faf9e78 --- /dev/null +++ b/internal/blockchain/checkedmath.go @@ -0,0 +1,24 @@ +// Copyright (c) 2026 The Decred developers +// Use of this source code is governed by an ISC +// license that can be found in the LICENSE file. + +package blockchain + +// addUnsigned returns the sum of the two unsigned ints of the same size and +// whether or not the result is safe to use (aka no overflow occurred). +func addUnsigned[T ~uint16 | ~uint32 | ~uint64](a, b T) (T, bool) { + sum := a + b + return sum, sum >= a +} + +// addSigned returns the sum of the two signed ints of the same size and whether +// or not the result is safe to use (aka no overflow or underflow occurred). +func addSigned[T ~int16 | ~int32 | ~int64](a, b T) (T, bool) { + // Overflow only occurs when adding a positive value when the sum is <= to + // left summand. Likewise, underflow only occurs when adding a non-positive + // value when the sum is > the left summand. The following is the logical + // negation of the result of testing both conditions at once so the returned + // flag indicates their absence. + sum := a + b + return sum, (sum > a) == (b > 0) +} From 41b06f646335f6d01b0bf8a46c2921ad29c80b8f Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Wed, 6 May 2026 07:07:31 -0500 Subject: [PATCH 03/10] blockchain: Add tests for new add funcs. This adds comprehensive tests for the new addUnsigned and addSigned funcs for all supported types. --- internal/blockchain/checkedmath_test.go | 130 ++++++++++++++++++++++++ 1 file changed, 130 insertions(+) create mode 100644 internal/blockchain/checkedmath_test.go diff --git a/internal/blockchain/checkedmath_test.go b/internal/blockchain/checkedmath_test.go new file mode 100644 index 000000000..7ce7f81c6 --- /dev/null +++ b/internal/blockchain/checkedmath_test.go @@ -0,0 +1,130 @@ +// Copyright (c) 2026 The Decred developers +// Use of this source code is governed by an ISC +// license that can be found in the LICENSE file. + +package blockchain + +import ( + "testing" +) + +// testAddUnsigned ensures [addUnsigned] produces the expected results for the +// given supported unsigned type. It uses generics to avoid repeating the tests +// for each type. +func testAddUnsigned[T uint16 | uint32 | uint64](t *testing.T, typ string) { + t.Helper() + + var maxVal = ^T(0) + tests := []struct { + name string // test description + a, b T // unsigned vals to test + sum T // expected sum + ok bool // expected result + }{ + // No overflow cases. + {"zero", 0, 0, 0, true}, + {"max + zero", maxVal, 0, maxVal, true}, + {"zero + max", 0, maxVal, maxVal, true}, + {"small positive", 10, 20, 30, true}, + {"max edge", maxVal - 1, 1, maxVal, true}, + {"max edge rev", 1, maxVal - 1, maxVal, true}, + {"halfmax + halfmax", maxVal / 2, maxVal / 2, maxVal - 1, true}, + {"mid + halfmax", maxVal/2 + 1, maxVal / 2, maxVal, true}, + {"halfmax + mid", maxVal / 2, maxVal/2 + 1, maxVal, true}, + + // Overflow cases. + {"max + 1 overflow exact", maxVal, 1, 0, false}, + {"1 + max overflow exact", 1, maxVal, 0, false}, + {"small overflow", maxVal - 5, 6, 0, false}, + {"small overflow rev", 6, maxVal - 5, 0, false}, + {"mid overflow", maxVal/2 + 1, maxVal/2 + 1, 0, false}, + {"mid + max overflow", maxVal/2 + 1, maxVal, maxVal / 2, false}, + {"max + mid overflow", maxVal, maxVal/2 + 1, maxVal / 2, false}, + } + + for _, test := range tests { + sum, ok := addUnsigned(test.a, test.b) + if sum != test.sum || ok != test.ok { + t.Errorf("%q (%s): unexpected result - got (%v, %v), want (%v, %v)", + test.name, typ, sum, ok, test.sum, test.ok) + } + } +} + +// TestAddUnsigned ensures [addUnsigned] produces the expected results for all +// three supported unsigned int types (uint16, uint32, uint64). +func TestAddUnsigned(t *testing.T) { + testAddUnsigned[uint16](t, "uint16") + testAddUnsigned[uint32](t, "uint32") + testAddUnsigned[uint64](t, "uint64") +} + +// testAddSigned ensures [addSigned] produces the expected results for the given +// supported signed type. It uses generics to avoid repeating the tests for +// each type. +func testAddSigned[T int16 | int32 | int64](t *testing.T, typ string, maxVal T) { + t.Helper() + + minVal := ^maxVal + tests := []struct { + name string // test description + a, b T // signed vals to test + sum T // expected sum + ok bool // expected result + }{ + // No overflow or underflow cases. + {"zero", 0, 0, 0, true}, + {"min + zero", minVal, 0, minVal, true}, + {"zero + min", 0, minVal, minVal, true}, + {"max + zero", maxVal, 0, maxVal, true}, + {"zero + max", 0, maxVal, maxVal, true}, + {"small positive", 10, 20, 30, true}, + {"small negative", -10, -20, -30, true}, + {"mixed small", 100, -50, 50, true}, + {"mixed small rev", -100, 50, -50, true}, + {"min edge", minVal + 1, -1, minVal, true}, + {"max edge", maxVal - 1, 1, maxVal, true}, + {"min + max", minVal, maxVal, -1, true}, + {"max + min", maxVal, minVal, -1, true}, + {"halfmin + halfmin", minVal / 2, minVal / 2, minVal, true}, + {"mid + min", maxVal/2 + 1, minVal, minVal / 2, true}, + {"min + mid", minVal, maxVal/2 + 1, minVal / 2, true}, + + // Overflow cases. + {"max + 1 overflow exact", maxVal, 1, minVal, false}, + {"1 + max overflow exact", 1, maxVal, minVal, false}, + {"small overflow", maxVal - 5, 6, minVal, false}, + {"small overflow rev", 6, maxVal - 5, minVal, false}, + {"pos to neg mid overflow", maxVal/2 + 1, maxVal/2 + 1, minVal, false}, + {"mid + max overflow", maxVal/2 + 1, maxVal, minVal/2 - 1, false}, + {"max + mid overflow", maxVal, maxVal/2 + 1, minVal/2 - 1, false}, + {"max + max overflow", maxVal, maxVal, -2, false}, + + // Underflow cases. + {"min + (-1) underflow exact", minVal, -1, maxVal, false}, + {"-1 + min underflow exact", -1, minVal, maxVal, false}, + {"small underflow", minVal + 5, -6, maxVal, false}, + {"small underflow rev", -6, minVal + 5, maxVal, false}, + {"neg to pos mid underflow", minVal/2 - 1, minVal / 2, maxVal, false}, + {"neg to pos mid underflow rev", minVal / 2, minVal/2 - 1, maxVal, false}, + {"halfmin + min underflow", minVal / 2, minVal, maxVal/2 + 1, false}, + {"min + halfmin underflow", minVal, minVal / 2, maxVal/2 + 1, false}, + {"min + min underflow", minVal, minVal, 0, false}, + } + + for _, test := range tests { + sum, ok := addSigned(test.a, test.b) + if sum != test.sum || ok != test.ok { + t.Errorf("%q (%s): unexpected result - got (%v, %v), want (%v, %v)", + test.name, typ, sum, ok, test.sum, test.ok) + } + } +} + +// TestAddSigned ensures [addSigned] produces the expected results for all three +// supported signed int types (int16, int32, int64). +func TestAddSigned(t *testing.T) { + testAddSigned[int16](t, "int16", 1<<15-1) + testAddSigned[int32](t, "int32", 1<<31-1) + testAddSigned[int64](t, "int64", 1<<63-1) +} From fe46e2996cee5cd15f3ffd7bcc58e09616d7a8d2 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Wed, 6 May 2026 07:07:32 -0500 Subject: [PATCH 04/10] multi: Unsigned sigop counts and cleaner overflow. Consensus code should ideally always use types of a specific size so there is no possibility of divergent behavior when compiled on different architectures due to differing upper bounds. Further, unsigned types for values that can never be negative should always be preferred. The current signature operations counting code does not adhere to that practice and uses plain ints which means the maximum possible value technically changes depending on the architecture. While this is not currently an issue due to a combination of various limits making it impossible to get anywhere near the limits of the smallest supported architecture (32-bit) and overflow detection, these types of hidden assumptions can easily lead to bugs over time as new features are introduced. This remedies that by modifying the consensus level signature operation counting to use an fixed unsigned 32-bit integer instead of an architecture dependent signed integer. Ideally, the return types on the underlying counting funcs in the script engine would be updated to match and avoid the need to cast, but that would require a major API version bump since it is a public module, so this limits the changes to the internal blockchain, mempool, and mining packages. While here, it also switches to the new consolidated add funcs with cleaner overflow detection versus the current more ad-hoc inline detection. Note that there is no risk of consensus divergence due to the aforementioned impossibility of hitting the conditions with the current combination of parameters and limits. --- internal/blockchain/validate.go | 221 ++++++++++++++----------- internal/mempool/mempool.go | 21 +-- internal/mining/mining.go | 63 ++++--- internal/mining/mining_harness_test.go | 20 +-- internal/mining/mining_view_test.go | 2 +- server.go | 2 +- 6 files changed, 177 insertions(+), 152 deletions(-) diff --git a/internal/blockchain/validate.go b/internal/blockchain/validate.go index cca5ad7a8..31e52a791 100644 --- a/internal/blockchain/validate.go +++ b/internal/blockchain/validate.go @@ -2161,9 +2161,9 @@ func (b *BlockChain) checkBlockContext(block *dcrutil.Block, prevNode *blockNode return ruleError(ErrRevocationsMismatch, str) } - // The number of signature operations must be less than the maximum allowed - // per block. - totalSigOps := 0 + // The number of signature operations must not overflow the accumulator and + // be less than the maximum allowed per block. + var totalSigOps uint32 regularTxns := block.Transactions() stakeTxns := block.STransactions() allTxns := make([]*dcrutil.Tx, 0, len(regularTxns)+len(stakeTxns)) @@ -2172,12 +2172,21 @@ func (b *BlockChain) checkBlockContext(block *dcrutil.Block, prevNode *blockNode for _, tx := range allTxns { msgTx := tx.MsgTx() - // We could potentially overflow the accumulator so check for overflow. - lastSigOps := totalSigOps isCoinBase := standalone.IsCoinBaseTx(msgTx, isTreasuryEnabled) isSSGen := stake.IsSSGen(msgTx) - totalSigOps += CountSigOps(tx, isCoinBase, isSSGen, isTreasuryEnabled) - if totalSigOps < lastSigOps || totalSigOps > MaxSigOpsPerBlock { + numSigOps, err := countSigOps(tx, isCoinBase, isSSGen, isTreasuryEnabled) + if err != nil { + return err + } + + var ok bool + totalSigOps, ok = addUnsigned(totalSigOps, numSigOps) + if !ok { + str := fmt.Sprintf("tx %v causes block signature operation count "+ + "to overflow", tx.Hash()) + return ruleError(ErrTooManySigOps, str) + } + if totalSigOps > MaxSigOpsPerBlock { str := fmt.Sprintf("block contains too many signature operations "+ "- got %v, max %v", totalSigOps, MaxSigOpsPerBlock) return ruleError(ErrTooManySigOps, str) @@ -3404,58 +3413,64 @@ func CheckTransactionInputs(subsidyCache *standalone.SubsidyCache, return txFeeInAtom, nil } -// CountSigOps returns the number of signature operations for all transaction -// input and output scripts in the provided transaction. This uses the -// quicker, but imprecise, signature operation counting mechanism from -// txscript. -func CountSigOps(tx *dcrutil.Tx, isCoinBaseTx bool, isSSGen bool, isTreasuryEnabled bool) int { +// countSigOps returns the number of signature operations for all input and +// output scripts in the provided transaction. This uses the quicker, but +// imprecise, signature operation counting mechanism from the script engine. +func countSigOps(tx *dcrutil.Tx, isCoinBase bool, isVote bool, isTreasuryEnabled bool) (uint32, error) { + // Treasurybases do not have any signature operations. msgTx := tx.MsgTx() - - totalSigOps := 0 - if isTreasuryEnabled && stake.IsTreasuryBase(msgTx) { - return totalSigOps + return 0, nil } - if !isCoinBaseTx { - // Accumulate the number of signature operations in all - // transaction inputs. - for i, txIn := range msgTx.TxIn { - // Skip stakebase inputs. - if isSSGen && i == 0 { - continue - } + // Determine which transaction inputs to consider. Coinbase transactions + // and the first input (aka stakebase) of a vote do not have normal input + // scripts to consider. + var txInStartIdx int + txIns := msgTx.TxIn + if isCoinBase { + txIns = nil + } else if isVote && len(txIns) > 0 { + txInStartIdx = 1 + txIns = txIns[txInStartIdx:] + } - numSigOps := txscript.GetSigOpCount(txIn.SignatureScript, - isTreasuryEnabled) - totalSigOps += numSigOps + // Accumulate the number of signature operations in all relevant transaction + // inputs. + var totalSigOps uint32 + var ok bool + for txInIdx, txIn := range txIns { + sigScript := txIn.SignatureScript + numSigOps := txscript.GetSigOpCount(sigScript, isTreasuryEnabled) + totalSigOps, ok = addUnsigned(totalSigOps, uint32(numSigOps)) + if !ok { + str := fmt.Sprintf("input %v:%d signature script causes signature "+ + "operation count to overflow", tx.Hash(), txInIdx+txInStartIdx) + return 0, ruleError(ErrTooManySigOps, str) } } - // Accumulate the number of signature operations in all transaction - // outputs. - for _, txOut := range msgTx.TxOut { - numSigOps := txscript.GetSigOpCount(txOut.PkScript, - isTreasuryEnabled) - totalSigOps += numSigOps + // Accumulate the number of signature operations in all transaction outputs. + for txOutIdx, txOut := range msgTx.TxOut { + numSigOps := txscript.GetSigOpCount(txOut.PkScript, isTreasuryEnabled) + totalSigOps, ok = addUnsigned(totalSigOps, uint32(numSigOps)) + if !ok { + str := fmt.Sprintf("output %v:%d public key script causes "+ + "signature operation count to overflow", tx.Hash(), txOutIdx) + return 0, ruleError(ErrTooManySigOps, str) + } } - return totalSigOps + return totalSigOps, nil } -// CountP2SHSigOps returns the number of signature operations for all input +// countP2SHSigOps returns the number of signature operations for all input // transactions which are of the pay-to-script-hash type. This uses the // precise, signature operation counting mechanism from the script engine which // requires access to the input transaction scripts. -func CountP2SHSigOps(tx *dcrutil.Tx, isCoinBaseTx bool, isStakeBaseTx bool, view *UtxoViewpoint, isTreasuryEnabled bool) (int, error) { +func countP2SHSigOps(tx *dcrutil.Tx, isCoinBase bool, isVote bool, view *UtxoViewpoint, isTreasuryEnabled bool) (uint32, error) { // Coinbase transactions have no interesting inputs. - if isCoinBaseTx { - return 0, nil - } - - // Stakebase (SSGen) transactions have no P2SH inputs. Same with SSRtx, - // but they will still pass the checks below. - if isStakeBaseTx { + if isCoinBase { return 0, nil } @@ -3469,42 +3484,44 @@ func CountP2SHSigOps(tx *dcrutil.Tx, isCoinBaseTx bool, isStakeBaseTx bool, view } } - // Accumulate the number of signature operations in all transaction - // inputs. - totalSigOps := 0 - for txInIndex, txIn := range msgTx.TxIn { + // The first input (aka stakebase) of votes have no P2SH inputs. + var txInStartIdx int + txIns := msgTx.TxIn + if isVote && len(txIns) > 0 { + txInStartIdx = 1 + txIns = txIns[txInStartIdx:] + } + + // Accumulate the number of signature operations from all pay-to-script-hash + // transaction inputs. + var totalSigOps uint32 + for txInIndex, txIn := range txIns { // Ensure the referenced input transaction is available. txInOutpoint := txIn.PreviousOutPoint utxoEntry := view.LookupEntry(txInOutpoint) if utxoEntry == nil || utxoEntry.IsSpent() { - str := fmt.Sprintf("output %v referenced from "+ - "transaction %s:%d either does not exist or "+ - "has already been spent", txInOutpoint, - tx.Hash(), txInIndex) + str := fmt.Sprintf("output %v referenced from transaction %s:%d "+ + "either does not exist or has already been spent", txInOutpoint, + tx.Hash(), txInIndex+txInStartIdx) return 0, ruleError(ErrMissingTxOut, str) } - // We're only interested in pay-to-script-hash types, so skip - // this input if it's not one. + // Skip inputs that aren't pay-to-script-hash. pkScript := utxoEntry.PkScript() if !txscript.IsPayToScriptHash(pkScript) { continue } - // Count the precise number of signature operations in the - // referenced public key script. + // Count the precise number of signature operations in the referenced + // public key script. + var ok bool sigScript := txIn.SignatureScript numSigOps := txscript.GetPreciseSigOpCount(sigScript, pkScript, isTreasuryEnabled) - - // We could potentially overflow the accumulator so check for - // overflow. - lastSigOps := totalSigOps - totalSigOps += numSigOps - if totalSigOps < lastSigOps { - str := fmt.Sprintf("the public key script from output "+ - "%v contains too many signature operations - "+ - "overflow", txInOutpoint) + totalSigOps, ok = addUnsigned(totalSigOps, uint32(numSigOps)) + if !ok { + str := fmt.Sprintf("output %v public key script causes signature "+ + "operations count to overflow", txInOutpoint) return 0, ruleError(ErrTooManySigOps, str) } } @@ -3512,42 +3529,32 @@ func CountP2SHSigOps(tx *dcrutil.Tx, isCoinBaseTx bool, isStakeBaseTx bool, view return totalSigOps, nil } -// checkNumSigOps Checks the number of P2SH signature operations to make -// sure they don't overflow the limits. It takes a cumulative number of sig -// ops as an argument and increments will each call. -func checkNumSigOps(tx *dcrutil.Tx, view *UtxoViewpoint, index int, stakeTree bool, cumulativeSigOps int, isTreasuryEnabled bool) (int, error) { - msgTx := tx.MsgTx() - isSSGen := stake.IsSSGen(msgTx) - isCoinbaseTx := (index == 0) && !stakeTree - numsigOps := CountSigOps(tx, isCoinbaseTx, isSSGen, isTreasuryEnabled) - - // Since the first (and only the first) transaction has already been - // verified to be a coinbase transaction, use (i == 0) && TxTree as an - // optimization for the flag to countP2SHSigOps for whether or not the - // transaction is a coinbase transaction rather than having to do a - // full coinbase check again. - numP2SHSigOps, err := CountP2SHSigOps(tx, isCoinbaseTx, isSSGen, view, - isTreasuryEnabled) +// CountTotalSigOps returns the total number of signature operations for the +// given transaction. This includes all input and output scripts as well as +// signature operations in any redeemed pay-to-script-hash inputs. +func CountTotalSigOps(tx *dcrutil.Tx, isCoinBase, isVote bool, view *UtxoViewpoint, isTreasuryEnabled bool) (uint32, error) { + // Count the number of regular signature operations. + numSigOps, err := countSigOps(tx, isCoinBase, isVote, isTreasuryEnabled) if err != nil { - log.Tracef("CountP2SHSigOps failed; error returned %v", err) return 0, err } - startCumSigOps := cumulativeSigOps - cumulativeSigOps += numsigOps - cumulativeSigOps += numP2SHSigOps + // Count the number of precise pay-to-script-hash signature operations. + numP2SHSigOps, err := countP2SHSigOps(tx, isCoinBase, isVote, view, + isTreasuryEnabled) + if err != nil { + return 0, err + } - // Check for overflow or going over the limits. We have to do - // this on every loop iteration to avoid overflow. - if cumulativeSigOps < startCumSigOps || - cumulativeSigOps > MaxSigOpsPerBlock { - str := fmt.Sprintf("block contains too many signature "+ - "operations - got %v, max %v", cumulativeSigOps, - MaxSigOpsPerBlock) + // Ensure the combined total does not overflow. + totalSigOps, ok := addUnsigned(numSigOps, numP2SHSigOps) + if !ok { + str := fmt.Sprintf("tx %v total signature operations overflow", + tx.Hash()) return 0, ruleError(ErrTooManySigOps, str) } - return cumulativeSigOps, nil + return totalSigOps, nil } // checkStakeBaseAmounts calculates the total amount given as subsidy from @@ -3735,16 +3742,34 @@ func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, } totalFees := int64(inputFees) // Stake tx tree carry forward prevHeader := node.parent.Header() - var cumulativeSigOps int + var cumulativeSigOps uint32 for idx, tx := range txs { - // Ensure that the number of signature operations is not beyond - // the consensus limit. - var err error - cumulativeSigOps, err = checkNumSigOps(tx, view, idx, stakeTree, - cumulativeSigOps, isTreasuryEnabled) + // Since the first (and only the first) transaction has already been + // verified to be a coinbase transaction, use (idx == 0) && !stakeTree + // as an optimization rather than having to do a full coinbase check + // again. + isCoinBase := (idx == 0) && !stakeTree + isVote := stakeTree && stake.IsSSGen(tx.MsgTx()) + + // The number of signature operations must not overflow the accumulator + // and be less than the maximum allowed per block. + var ok bool + numSigOps, err := CountTotalSigOps(tx, isCoinBase, isVote, view, + isTreasuryEnabled) if err != nil { return err } + cumulativeSigOps, ok = addUnsigned(cumulativeSigOps, numSigOps) + if !ok { + str := fmt.Sprintf("tx %v causes block signature operation count "+ + "to overflow", tx.Hash()) + return ruleError(ErrTooManySigOps, str) + } + if cumulativeSigOps > MaxSigOpsPerBlock { + str := fmt.Sprintf("block contains too many signature operations "+ + "- got %v, max %v", cumulativeSigOps, MaxSigOpsPerBlock) + return ruleError(ErrTooManySigOps, str) + } // Perform a series of checks on the inputs to the transaction to ensure // they are valid and calculate the total fees for it. diff --git a/internal/mempool/mempool.go b/internal/mempool/mempool.go index a69a1293f..6985a76b3 100644 --- a/internal/mempool/mempool.go +++ b/internal/mempool/mempool.go @@ -1130,7 +1130,7 @@ func (mp *TxPool) FetchTransaction(txHash *chainhash.Hash) (*dcrutil.Tx, error) // newTxDesc returns a new TxDesc instance that captures mempool state // relevant to the provided transaction at the current time. func (mp *TxPool) newTxDesc(tx *dcrutil.Tx, txType stake.TxType, height int64, - fee int64, totalSigOps int, txSize int64) *TxDesc { + fee int64, totalSigOps uint32, txSize int64) *TxDesc { return &TxDesc{ TxDesc: mining.TxDesc{ @@ -1582,13 +1582,13 @@ func (mp *TxPool) maybeAcceptTransaction(tx *dcrutil.Tx, isNew, allowHighFees, // you should add code here to check that the transaction does a // reasonable number of ECDSA signature verifications. - // Don't allow transactions with an excessive number of signature - // operations which would result in making it impossible to mine. Since - // the coinbase address itself can contain signature operations, the - // maximum allowed signature operations per transaction is less than - // the maximum allowed signature operations per block. - numP2SHSigOps, err := blockchain.CountP2SHSigOps(tx, false, - (txType == stake.TxTypeSSGen), utxoView, isTreasuryEnabled) + // Don't allow transactions with an excessive number of signature operations + // which would result in making it impossible to mine. Since the coinbase + // address itself can contain signature operations, the maximum allowed + // signature operations per transaction is less than the maximum allowed + // signature operations per block. + totalSigOps, err := blockchain.CountTotalSigOps(tx, false, isVote, utxoView, + isTreasuryEnabled) if err != nil { var cerr blockchain.RuleError if errors.As(err, &cerr) { @@ -1596,10 +1596,7 @@ func (mp *TxPool) maybeAcceptTransaction(tx *dcrutil.Tx, isNew, allowHighFees, } return nil, err } - - numSigOps := blockchain.CountSigOps(tx, false, isVote, isTreasuryEnabled) - totalSigOps := numP2SHSigOps + numSigOps - if totalSigOps > mp.cfg.Policy.MaxSigOpsPerTx { + if totalSigOps > uint32(mp.cfg.Policy.MaxSigOpsPerTx) { str := fmt.Sprintf("transaction %v has too many sigops: %d > %d", txHash, totalSigOps, mp.cfg.Policy.MaxSigOpsPerTx) return nil, txRuleError(ErrNonStandard, str) diff --git a/internal/mining/mining.go b/internal/mining/mining.go index 26ef90341..302a837dc 100644 --- a/internal/mining/mining.go +++ b/internal/mining/mining.go @@ -110,10 +110,12 @@ type Config struct { // tspend has enough votes to be included in a block AFTER the specified block. CheckTSpendHasVotes func(prevHash chainhash.Hash, tspend *dcrutil.Tx) error - // CountSigOps defines the function to use to count the number of signature - // operations for all transaction input and output scripts in the provided - // transaction. - CountSigOps func(tx *dcrutil.Tx, isCoinBaseTx bool, isSSGen bool, isTreasuryEnabled bool) int + // CountTotalSigOps defines the function to use to count the total number of + // signature operations for the given transaction. This includes all input + // and output scripts as well as signature operations in any redeemed + // pay-to-script-hash inputs. + CountTotalSigOps func(tx *dcrutil.Tx, isCoinBaseTx, isVoteTx bool, + view *blockchain.UtxoViewpoint, isTreasuryEnabled bool) (uint32, error) // FetchUtxoEntry defines the function to use to load and return the requested // unspent transaction output from the point of view of the main chain tip. @@ -224,7 +226,7 @@ type TxDesc struct { Fee int64 // TotalSigOps is the total signature operations for this transaction. - TotalSigOps int + TotalSigOps uint32 // TxSize is the size of the transaction. TxSize int64 @@ -240,7 +242,7 @@ type TxAncestorStats struct { SizeBytes int64 // TotalSigOps is the total number of signature operations of all ancestors. - TotalSigOps int + TotalSigOps uint32 // NumAncestors is the total number of ancestors for a given transaction. NumAncestors int @@ -421,7 +423,7 @@ type BlockTemplate struct { // SigOpCounts contains the number of signature operations each // transaction in the generated template performs. - SigOpCounts []int64 + SigOpCounts []uint64 // Height is the height at which the block template connects to the main // chain. @@ -879,7 +881,7 @@ func (g *BlkTmplGenerator) handleTooFewVoters(nextHeight int64, bt := &BlockTemplate{ Block: &block, Fees: []int64{0}, - SigOpCounts: []int64{0}, + SigOpCounts: []uint64{0}, Height: int64(tipHeader.Height), ValidPayAddress: miningAddress != nil, } @@ -992,12 +994,18 @@ func (g *BlkTmplGenerator) createRevocationFromTicket(ticketHash *chainhash.Hash } revocationTx := dcrutil.NewTx(revocationMsgTx) revocationTx.SetTree(wire.TxTreeStake) + + totalSigOps, err := g.cfg.CountTotalSigOps(revocationTx, false, false, + blockUtxos, isTreasuryEnabled) + if err != nil { + return nil, err + } + txDesc := &TxDesc{ - Tx: revocationTx, - Type: stake.TxTypeSSRtx, - TotalSigOps: g.cfg.CountSigOps(revocationTx, false, false, - isTreasuryEnabled), - TxSize: int64(revocationMsgTx.SerializeSize()), + Tx: revocationTx, + Type: stake.TxTypeSSRtx, + TotalSigOps: totalSigOps, + TxSize: int64(revocationMsgTx.SerializeSize()), } return txDesc, nil @@ -1316,8 +1324,8 @@ func (g *BlkTmplGenerator) NewBlockTemplate(payToAddress stdaddr.Address) (*Bloc // the coinbase fee which will be updated later. txFees := make([]int64, 0, len(sourceTxns)) txFeesMap := make(map[chainhash.Hash]int64) - txSigOpCounts := make([]int64, 0, len(sourceTxns)) - txSigOpCountsMap := make(map[chainhash.Hash]int64) + txSigOpCounts := make([]uint64, 0, len(sourceTxns)) + txSigOpCountsMap := make(map[chainhash.Hash]uint64) txFees = append(txFees, -1) // Updated once known log.Debugf("Considering %d transactions for inclusion to new block", @@ -1443,7 +1451,7 @@ mempoolLoop: // trees if they fail one of the stake checks below the priorityQueue // pop loop. This is buggy, but not catastrophic behaviour. A future // release should fix it. TODO - blockSigOps := int64(0) + blockSigOps := uint64(0) totalFees := int64(0) numSStx := 0 @@ -1693,8 +1701,8 @@ nextPriorityQueueItem: // Enforce maximum signature operations per block. Also check // for overflow. - numSigOps := int64(prioItem.txDesc.TotalSigOps) - numSigOpsBundle := numSigOps + int64(ancestorStats.TotalSigOps) + numSigOps := uint64(prioItem.txDesc.TotalSigOps) + numSigOpsBundle := numSigOps + uint64(ancestorStats.TotalSigOps) if blockSigOps+numSigOpsBundle < blockSigOps || blockSigOps+numSigOpsBundle > blockchain.MaxSigOpsPerBlock { log.Tracef("Skipping tx %s because it would "+ @@ -1778,7 +1786,7 @@ nextPriorityQueueItem: // template. blockTxns = append(blockTxns, bundledTx) blockSize += uint32(bundledTx.MsgTx().SerializeSize()) - bundledTxSigOps := int64(bundledTxDesc.TotalSigOps) + bundledTxSigOps := uint64(bundledTxDesc.TotalSigOps) blockSigOps += bundledTxSigOps // Accumulate the SStxs in the block, because only a certain number @@ -2058,16 +2066,19 @@ nextPriorityQueueItem: g.cfg.ChainParams, isTreasuryEnabled, subsidySplitVariant) coinbaseTx.SetTree(wire.TxTreeRegular) - numCoinbaseSigOps := int64(g.cfg.CountSigOps(coinbaseTx, true, - false, isTreasuryEnabled)) + numCoinbaseSigOps, err := g.cfg.CountTotalSigOps(coinbaseTx, true, false, + blockUtxos, isTreasuryEnabled) + if err != nil { + log.Debug(err) + return nil, err + } blockSize += uint32(coinbaseTx.MsgTx().SerializeSize()) - blockSigOps += numCoinbaseSigOps + blockSigOps += uint64(numCoinbaseSigOps) txFeesMap[*coinbaseTx.Hash()] = 0 - txSigOpCountsMap[*coinbaseTx.Hash()] = numCoinbaseSigOps + txSigOpCountsMap[*coinbaseTx.Hash()] = uint64(numCoinbaseSigOps) if treasuryBase != nil { txFeesMap[*treasuryBase.Hash()] = 0 - n := int64(g.cfg.CountSigOps(treasuryBase, true, false, isTreasuryEnabled)) - txSigOpCountsMap[*treasuryBase.Hash()] = n + txSigOpCountsMap[*treasuryBase.Hash()] = 0 } // Build tx lists for regular tx. @@ -2129,7 +2140,7 @@ nextPriorityQueueItem: totalFees /= int64(g.cfg.ChainParams.TicketsPerBlock) } - txSigOpCounts = append(txSigOpCounts, numCoinbaseSigOps) + txSigOpCounts = append(txSigOpCounts, uint64(numCoinbaseSigOps)) // Now that the actual transactions have been selected, update the // block size for the real transaction count and coinbase value with diff --git a/internal/mining/mining_harness_test.go b/internal/mining/mining_harness_test.go index 7ce294a8a..4eaa938fa 100644 --- a/internal/mining/mining_harness_test.go +++ b/internal/mining/mining_harness_test.go @@ -1,4 +1,4 @@ -// Copyright (c) 2020-2024 The Decred developers +// Copyright (c) 2020-2026 The Decred developers // Use of this source code is governed by an ISC // license that can be found in the LICENSE file. @@ -392,23 +392,15 @@ func (p *fakeTxSource) IsRegTxTreeKnownDisapproved(hash *chainhash.Hash) bool { // CountTotalSigOps returns the total number of signature operations for the // given transaction. -func (p *fakeTxSource) CountTotalSigOps(tx *dcrutil.Tx, txType stake.TxType) (int, error) { +func (p *fakeTxSource) CountTotalSigOps(tx *dcrutil.Tx, txType stake.TxType) (uint32, error) { isVote := txType == stake.TxTypeSSGen - isStakeBase := txType == stake.TxTypeSSGen utxoView, err := p.fetchInputUtxos(tx, p.chain.isTreasuryAgendaActive) if err != nil { return 0, err } - sigOps := blockchain.CountSigOps(tx, false, isVote, + return blockchain.CountTotalSigOps(tx, false, isVote, utxoView, p.chain.isTreasuryAgendaActive) - p2shSigOps, err := blockchain.CountP2SHSigOps(tx, false, isStakeBase, - utxoView, p.chain.isTreasuryAgendaActive) - if err != nil { - return 0, err - } - - return sigOps + p2shSigOps, nil } // fetchRedeemers returns all transactions that reference an outpoint for the @@ -508,7 +500,7 @@ func (p *fakeTxSource) removeOrphanDoubleSpends(tx *dcrutil.Tx) { } // addTransaction adds the passed transaction to the fake tx source. -func (p *fakeTxSource) addTransaction(tx *dcrutil.Tx, txType stake.TxType, height int64, fee int64, totalSigOps int) { +func (p *fakeTxSource) addTransaction(tx *dcrutil.Tx, txType stake.TxType, height int64, fee int64, totalSigOps uint32) { // Add the transaction to the pool and mark the referenced outpoints // as spent by the pool. txDesc := TxDesc{ @@ -1299,7 +1291,7 @@ func (m *miningHarness) CreateVote(ticket *dcrutil.Tx, mungers ...func(*wire.Msg // CountTotalSigOps returns the total number of signature operations for the // given transaction. -func (m *miningHarness) CountTotalSigOps(tx *dcrutil.Tx) (int, error) { +func (m *miningHarness) CountTotalSigOps(tx *dcrutil.Tx) (uint32, error) { txType := stake.DetermineTxType(tx.MsgTx()) return m.txSource.CountTotalSigOps(tx, txType) } @@ -1459,7 +1451,7 @@ func newMiningHarness(chainParams *chaincfg.Params) (*miningHarness, []spendable isAutoRevocationsEnabled, subsidySplitVariant) }, CheckTSpendHasVotes: chain.CheckTSpendHasVotes, - CountSigOps: blockchain.CountSigOps, + CountTotalSigOps: blockchain.CountTotalSigOps, FetchUtxoEntry: chain.FetchUtxoEntry, FetchUtxoView: chain.FetchUtxoView, FetchUtxoViewParentTemplate: chain.FetchUtxoViewParentTemplate, diff --git a/internal/mining/mining_view_test.go b/internal/mining/mining_view_test.go index 47651fc19..a9371ffd7 100644 --- a/internal/mining/mining_view_test.go +++ b/internal/mining/mining_view_test.go @@ -94,7 +94,7 @@ func TestMiningView(t *testing.T) { subject *dcrutil.Tx expectedAncestorFees int64 expectedSizeBytes int64 - expectedSigOps int + expectedSigOps uint32 ancestors []*dcrutil.Tx descendants map[chainhash.Hash]*dcrutil.Tx orderedAncestors [][]*dcrutil.Tx diff --git a/server.go b/server.go index 3f823bc93..5439f135c 100644 --- a/server.go +++ b/server.go @@ -4308,7 +4308,7 @@ func newServer(ctx context.Context, profiler *profileServer, isAutoRevocationsEnabled, subsidySplitVariant) }, CheckTSpendHasVotes: s.chain.CheckTSpendHasVotes, - CountSigOps: blockchain.CountSigOps, + CountTotalSigOps: blockchain.CountTotalSigOps, FetchUtxoEntry: s.chain.FetchUtxoEntry, FetchUtxoView: s.chain.FetchUtxoView, FetchUtxoViewParentTemplate: s.chain.FetchUtxoViewParentTemplate, From 14f42ba5e0d56720c36fa1ddb3812c3498b99497 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Wed, 6 May 2026 07:07:33 -0500 Subject: [PATCH 05/10] blockchain: Unsigned treasury spend vote counts. As mentioned in the previous commit, consensus code should ideally always use types of a specific size so there is no possibility of divergent behavior when compiled on different architectures due to differing upper bounds. Further, unsigned types for values that can never be negative should always be preferred. The current code for counting treasury spend votes does not adhere to practice and uses plain signed integers. It is worth noting that this is not an active issue at the moment because the maximum possible counts achievable due to other limits imposed on the max number of votes and the window over which the number of votes are counted are less than the maximum value of the smallest supported architecture (32 bits). Nevertheless, these types of hidden assumptions are fragile and can easily lead to undetected and unexpected behavior when parameters change and new features are introduced. The primary goal of this commit is to address that by modifying the relevant code to use fixed unsigned 32-bit integers instead. While here, it also takes the opportunity to cleanup the code to make it more consistent with the other consensus code. Note that there is no risk of consensus divergence due to the aforementioned impossibility of hitting the conditions with the current combination of parameters and limits. --- internal/blockchain/error.go | 20 ++- internal/blockchain/error_test.go | 4 +- internal/blockchain/treasury.go | 155 +++++++++---------- internal/blockchain/treasury_test.go | 48 +++--- internal/blockchain/validate.go | 2 + internal/rpcserver/interface.go | 2 +- internal/rpcserver/rpcserver.go | 6 +- internal/rpcserver/rpcserverhandlers_test.go | 6 +- 8 files changed, 130 insertions(+), 113 deletions(-) diff --git a/internal/blockchain/error.go b/internal/blockchain/error.go index 5382d360b..f4c864cb1 100644 --- a/internal/blockchain/error.go +++ b/internal/blockchain/error.go @@ -477,14 +477,30 @@ const ( // block that is not at a TVI interval. ErrNotTVI = ErrorKind("ErrNotTVI") - // ErrInvalidTSpendWindow indicates that this treasury spend - // transaction is outside of the allowed window. + // ErrInvalidTreasurySpendExpiry indicates that a treasury spend transaction + // has an invalid expiry. + ErrInvalidTreasurySpendExpiry = ErrorKind("ErrInvalidTreasurySpendExpiry") + + // ErrInvalidTSpendWindow indicates that a treasury spend transaction is + // outside of the allowed window. ErrInvalidTSpendWindow = ErrorKind("ErrInvalidTSpendWindow") // ErrNotEnoughTSpendVotes indicates that a treasury spend transaction // does not have enough votes to be included in block. ErrNotEnoughTSpendVotes = ErrorKind("ErrNotEnoughTSpendVotes") + // ErrTooManyTreasurySpendVotes indicates that the number of treasury spend + // votes in a treasury voting window exceeeded maximum allowable number of + // votes. + // + // In practice, this implies there was an unexpected overflow when tallying + // votes since there is not directly an explicit upper bound on the allowed + // votes. Rather, the upper bound is implicit due to the size of the voting + // window and the maximum number of allowed stake votes per block. + // + // This error is not possible to hit at the time this comment was written. + ErrTooManyTreasurySpendVotes = ErrorKind("ErrTooManyTreasurySpendVotes") + // ErrInvalidTSpendValueIn indicates that a treasury spend transaction // ValueIn does not match the encoded copy in the first TxOut. ErrInvalidTSpendValueIn = ErrorKind("ErrInvalidTSpendValueIn") diff --git a/internal/blockchain/error_test.go b/internal/blockchain/error_test.go index 2262e06ed..4dcd630a8 100644 --- a/internal/blockchain/error_test.go +++ b/internal/blockchain/error_test.go @@ -1,5 +1,5 @@ // Copyright (c) 2014 The btcsuite developers -// Copyright (c) 2015-2023 The Decred developers +// Copyright (c) 2015-2026 The Decred developers // Use of this source code is governed by an ISC // license that can be found in the LICENSE file. @@ -121,8 +121,10 @@ func TestErrorKindStringer(t *testing.T) { {ErrInvalidPiSignature, "ErrInvalidPiSignature"}, {ErrInvalidTVoteWindow, "ErrInvalidTVoteWindow"}, {ErrNotTVI, "ErrNotTVI"}, + {ErrInvalidTreasurySpendExpiry, "ErrInvalidTreasurySpendExpiry"}, {ErrInvalidTSpendWindow, "ErrInvalidTSpendWindow"}, {ErrNotEnoughTSpendVotes, "ErrNotEnoughTSpendVotes"}, + {ErrTooManyTreasurySpendVotes, "ErrTooManyTreasurySpendVotes"}, {ErrInvalidTSpendValueIn, "ErrInvalidTSpendValueIn"}, {ErrTSpendExists, "ErrTSpendExists"}, {ErrInvalidExpenditure, "ErrInvalidExpenditure"}, diff --git a/internal/blockchain/treasury.go b/internal/blockchain/treasury.go index 091b595ea..3c3e7e15d 100644 --- a/internal/blockchain/treasury.go +++ b/internal/blockchain/treasury.go @@ -1,4 +1,4 @@ -// Copyright (c) 2020-2025 The Decred developers +// Copyright (c) 2020-2026 The Decred developers // Use of this source code is governed by an ISC // license that can be found in the LICENSE file. @@ -976,104 +976,105 @@ func (b *BlockChain) CheckTSpendExists(prevHash, tspend chainhash.Hash) error { return b.checkTSpendExists(prevNode, tspend) } -// getVotes returns yes and no votes for the provided hash. -func getVotes(votes []stake.TreasuryVoteTuple, hash *chainhash.Hash) (yes int, no int) { - if votes == nil { - return - } - - for _, v := range votes { - if !hash.IsEqual(&v.Hash) { - continue - } - - switch v.Vote { - case stake.TreasuryVoteYes: - yes++ - case stake.TreasuryVoteNo: - no++ - default: - // Can't happen. - trsyLog.Criticalf("getVotes: invalid vote 0x%v", v.Vote) - } - } - - return -} - // tspendVotes is a structure that contains a treasury vote tally for a given // window. type tspendVotes struct { start uint32 // Start block end uint32 // End block - yes int // Yes vote tally - no int // No vote tally + yes uint32 // Yes vote tally + no uint32 // No vote tally } -// tSpendCountVotes returns the vote tally for a given tspend up to the -// specified block. Note that this function errors if the block is outside the -// voting window for the given tspend. +// tSpendCountVotes returns the total yes and no vote counts for a given +// treasury spend up to the specified block. +// +// An error is returned if the block is outside the voting window for the given +// treasury spend. +// +// This function is safe for concurrent access. func (b *BlockChain) tSpendCountVotes(prevNode *blockNode, tspend *dcrutil.Tx) (*tspendVotes, error) { - var ( - t tspendVotes - err error - ) + // Shorter version of various parameter for convenience. + tvi := b.chainParams.TreasuryVoteInterval + tvim := b.chainParams.TreasuryVoteIntervalMultiplier expiry := tspend.MsgTx().Expiry - t.start, t.end, err = standalone.CalcTSpendWindow(expiry, - b.chainParams.TreasuryVoteInterval, - b.chainParams.TreasuryVoteIntervalMultiplier) + start, end, err := standalone.CalcTSpendWindow(expiry, tvi, tvim) if err != nil { - return nil, err + return nil, standaloneToChainRuleError(err) } + trsySpendHash := tspend.Hash() nextHeight := prevNode.height + 1 trsyLog.Tracef("Counting votes for treasury spend %s (height %d, voting "+ - "window [%d, %d], expiry %d)", tspend.Hash(), nextHeight, t.start, - t.end, expiry) - - // Ensure tspend is within the window. - if !standalone.InsideTSpendWindow(nextHeight, - expiry, b.chainParams.TreasuryVoteInterval, - b.chainParams.TreasuryVoteIntervalMultiplier) { - err = fmt.Errorf("tspend outside of window: nextHeight %v "+ - "start %v expiry %v", nextHeight, t.start, expiry) - return nil, err - } - - node := prevNode - for { - if node.height < int64(t.start) { - break - } - - // Find SSGen and peel out votes. - var xblock *dcrutil.Block - xblock, err = b.fetchBlockByNode(node) + "window [%d, %d], expiry %d)", trsySpendHash, nextHeight, start, end, + expiry) + + // Ensure the treasury spend is within its valid voting window. + if !standalone.InsideTSpendWindow(nextHeight, expiry, tvi, tvim) { + str := fmt.Sprintf("treasury spend %v at height %d with expiry %d is "+ + "outside of the valid window [%d, %d]", trsySpendHash, nextHeight, + expiry, start, end) + return nil, ruleError(ErrInvalidTSpendWindow, str) + } + + // Tally the total number of yes and no votes in the voting window. + var totalYes, totalNo uint32 + for n := prevNode; n != nil && n.height >= int64(start); n = n.parent { + // Find stake votes and extract treasury spend votes. + var ok bool + var block *dcrutil.Block + block, err = b.fetchBlockByNode(n) if err != nil { // Should not happen. return nil, err } - for _, v := range xblock.STransactions() { - votes, err := stake.CheckSSGenVotes(v.MsgTx()) + for _, stx := range block.MsgBlock().STransactions { + // Nothing to do for transactions that are not stake votes or do not + // contain any treasury spend votes. + votes, err := stake.CheckSSGenVotes(stx) if err != nil { - // Not an SSGEN + // Not a stake vote. + continue + } + if len(votes) == 0 { continue } - // Find our vote bits. - yes, no := getVotes(votes, tspend.Hash()) - t.yes += yes - t.no += no - } + // Find and tally votes for the treasury spend hash. + for _, v := range votes { + if *trsySpendHash != v.Hash { + continue + } - node = node.parent - if node == nil { - break + switch v.Vote { + case stake.TreasuryVoteYes: + totalYes, ok = addUnsigned(totalYes, 1) + if !ok { + str := fmt.Sprintf("yes vote for treasury spend %v at "+ + "height %d causes yes count to overflow", + trsySpendHash, n.height) + return nil, ruleError(ErrTooManyTreasurySpendVotes, str) + } + + case stake.TreasuryVoteNo: + totalNo, ok = addUnsigned(totalNo, 1) + if !ok { + str := fmt.Sprintf("no vote for treasury spend %v at "+ + "height %d causes no count to overflow", + trsySpendHash, n.height) + return nil, ruleError(ErrTooManyTreasurySpendVotes, str) + } + } + } } } - return &t, nil + return &tspendVotes{ + start: start, + end: end, + yes: totalYes, + no: totalNo, + }, nil } // TSpendCountVotes tallies the votes given for the specified tspend during its @@ -1086,20 +1087,18 @@ func (b *BlockChain) tSpendCountVotes(prevNode *blockNode, tspend *dcrutil.Tx) ( // the next block is outside the voting interval. // // This function is safe for concurrent access. -func (b *BlockChain) TSpendCountVotes(blockHash *chainhash.Hash, tspend *dcrutil.Tx) (yesVotes, noVotes int64, err error) { - b.index.RLock() - defer b.index.RUnlock() - - prevNode := b.index.lookupNode(blockHash) +func (b *BlockChain) TSpendCountVotes(blockHash *chainhash.Hash, tspend *dcrutil.Tx) (yesVotes, noVotes uint32, err error) { + prevNode := b.index.LookupNode(blockHash) if prevNode == nil { return 0, 0, unknownBlockError(blockHash) } + tv, err := b.tSpendCountVotes(prevNode, tspend) if err != nil { return 0, 0, err } - return int64(tv.yes), int64(tv.no), nil + return tv.yes, tv.no, nil } // checkTSpendHasVotes verifies that the provided TSpend has enough votes to be diff --git a/internal/blockchain/treasury_test.go b/internal/blockchain/treasury_test.go index e296bca3c..2381053a9 100644 --- a/internal/blockchain/treasury_test.go +++ b/internal/blockchain/treasury_test.go @@ -505,13 +505,13 @@ func TestTSpendVoteCount(t *testing.T) { t.Fatalf("invalid end block got %v wanted %v", tv.end, end) } - expectedYesVotes := 0 // We voted a bunch of times outside the window + const expectedYesVotes = 0 // We voted a bunch of times outside the window expectedNoVotes := tvi * mul * uint64(params.TicketsPerBlock) - if expectedYesVotes != tv.yes { - t.Fatalf("invalid yes votes got %v wanted %v", expectedYesVotes, tv.yes) + if tv.yes != expectedYesVotes { + t.Fatalf("invalid yes votes got %v wanted %v", tv.yes, expectedYesVotes) } - if expectedNoVotes != uint64(tv.no) { - t.Fatalf("invalid no votes got %v wanted %v", expectedNoVotes, tv.no) + if uint64(tv.no) != expectedNoVotes { + t.Fatalf("invalid no votes got %v wanted %v", tv.no, expectedNoVotes) } // --------------------------------------------------------------------- @@ -614,9 +614,8 @@ func TestTSpendVoteCount(t *testing.T) { if err != nil { t.Fatal(err) } - if int(quorum-1) != tv.yes { - t.Fatalf("unexpected yesVote count got %v wanted %v", - tv.yes, quorum-1) + if uint64(tv.yes) != quorum-1 { + t.Fatalf("unexpected yesVote count got %v wanted %v", tv.yes, quorum-1) } // Hit exact yes vote quorum @@ -647,9 +646,8 @@ func TestTSpendVoteCount(t *testing.T) { if err != nil { t.Fatal(err) } - if int(quorum) != tv.yes { - t.Fatalf("unexpected yesVote count got %v wanted %v", - tv.yes, quorum) + if uint64(tv.yes) != quorum { + t.Fatalf("unexpected yesVote count got %v wanted %v", tv.yes, quorum) } // Verify TSpend can be added exactly on quorum. @@ -2853,13 +2851,13 @@ func TestTSpendVoteCountSynthetic(t *testing.T) { numNodes int64 set func(*BlockChain, *blockNode) blockVersion int32 - expectedYesVotes int - expectedNoVotes int + expectedYesVotes uint32 + expectedNoVotes uint32 failure bool }{{ name: "All yes", numNodes: int64(end), - expectedYesVotes: int(tpb) * int(tvi*mul), + expectedYesVotes: uint32(tpb) * uint32(tvi*mul), expectedNoVotes: 0, failure: false, set: func(bc *BlockChain, node *blockNode) { @@ -2887,7 +2885,7 @@ func TestTSpendVoteCountSynthetic(t *testing.T) { name: "All no", numNodes: int64(end), expectedYesVotes: 0, - expectedNoVotes: int(tpb) * int(tvi*mul), + expectedNoVotes: uint32(tpb) * uint32(tvi*mul), failure: true, set: func(bc *BlockChain, node *blockNode) { // Create block. @@ -2979,7 +2977,7 @@ func TestTSpendVoteCountSynthetic(t *testing.T) { }, { name: "All yes quorum", numNodes: int64(end), - expectedYesVotes: int(quorum), + expectedYesVotes: uint32(quorum), expectedNoVotes: 0, failure: false, set: func(bc *BlockChain, node *blockNode) { @@ -3010,7 +3008,7 @@ func TestTSpendVoteCountSynthetic(t *testing.T) { }, { name: "All yes quorum - 1", numNodes: int64(end), - expectedYesVotes: int(quorum - 1), + expectedYesVotes: uint32(quorum - 1), expectedNoVotes: 0, failure: true, set: func(bc *BlockChain, node *blockNode) { @@ -3041,7 +3039,7 @@ func TestTSpendVoteCountSynthetic(t *testing.T) { }, { name: "All yes quorum + 1", numNodes: int64(end), - expectedYesVotes: int(quorum + 1), + expectedYesVotes: uint32(quorum + 1), expectedNoVotes: 0, failure: false, set: func(bc *BlockChain, node *blockNode) { @@ -3072,8 +3070,8 @@ func TestTSpendVoteCountSynthetic(t *testing.T) { }, { name: "Exactly yes required", numNodes: int64(end), - expectedYesVotes: int(requiredVotes), - expectedNoVotes: int(maxVotes) - int(requiredVotes), + expectedYesVotes: uint32(requiredVotes), + expectedNoVotes: maxVotes - uint32(requiredVotes), failure: false, set: func(bc *BlockChain, node *blockNode) { // Create block. @@ -3105,8 +3103,8 @@ func TestTSpendVoteCountSynthetic(t *testing.T) { }, { name: "Yes required - 1", numNodes: int64(end), - expectedYesVotes: int(requiredVotes - 1), - expectedNoVotes: int(maxVotes) - int(requiredVotes-1), + expectedYesVotes: uint32(requiredVotes - 1), + expectedNoVotes: maxVotes - uint32(requiredVotes-1), failure: true, set: func(bc *BlockChain, node *blockNode) { // Create block. @@ -3138,8 +3136,8 @@ func TestTSpendVoteCountSynthetic(t *testing.T) { }, { name: "Yes required + 1", numNodes: int64(end), - expectedYesVotes: int(requiredVotes + 1), - expectedNoVotes: int(maxVotes) - int(requiredVotes+1), + expectedYesVotes: uint32(requiredVotes + 1), + expectedNoVotes: maxVotes - uint32(requiredVotes+1), failure: false, set: func(bc *BlockChain, node *blockNode) { // Create block. @@ -3171,7 +3169,7 @@ func TestTSpendVoteCountSynthetic(t *testing.T) { }, { name: "Exactly yes required with abstain", numNodes: int64(end), - expectedYesVotes: int(requiredVotes), + expectedYesVotes: uint32(requiredVotes), expectedNoVotes: 0, failure: false, set: func(bc *BlockChain, node *blockNode) { diff --git a/internal/blockchain/validate.go b/internal/blockchain/validate.go index 31e52a791..cef3849de 100644 --- a/internal/blockchain/validate.go +++ b/internal/blockchain/validate.go @@ -690,6 +690,8 @@ func standaloneToChainRuleError(err error) error { return ruleError(ErrFraudAmountIn, err.Error()) case errors.Is(err, standalone.ErrDuplicateTxInputs): return ruleError(ErrDuplicateTxInputs, err.Error()) + case errors.Is(err, standalone.ErrInvalidTSpendExpiry): + return ruleError(ErrInvalidTreasurySpendExpiry, err.Error()) } return err diff --git a/internal/rpcserver/interface.go b/internal/rpcserver/interface.go index 4a078f181..97b88fde9 100644 --- a/internal/rpcserver/interface.go +++ b/internal/rpcserver/interface.go @@ -440,7 +440,7 @@ type Chain interface { // TSpendCountVotes returns the votes for the specified tspend up to // the specified block. - TSpendCountVotes(*chainhash.Hash, *dcrutil.Tx) (int64, int64, error) + TSpendCountVotes(*chainhash.Hash, *dcrutil.Tx) (uint32, uint32, error) // InvalidateBlock manually invalidates the provided block as if the block // had violated a consensus rule and marks all of its descendants as having diff --git a/internal/rpcserver/rpcserver.go b/internal/rpcserver/rpcserver.go index da4e0c1dd..f4907bdd8 100644 --- a/internal/rpcserver/rpcserver.go +++ b/internal/rpcserver/rpcserver.go @@ -3437,7 +3437,7 @@ func handleGetTreasurySpendVotes(_ context.Context, s *Server, cmd any) (any, er // We only count votes for tspends that are inside their voting // window. Otherwise we just return the appropriate vote start // and end heights for it. - var yes, no int64 + var yes, no uint32 insideWindow := standalone.InsideTSpendWindow(blockHeight, expiry, tvi, mul) minedBlock, isMined := endBlocks[*txHash] if insideWindow || isMined { @@ -3470,8 +3470,8 @@ func handleGetTreasurySpendVotes(_ context.Context, s *Server, cmd any) (any, er Expiry: int64(expiry), VoteStart: int64(start), VoteEnd: int64(end), - YesVotes: yes, - NoVotes: no, + YesVotes: int64(yes), + NoVotes: int64(no), } } diff --git a/internal/rpcserver/rpcserverhandlers_test.go b/internal/rpcserver/rpcserverhandlers_test.go index 562282735..ac6d8fd3b 100644 --- a/internal/rpcserver/rpcserverhandlers_test.go +++ b/internal/rpcserver/rpcserverhandlers_test.go @@ -127,8 +127,8 @@ func (u *testRPCUtxoEntry) TicketMinimalOutputs() []*stake.MinimalOutput { // tspendVotes is used to mock the results of a chain TSpendCountVotes call. type tspendVotes struct { - yes int64 - no int64 + yes uint32 + no uint32 err error } @@ -434,7 +434,7 @@ func (c *testRPCChain) FetchTSpend(chainhash.Hash) ([]chainhash.Hash, error) { // TSpendCountVotes counts the number of votes a given tspend has received up // to the given block. -func (c *testRPCChain) TSpendCountVotes(*chainhash.Hash, *dcrutil.Tx) (int64, int64, error) { +func (c *testRPCChain) TSpendCountVotes(*chainhash.Hash, *dcrutil.Tx) (uint32, uint32, error) { return c.tspendVotes.yes, c.tspendVotes.no, c.tspendVotes.err } From 9088dd938c1d436eaa3f1c65fd5be8a4cc600d7a Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Wed, 6 May 2026 07:07:34 -0500 Subject: [PATCH 06/10] blockchain: Enforce trsybase fee in inputs check. The requirement that treasurybases have zero fee is currently indirectly enforced by ensuring the input sum is the required subsidy and that the total of all fees paid in the stake tree do not exceed the input sum. While that doesn't cause any real issues, it's not very clear and it ideally should be done in the per-transaction input checks so it is consistent with all other transaction types. This modifes CheckTransactionInputs to apply the additional input versus output sum check to apply to treasurybase transactions as well. --- internal/blockchain/treasury_test.go | 2 +- internal/blockchain/validate.go | 13 ++++++------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/internal/blockchain/treasury_test.go b/internal/blockchain/treasury_test.go index 2381053a9..e80179e30 100644 --- a/internal/blockchain/treasury_test.go +++ b/internal/blockchain/treasury_test.go @@ -2342,7 +2342,7 @@ func TestTreasuryBaseCorners(t *testing.T) { // corruptTreasurybaseValueIn is a munge function which modifies the // provided block by mutating the treasurybase in value. corruptTreasurybaseValueIn := func(b *wire.MsgBlock) { - b.STransactions[0].TxIn[0].ValueIn-- + b.STransactions[0].TxIn[0].ValueIn++ } // corruptTreasurybaseValueOut is a munge function which modifies the diff --git a/internal/blockchain/validate.go b/internal/blockchain/validate.go index cef3849de..8b0c26d17 100644 --- a/internal/blockchain/validate.go +++ b/internal/blockchain/validate.go @@ -3110,14 +3110,13 @@ func CheckTransactionInputs(subsidyCache *standalone.SubsidyCache, isTreasuryEnabled, isAutoRevocationsEnabled bool, subsidySplitVariant standalone.SubsidySplitVariant) (int64, error) { - // Coinbase transactions have no inputs. + // NOTE: This check MUST come before the coinbase check because a + // treasurybase will be identified as a coinbase as well. msgTx := tx.MsgTx() - if standalone.IsCoinBaseTx(msgTx, isTreasuryEnabled) { - return 0, nil - } + isTreasuryBase := isTreasuryEnabled && standalone.IsTreasuryBase(msgTx) - // Treasurybase transactions have no inputs. - if isTreasuryEnabled && standalone.IsTreasuryBase(msgTx) { + // Coinbase transactions have no inputs. + if !isTreasuryBase && standalone.IsCoinBaseTx(msgTx, isTreasuryEnabled) { return 0, nil } @@ -3219,7 +3218,7 @@ func CheckTransactionInputs(subsidyCache *standalone.SubsidyCache, } // idx can only be 0 in this case but check it anyway. - if isTSpend && idx == 0 { + if (isTreasuryBase || isTSpend) && idx == 0 { totalAtomIn += txIn.ValueIn continue } From 1d5b5a7ca2ef2b60dd800600055d3d43d42af9cc Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Wed, 6 May 2026 07:07:35 -0500 Subject: [PATCH 07/10] blockchain: Enforce trsy spend amt in input checks. Currently, enforcement that treasury spends commit to the amount they are spending in the first output and that the amount matches the value specified as the input amount in the first input happen when blocks are connected. The check is not dependent on the current treasury balance or anything that would require it to be limited to block connection only. It should ideally be in the per-transaction input checks so that it is also enforced early when a treasury spend is added to the mempool. To accomplish that, this moves that check, along with a couple of other additional sanity checks, into a separate method dedicated to checking treasury spend inputs so that it is more consistent with the other stake transaction type handling and invokes that method from CheckTransactionInputs. It also moves the other treasury spend checks after the call that checks and connects transactions in the stake tree to ensure the commitment is still verified prior to the other checks that depend on it. --- internal/blockchain/validate.go | 121 ++++++++++++++++++++++++-------- 1 file changed, 92 insertions(+), 29 deletions(-) diff --git a/internal/blockchain/validate.go b/internal/blockchain/validate.go index 8b0c26d17..0ec6e33ba 100644 --- a/internal/blockchain/validate.go +++ b/internal/blockchain/validate.go @@ -3092,6 +3092,68 @@ func checkRevocationInputs(tx *dcrutil.Tx, txHeight int64, view *UtxoViewpoint, false, 0, prevHeader, isTreasuryEnabled, isAutoRevocationsEnabled) } +// extractTreasurySpendCommitAmount extracts and decodes the amount from a +// treasury spend output commitment script. +// +// NOTE: The caller MUST have already determined that the provided script is the +// script from the first output of a treasury spend and that the script is well +// formed as required or the function may panic. +func extractTreasurySpendCommitAmount(script []byte) int64 { + // A treasury spend commitment output is an OP_RETURN script with a 32-byte + // data push that consists of an 8-byte little-endian amount to commit to + // and a 24-byte random value. Thus, 1 byte for the OP_RETURN + 1 byte for + // the data push means the encoded amount is at offset 2. + const startIdx = 2 + const endIdx = startIdx + 8 + encodedValue := script[startIdx:endIdx] + return int64(binary.LittleEndian.Uint64(encodedValue)) +} + +// checkTreasurySpendInputs performs a series of checks on the inputs to a +// treasury spend transaction. An example of some of the checks include +// verifying the input values are sane and the spend amount commitment in the +// first output matches the input amount. +// +// NOTE: The caller MUST have already determined that the provided transaction +// is a treasury spend or the function may panic. +func checkTreasurySpendInputs(msgTx *wire.MsgTx) error { + // Assert there is at least one input and one output. + if len(msgTx.TxIn) < 1 || len(msgTx.TxOut) < 1 { + panicf("attempt to check treasury spend inputs on tx %s which does "+ + "not appear to be a treasury spend (%d inputs, %d outputs)", + msgTx.TxHash(), len(msgTx.TxIn), len(msgTx.TxOut)) + } + + // Ensure all input amounts are sane. This is only a fast check for + // obviously invalid values. The expenditure policy is enforced separately. + for _, txIn := range msgTx.TxIn { + valueIn := txIn.ValueIn + if valueIn < 0 { + str := fmt.Sprintf("treasury spend has negative value of %v", + valueIn) + return ruleError(ErrBadTxInput, str) + } + if valueIn > dcrutil.MaxAmount { + str := fmt.Sprintf("treasury spend value of %v is higher than "+ + "max allowed value of %v", valueIn, dcrutil.MaxAmount) + return ruleError(ErrBadTxInput, str) + } + } + + // A valid treasury spend must specify the entire amount that the treasury + // is spending in the first input and commit to that amount in the script of + // the first output. + valueIn := msgTx.TxIn[0].ValueIn + commitmentAmt := extractTreasurySpendCommitAmount(msgTx.TxOut[0].PkScript) + if valueIn != commitmentAmt { + str := fmt.Sprintf("treasury spend input value %v does not match "+ + "spend amount commitment %v", valueIn, commitmentAmt) + return ruleError(ErrInvalidTSpendValueIn, str) + } + + return nil +} + // CheckTransactionInputs performs a series of checks on the inputs to a // transaction to ensure they are valid. An example of some of the checks // include verifying all inputs exist, ensuring the coinbase seasoning @@ -3177,11 +3239,11 @@ func CheckTransactionInputs(subsidyCache *standalone.SubsidyCache, // they have a valid signature from a sanctioned key. // // Also keep track of whether or not it is a treasury spend for later. - var isTSpend bool + var isTreasurySpend bool if isTreasuryEnabled { signature, pubKey, err := stake.CheckTSpend(msgTx) - isTSpend = err == nil - if isTSpend { + isTreasurySpend = err == nil + if isTreasurySpend { // The public key used to sign the treasury spend must be one of the // sanctioned pi keys. if !chainParams.PiKeyExists(pubKey) { @@ -3200,6 +3262,16 @@ func CheckTransactionInputs(subsidyCache *standalone.SubsidyCache, } } + // Perform additional checks on treasury spends such as verifying the input + // values are sane and the spend amount commitment in the first output + // matches the input amount. + if isTreasurySpend { + err := checkTreasurySpendInputs(tx.MsgTx()) + if err != nil { + return 0, err + } + } + // ------------------------------------------------------------------- // Decred general transaction testing (and a few stake exceptions). // ------------------------------------------------------------------- @@ -3218,7 +3290,7 @@ func CheckTransactionInputs(subsidyCache *standalone.SubsidyCache, } // idx can only be 0 in this case but check it anyway. - if (isTreasuryBase || isTSpend) && idx == 0 { + if (isTreasuryBase || isTreasurySpend) && idx == 0 { totalAtomIn += txIn.ValueIn continue } @@ -3977,6 +4049,9 @@ func (b *BlockChain) consensusScriptVerifyFlags(node *blockNode) (txscript.Scrip // block. It verifies that it is on a TVI, is within the correct window, has // not been mined before and that it doesn't overspend the treasury. This // function assumes that the treasury agenda is enabled. +// +// The caller MUST have already have already called [checkTreasurySpendInputs] +// on all treasury spends in the block and prior to calling this method. func (b *BlockChain) tspendChecks(prevNode *blockNode, block *dcrutil.Block) error { blockHeight := prevNode.height + 1 isTVI := standalone.IsTreasuryVoteInterval(uint64(blockHeight), @@ -4007,25 +4082,12 @@ func (b *BlockChain) tspendChecks(prevNode *blockNode, block *dcrutil.Block) err return ruleError(ErrInvalidTSpendWindow, str) } - // A valid TSPEND always stores the entire amount that the - // treasury is spending in the first TxIn. + // A valid treasury spend always stores the entire amount that the + // treasury is spending in the first input. It is safe to use since it + // has already been verified to match the commitment value. valueIn := stx.MsgTx().TxIn[0].ValueIn totalTSpendAmount += valueIn - // Verify that the ValueIn amount is identical to the LE - // encoded ValueIn in the OP_RETURN. Since the TSpend has been - // validated to be correct we simply index the bytes directly - // without additional checks. - leValueIn := stx.MsgTx().TxOut[0].PkScript[2 : 2+8] - valueInOpRet := int64(binary.LittleEndian.Uint64(leValueIn)) - if valueIn != valueInOpRet { - str := fmt.Sprintf("block contains TSpend transaction "+ - "(%v) that did not encode ValueIn correctly "+ - "got %v wanted %v", stx.Hash(), valueInOpRet, - valueIn) - return ruleError(ErrInvalidTSpendValueIn, str) - } - // Verify this TSpend hash has not been included in a // prior block. err := b.checkTSpendExists(prevNode, *stx.Hash()) @@ -4116,15 +4178,6 @@ func (b *BlockChain) checkConnectBlock(node *blockNode, block, parent *dcrutil.B } } - // Verify treasury spends. This is done relatively late because the - // database needs to be coherent. - if isTreasuryEnabled { - err = b.tspendChecks(node.parent, block) - if err != nil { - return err - } - } - // Don't run scripts if this node is both an ancestor of the assumed valid // block and an ancestor of the best header since the validity is verified // via the assumed valid node (all transactions are included in the merkle @@ -4201,6 +4254,16 @@ func (b *BlockChain) checkConnectBlock(node *blockNode, block, parent *dcrutil.B return err } + // Verify treasury spends. This is done relatively late because the + // database needs to be coherent and it depends on input checks already + // being done. + if isTreasuryEnabled { + err = b.tspendChecks(node.parent, block) + if err != nil { + return err + } + } + stakeTreeFees, err := getStakeTreeFees(b.subsidyCache, node.height, block.STransactions(), view, isTreasuryEnabled, subsidySplitVariant) if err != nil { From f219cff5b939ee661c8ec204c9b2986e8541b20b Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Wed, 6 May 2026 07:07:35 -0500 Subject: [PATCH 08/10] blockchain: Add a couple of trsybase corner tests. This adds a couple new tests to help ensure the treasurybase overall amount sum semantics are correct. --- internal/blockchain/treasury_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/internal/blockchain/treasury_test.go b/internal/blockchain/treasury_test.go index e80179e30..db309a5ee 100644 --- a/internal/blockchain/treasury_test.go +++ b/internal/blockchain/treasury_test.go @@ -2430,6 +2430,28 @@ func TestTreasuryBaseCorners(t *testing.T) { g.NextBlock("invalidout0", nil, outs[1:], corruptTreasurybaseValueOut) g.RejectTipBlock(ErrTreasurybaseOutValue) + // ------------------------------------------------------------------------- + // Treasury base spends more than input amount. + // ------------------------------------------------------------------------- + + g.SetTip(startTip) + g.NextBlock("invalidout1", nil, outs[1:], func(b *wire.MsgBlock) { + b.STransactions[0].TxOut[1].Value++ + }) + g.RejectTipBlock(ErrSpendTooHigh) + + // ------------------------------------------------------------------------- + // Treasury base spends the correct amount overall, but does not give the + // correct amount to the treasury. + // ------------------------------------------------------------------------- + + g.SetTip(startTip) + g.NextBlock("invalidout2", nil, outs[1:], func(b *wire.MsgBlock) { + b.STransactions[0].TxOut[0].Value-- + b.STransactions[0].TxOut[1].Value++ + }) + g.RejectTipBlock(ErrTreasurybaseOutValue) + // Note we can't hit the following errors in consensus: // * ErrFirstTxNotTreasurybase (missing OP_RETURN) // * ErrFirstTxNotTreasurybase (version) From befa0ba19fe6dcf7f65e243b9f5559cb31278877 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Wed, 6 May 2026 07:07:36 -0500 Subject: [PATCH 09/10] blockchain: Don't recalculate stake tree fees. The current code recalculates the total stake tree fees via a separate getStakeTreeFees function in order to pass them to the regular tree transaction and connection checks so they are accumulated as part of the total overall fees. This behavior is correct, but the total fees are already calculated when checking and connecting the stake tree transactions just before that, so there is no reason to calculate them again when they can simply be returned to the caller. This modifies checkTransactionsAndConnect accordingly and removes the separate method which is no longer necessary. --- internal/blockchain/validate.go | 127 +++++++------------------------- 1 file changed, 26 insertions(+), 101 deletions(-) diff --git a/internal/blockchain/validate.go b/internal/blockchain/validate.go index 0ec6e33ba..e16b679c4 100644 --- a/internal/blockchain/validate.go +++ b/internal/blockchain/validate.go @@ -3710,96 +3710,28 @@ func getStakeBaseAmounts(txs []*dcrutil.Tx, view *UtxoViewpoint) (int64, error) return totalOutputs - totalInputs, nil } -// getStakeTreeFees determines the amount of fees for in the stake tx tree of -// some node given a utxo view. -func getStakeTreeFees(subsidyCache *standalone.SubsidyCache, height int64, - txs []*dcrutil.Tx, view *UtxoViewpoint, isTreasuryEnabled bool, - subsidySplitVariant standalone.SubsidySplitVariant) (dcrutil.Amount, error) { - - totalInputs := int64(0) - totalOutputs := int64(0) - for _, tx := range txs { - msgTx := tx.MsgTx() - isSSGen := stake.IsSSGen(msgTx) - isTreasuryBase := isTreasuryEnabled && stake.IsTreasuryBase(msgTx) - isTreasurySpend := isTreasuryEnabled && stake.IsTSpend(msgTx) - - for i, in := range msgTx.TxIn { - // Ignore stakebases. - if isSSGen && i == 0 { - continue - } - - // Ignore treasury spends and treasurybases since they have no - // inputs. - if isTreasuryBase || isTreasurySpend { - continue - } - - txInOutpoint := in.PreviousOutPoint - txInHash := &txInOutpoint.Hash - utxoEntry, exists := view.entries[txInOutpoint] - if !exists || utxoEntry == nil { - str := fmt.Sprintf("couldn't find input tx "+ - "%v for stake tree fee calculation", - txInHash) - return 0, ruleError(ErrTicketUnavailable, str) - } - - originTxAtom := utxoEntry.Amount() - - totalInputs += originTxAtom - } - - for _, out := range msgTx.TxOut { - totalOutputs += out.Value - } - - // For votes, subtract the subsidy to determine actual fees. - if isSSGen { - // Subsidy aligns with the height we're voting on, not with the - // height of the current block. - totalOutputs -= subsidyCache.CalcStakeVoteSubsidyV3(height-1, - subsidySplitVariant) - } - - if isTreasurySpend { - totalOutputs -= msgTx.TxIn[0].ValueIn - } - - if isTreasuryBase { - totalOutputs -= msgTx.TxIn[0].ValueIn - } - } - - if totalInputs < totalOutputs { - str := fmt.Sprintf("negative cumulative fees found in stake " + - "tx tree") - return 0, ruleError(ErrStakeFees, str) - } - - return dcrutil.Amount(totalInputs - totalOutputs), nil -} - // checkTransactionsAndConnect is the local function used to check the // transaction inputs for a transaction list given a predetermined utxo view. // After ensuring the transaction is valid, the transaction is connected to the // utxo view. +// +// It returns the total fees paid by the transactions or 0 when the error is not +// nil. func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, node *blockNode, txs []*dcrutil.Tx, view *UtxoViewpoint, stxos *[]spentTxOut, stakeTree bool, - subsidySplitVariant standalone.SubsidySplitVariant) error { + subsidySplitVariant standalone.SubsidySplitVariant) (dcrutil.Amount, error) { isTreasuryEnabled, err := b.isTreasuryAgendaActive(node.parent) if err != nil { - return err + return 0, err } // Determine if the automatic ticket revocations agenda is active as of the // block being checked. isAutoRevocationsEnabled, err := b.isAutoRevocationsAgendaActive(node.parent) if err != nil { - return err + return 0, err } // Perform several checks on the inputs for each transaction. Also @@ -3830,18 +3762,18 @@ func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, numSigOps, err := CountTotalSigOps(tx, isCoinBase, isVote, view, isTreasuryEnabled) if err != nil { - return err + return 0, err } cumulativeSigOps, ok = addUnsigned(cumulativeSigOps, numSigOps) if !ok { str := fmt.Sprintf("tx %v causes block signature operation count "+ "to overflow", tx.Hash()) - return ruleError(ErrTooManySigOps, str) + return 0, ruleError(ErrTooManySigOps, str) } if cumulativeSigOps > MaxSigOpsPerBlock { str := fmt.Sprintf("block contains too many signature operations "+ "- got %v, max %v", cumulativeSigOps, MaxSigOpsPerBlock) - return ruleError(ErrTooManySigOps, str) + return 0, ruleError(ErrTooManySigOps, str) } // Perform a series of checks on the inputs to the transaction to ensure @@ -3859,7 +3791,7 @@ func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, isTreasuryEnabled, isAutoRevocationsEnabled, subsidySplitVariant) if err != nil { log.Tracef("CheckTransactionInputs failed; error returned: %v", err) - return err + return 0, err } // Sum the total fees and ensure we don't overflow the @@ -3867,7 +3799,7 @@ func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, lastTotalFees := totalFees totalFees += txFee if totalFees < lastTotalFees { - return ruleError(ErrBadFees, "total fees for block "+ + return 0, ruleError(ErrBadFees, "total fees for block "+ "overflows accumulator") } @@ -3885,13 +3817,13 @@ func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, err := view.connectRegularTransaction(tx, node.height, uint32(idx), inFlightRegularTx, stxos, isTreasuryEnabled) if err != nil { - return err + return 0, err } } else { err := view.connectStakeTransaction(tx, node.height, uint32(idx), stxos, isTreasuryEnabled) if err != nil { - return err + return 0, err } } } @@ -3939,7 +3871,7 @@ func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, errStr := fmt.Sprintf("bad coinbase subsidy in input;"+ " got %v, expected %v", coinbaseIn.ValueIn, subsidyWithoutFees) - return ruleError(ErrBadCoinbaseAmountIn, errStr) + return 0, ruleError(ErrBadCoinbaseAmountIn, errStr) } if totalAtomOutRegular > expAtomOut { @@ -3947,7 +3879,7 @@ func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, " pays %v which is more than expected value "+ "of %v", node.hash, totalAtomOutRegular, expAtomOut) - return ruleError(ErrBadCoinbaseValue, str) + return 0, ruleError(ErrBadCoinbaseValue, str) } } else { // TxTreeStake // When treasury is enabled check treasurybase value @@ -3958,7 +3890,7 @@ func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, str := fmt.Sprintf("empty tx tree stake, "+ "expected treasurybase at height %v", node.height) - return ruleError(ErrNoStakeTx, str) + return 0, ruleError(ErrNoStakeTx, str) } subsidyTax := b.subsidyCache.CalcTreasurySubsidy(node.height, node.voters, isTreasuryEnabled) @@ -3968,30 +3900,30 @@ func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, "subsidy in input; got %v, expected %v", treasurybaseIn.ValueIn, subsidyTax) - return ruleError(ErrBadTreasurybaseAmountIn, errStr) + return 0, ruleError(ErrBadTreasurybaseAmountIn, errStr) } } if len(txs) == 0 && node.height < b.chainParams.StakeValidationHeight { - return nil + return dcrutil.Amount(totalFees), nil } if len(txs) == 0 && node.height >= b.chainParams.StakeValidationHeight { str := fmt.Sprintf("empty tx tree stake in block " + "after stake validation height") - return ruleError(ErrNoStakeTx, str) + return 0, ruleError(ErrNoStakeTx, str) } err := checkStakeBaseAmounts(b.subsidyCache, node.height, txs, view, subsidySplitVariant) if err != nil { - return err + return 0, err } totalAtomOutStake, err := getStakeBaseAmounts(txs, view) if err != nil { - return err + return 0, err } var expAtomOut int64 @@ -4007,11 +3939,11 @@ func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, str := fmt.Sprintf("stakebase transactions for block pays %v "+ "which is more than expected value of %v", totalAtomOutStake, expAtomOut) - return ruleError(ErrBadStakebaseValue, str) + return 0, ruleError(ErrBadStakebaseValue, str) } } - return nil + return dcrutil.Amount(totalFees), nil } // consensusScriptVerifyFlags returns the script flags that must be used when @@ -4247,8 +4179,8 @@ func (b *BlockChain) checkConnectBlock(node *blockNode, block, parent *dcrutil.B } const stakeTreeTrue = true - err = b.checkTransactionsAndConnect(0, node, block.STransactions(), - view, stxos, stakeTreeTrue, subsidySplitVariant) + stakeTreeFees, err := b.checkTransactionsAndConnect(0, node, + block.STransactions(), view, stxos, stakeTreeTrue, subsidySplitVariant) if err != nil { log.Tracef("checkTransactionsAndConnect failed for stake tree: %v", err) return err @@ -4264,13 +4196,6 @@ func (b *BlockChain) checkConnectBlock(node *blockNode, block, parent *dcrutil.B } } - stakeTreeFees, err := getStakeTreeFees(b.subsidyCache, node.height, - block.STransactions(), view, isTreasuryEnabled, subsidySplitVariant) - if err != nil { - log.Tracef("getStakeTreeFees failed for stake tree: %v", err) - return err - } - // Enforce all relative lock times via sequence numbers for the stake // transaction tree once the stake vote for the agenda is active. var prevMedianTime time.Time @@ -4320,7 +4245,7 @@ func (b *BlockChain) checkConnectBlock(node *blockNode, block, parent *dcrutil.B } const stakeTreeFalse = false - err = b.checkTransactionsAndConnect(stakeTreeFees, node, + _, err = b.checkTransactionsAndConnect(stakeTreeFees, node, block.Transactions(), view, stxos, stakeTreeFalse, subsidySplitVariant) if err != nil { log.Tracef("checkTransactionsAndConnect failed for regular tree: %v", From 91b17a271ac7b2ebf35fa3079bd310ce466cc650 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Wed, 6 May 2026 07:07:37 -0500 Subject: [PATCH 10/10] blockchain: Cleanup tx input and fee overflow. This updates the transaction input and fee summing code to make use of the new consolidated add funcs with cleaner overflow detection. It also consolidates the input summing for all transaction types into a new closure instead of repeating the logic multiple times throughout the input checks function. Consolidating it makes it more readable and less error prone. Finally, while here, it consolidates, cleans up and slightly optimizes the input sum handling for the transaction types that do not have normal inputs. Namely, first the stakebase summing is changes to use the input value field instead of recalculating the subsidy to make it consistent with the treasurybase and treasury spend cases. This is safe because the code earlier in the function ensures the values matches the expected amounts. Second, it combines the checks for all three types since that change makes the checks for them identical. --- internal/blockchain/validate.go | 105 ++++++++++++++++++++++---------- 1 file changed, 73 insertions(+), 32 deletions(-) diff --git a/internal/blockchain/validate.go b/internal/blockchain/validate.go index e16b679c4..f4f89a30e 100644 --- a/internal/blockchain/validate.go +++ b/internal/blockchain/validate.go @@ -3165,7 +3165,7 @@ func checkTreasurySpendInputs(msgTx *wire.MsgTx) error { // that value. // // NOTE: The transaction MUST have already been sanity checked with the -// standalone.CheckTransactionSanity function prior to calling this function. +// [standalone.CheckTransactionSanity] function prior to calling this function. func CheckTransactionInputs(subsidyCache *standalone.SubsidyCache, tx *dcrutil.Tx, txHeight int64, view *UtxoViewpoint, checkFraudProof bool, chainParams *chaincfg.Params, prevHeader *wire.BlockHeader, @@ -3276,22 +3276,60 @@ func CheckTransactionInputs(subsidyCache *standalone.SubsidyCache, // Decred general transaction testing (and a few stake exceptions). // ------------------------------------------------------------------- - txHash := tx.Hash() + // sumTotalAtomIn is a convenience func that adds the provided amount to the + // total sum of all inputs while ensuring that the accumulator does not + // overflow. It also ensures the total does not exceed the max allowed per + // transaction. + // + // In practice, at the time this comment is being written, it is not + // possible to overflow the accumulator due to the combination of limits + // placed on the amounts and number of inputs possible per transaction, but + // be safe and check anyway in case that is no longer true at some point in + // the future. var totalAtomIn int64 - for idx, txIn := range msgTx.TxIn { - // Inputs won't exist for stakebase tx, so ignore them. - if isVote && idx == 0 { - // However, do add the reward amount. - _, heightVotingOn := stake.SSGenBlockVotedOn(msgTx) - stakeVoteSubsidy := subsidyCache.CalcStakeVoteSubsidyV3( - int64(heightVotingOn), subsidySplitVariant) - totalAtomIn += stakeVoteSubsidy - continue + sumTotalAtomIn := func(amount int64) error { + var ok bool + totalAtomIn, ok = addSigned(totalAtomIn, amount) + if !ok { + const str = "total value of all transaction inputs overflows " + + "accumulator" + return ruleError(ErrBadTxOutValue, str) + } + if totalAtomIn > dcrutil.MaxAmount { + str := fmt.Sprintf("total value of all transaction inputs is %v "+ + "which is higher than max allowed value of %v", totalAtomIn, + dcrutil.MaxAmount) + return ruleError(ErrBadTxOutValue, str) } - // idx can only be 0 in this case but check it anyway. - if (isTreasuryBase || isTreasurySpend) && idx == 0 { - totalAtomIn += txIn.ValueIn + return nil + } + + txHash := tx.Hash() + for idx, txIn := range msgTx.TxIn { + // The stakebase of votes, treasurybases, and treasuryspends do not have + // normal inputs, so handle them separately. + // + // Their input value commitments all contribute to the total input sum + // and are safe to use here because they have been proven to commit to + // correct values that are in a valid range previously. + // + // The input values correspond to the following: + // - Stakebases: the stake vote subsidy + // - Treasurybases: the treasury subsidy + // - Treasury spends: the amount to deduct from the treasury + // + // Treasurybases and treasury spends only have a single input, so their + // index can only be 0. That means their input sums could technically + // just be set directly. However, be paranoid and double check in case + // that ever changes in the future. + if idx == 0 && (isVote || isTreasuryBase || isTreasurySpend) { + // The total of all input amounts must not be more than the max + // allowed per transaction. Also, ensure the accumulator does not + // overflow. + if err := sumTotalAtomIn(txIn.ValueIn); err != nil { + return 0, err + } continue } @@ -3452,16 +3490,10 @@ func CheckTransactionInputs(subsidyCache *standalone.SubsidyCache, return 0, ruleError(ErrBadTxOutValue, str) } - // The total of all outputs must not be more than the max allowed per - // transaction. Also, we could potentially overflow the accumulator so - // check for overflow. - lastAtomIn := totalAtomIn - totalAtomIn += originTxAtom - if totalAtomIn < lastAtomIn || totalAtomIn > dcrutil.MaxAmount { - str := fmt.Sprintf("total value of all transaction inputs is %v "+ - "which is higher than max allowed value of %v", totalAtomIn, - dcrutil.MaxAmount) - return 0, ruleError(ErrBadTxOutValue, str) + // The total of all input amounts must not be more than the max allowed + // per transaction. Also, ensure the accumulator does not overflow. + if err := sumTotalAtomIn(originTxAtom); err != nil { + return 0, err } } @@ -3794,13 +3826,11 @@ func (b *BlockChain) checkTransactionsAndConnect(inputFees dcrutil.Amount, return 0, err } - // Sum the total fees and ensure we don't overflow the - // accumulator. - lastTotalFees := totalFees - totalFees += txFee - if totalFees < lastTotalFees { - return 0, ruleError(ErrBadFees, "total fees for block "+ - "overflows accumulator") + // Sum the total fees and ensure they do overflow the accumulator. + totalFees, ok = addSigned(totalFees, txFee) + if !ok { + const str = "total fees for block overflows accumulator" + return 0, ruleError(ErrBadFees, str) } // Update the view to mark all utxos spent by the transaction as spent @@ -4017,8 +4047,19 @@ func (b *BlockChain) tspendChecks(prevNode *blockNode, block *dcrutil.Block) err // A valid treasury spend always stores the entire amount that the // treasury is spending in the first input. It is safe to use since it // has already been verified to match the commitment value. + // + // The extra overflow checks could technically be avoided here because + // the treasury spends are a subset of all transactions in the tree + // which means the overall sum for all transactions would overflow and + // cause the block to be rejected anyway, but be paranoid to protect + // against refactors violating that assumption. + var ok bool valueIn := stx.MsgTx().TxIn[0].ValueIn - totalTSpendAmount += valueIn + totalTSpendAmount, ok = addSigned(totalTSpendAmount, valueIn) + if !ok { + const str = "total value of all treasury spends overflows accumulator" + return ruleError(ErrBadTxOutValue, str) + } // Verify this TSpend hash has not been included in a // prior block.