From 3ebded9ae70d86a60b506aa6023ef578d81a4ac1 Mon Sep 17 00:00:00 2001 From: Ori Newman Date: Mon, 29 Oct 2018 17:58:09 +0200 Subject: [PATCH] [DEV-255] create checkConnectToPastUTXO and move the required functionalities to it from checkConnectBlock1) * [DEV-255] create checkConnectToPastUTXO and move the required functionalities to it from checkConnectBlock * [DEV-255] get rid of checkConnectBlock * [DEV-255] rename pNode -> node * [DEV-255] add comment to describe ErrWithDiff --- blockdag/dag.go | 87 +++++++++++++++++++++----------------- blockdag/error.go | 4 ++ blockdag/error_test.go | 1 + blockdag/scriptval.go | 8 ++-- blockdag/scriptval_test.go | 9 +++- blockdag/utxoset.go | 8 ++-- blockdag/utxoset_test.go | 12 +++++- blockdag/validate.go | 71 +++++++++++-------------------- 8 files changed, 105 insertions(+), 95 deletions(-) diff --git a/blockdag/dag.go b/blockdag/dag.go index 6b074f5aa..2858cdbaf 100644 --- a/blockdag/dag.go +++ b/blockdag/dag.go @@ -5,6 +5,7 @@ package blockdag import ( + "errors" "fmt" "math" "sync" @@ -446,37 +447,21 @@ func (dag *BlockDAG) connectToDAG(node *blockNode, parentNodes blockSet, block * // Skip checks if node has already been fully validated. fastAdd := flags&BFFastAdd == BFFastAdd || dag.index.NodeStatus(node).KnownValid() - // Perform several checks to verify the block can be connected - // to the DAG without violating any rules and without actually - // connecting the block. - if !fastAdd { - err := dag.checkConnectBlock(node, block) - if err == nil { - dag.index.SetStatusFlags(node, statusValid) - } else if _, ok := err.(RuleError); ok { - dag.index.SetStatusFlags(node, statusValidateFailed) - } else { - return err - } - - // Intentionally ignore errors writing updated node status to DB. If - // it fails to write, it's not the end of the world. If the block is - // valid, we flush in connectBlock and if the block is invalid, the - // worst that can happen is we revalidate the block after a restart. - if writeErr := dag.index.flushToDB(); writeErr != nil { - log.Warnf("Error flushing block index changes to disk: %v", - writeErr) - } - - if err != nil { - return err - } + // Connect the block to the DAG. + err := dag.connectBlock(node, block, fastAdd) + if _, ok := err.(RuleError); ok { + dag.index.SetStatusFlags(node, statusValidateFailed) + } else { + return err } - // Connect the block to the DAG. - err := dag.connectBlock(node, block) - if err != nil { - return err + // Intentionally ignore errors writing updated node status to DB. If + // it fails to write, it's not the end of the world. If the block is + // invalid, the worst that can happen is we revalidate the block + // after a restart. + if writeErr := dag.index.flushToDB(); writeErr != nil { + log.Warnf("Error flushing block index changes to disk: %v", + writeErr) } return nil @@ -485,7 +470,14 @@ func (dag *BlockDAG) connectToDAG(node *blockNode, parentNodes blockSet, block * // connectBlock handles connecting the passed node/block to the DAG. // // This function MUST be called with the chain state lock held (for writes). -func (dag *BlockDAG) connectBlock(node *blockNode, block *util.Block) error { +func (dag *BlockDAG) connectBlock(node *blockNode, block *util.Block, fastAdd bool) error { + // The coinbase for the Genesis block is not spendable, so just return + // an error now. + if node.hash.IsEqual(dag.dagParams.GenesisHash) { + str := "the coinbase for the genesis block is not spendable" + return ruleError(ErrMissingTxOut, str) + } + // No warnings about unknown rules or versions until the DAG is // current. if dag.isCurrent() { @@ -503,7 +495,7 @@ func (dag *BlockDAG) connectBlock(node *blockNode, block *util.Block) error { } // Add the node to the virtual and update the UTXO set of the DAG. - utxoDiff, acceptedTxsData, err := dag.applyUTXOChanges(node, block) + utxoDiff, acceptedTxsData, err := dag.applyUTXOChanges(node, block, fastAdd) if err != nil { return err } @@ -570,7 +562,7 @@ func (dag *BlockDAG) connectBlock(node *blockNode, block *util.Block) error { // 5. Updates each of the tips' utxoDiff. // // This function MUST be called with the chain state lock held (for writes). -func (dag *BlockDAG) applyUTXOChanges(node *blockNode, block *util.Block) (utxoDiff *UTXODiff, acceptedTxData []*TxWithBlockHash, err error) { +func (dag *BlockDAG) applyUTXOChanges(node *blockNode, block *util.Block, fastAdd bool) (utxoDiff *UTXODiff, acceptedTxData []*TxWithBlockHash, err error) { // Prepare provisionalNodes for all the relevant nodes to avoid modifying the original nodes. // We avoid modifying the original nodes in this function because it could potentially // fail if the block is not valid, thus bringing all the affected nodes (and the virtual) @@ -581,9 +573,13 @@ func (dag *BlockDAG) applyUTXOChanges(node *blockNode, block *util.Block) (utxoD // Clone the virtual block so that we don't modify the existing one. virtualClone := dag.virtual.clone() - newBlockUTXO, acceptedTxData, err := newNodeProvisional.verifyAndBuildUTXO(virtualClone, dag.db) + newBlockUTXO, acceptedTxData, err := newNodeProvisional.verifyAndBuildUTXO(virtualClone, dag, fastAdd) if err != nil { - return nil, nil, fmt.Errorf("error verifying UTXO for %v: %s", node, err) + newErrString := fmt.Sprintf("error verifying UTXO for %v: %s", node, err) + if err, ok := err.(RuleError); ok { + return nil, nil, ruleError(err.ErrorCode, newErrString) + } + return nil, nil, errors.New(newErrString) } err = newNodeProvisional.updateParents(virtualClone, newBlockUTXO) @@ -598,13 +594,21 @@ func (dag *BlockDAG) applyUTXOChanges(node *blockNode, block *util.Block) (utxoD virtualNodeProvisional := provisionalSet.newProvisionalNode(&virtualClone.blockNode, true, nil) newVirtualUTXO, _, err := virtualNodeProvisional.pastUTXO(virtualClone, dag.db) if err != nil { - return nil, nil, fmt.Errorf("could not restore past UTXO for virtual %v: %s", virtualClone, err) + newErrString := fmt.Sprintf("could not restore past UTXO for virtual %v: %s", virtualClone, err) + if err, ok := err.(RuleError); ok { + return nil, nil, ruleError(err.ErrorCode, newErrString) + } + return nil, nil, errors.New(newErrString) } // Apply new utxoDiffs to all the tips err = updateTipsUTXO(virtualNodeProvisional.parents, virtualClone, newVirtualUTXO) if err != nil { - return nil, nil, fmt.Errorf("failed updating the tips' UTXO: %s", err) + newErrString := fmt.Sprintf("failed updating the tips' UTXO: %s", err) + if err, ok := err.(RuleError); ok { + return nil, nil, ruleError(err.ErrorCode, newErrString) + } + return nil, nil, errors.New(newErrString) } // It is now safe to meld the UTXO set to base. @@ -704,12 +708,19 @@ func (pns provisionalNodeSet) newProvisionalNode(node *blockNode, withRelatives } // verifyAndBuildUTXO verifies all transactions in the given block (in provisionalNode format) and builds its UTXO -func (p *provisionalNode) verifyAndBuildUTXO(virtual *virtualBlock, db database.DB) (utxoSet UTXOSet, acceptedTxData []*TxWithBlockHash, err error) { - pastUTXO, pastUTXOaccpetedTxData, err := p.pastUTXO(virtual, db) +func (p *provisionalNode) verifyAndBuildUTXO(virtual *virtualBlock, dag *BlockDAG, fastAdd bool) (utxoSet UTXOSet, acceptedTxData []*TxWithBlockHash, err error) { + pastUTXO, pastUTXOaccpetedTxData, err := p.pastUTXO(virtual, dag.db) if err != nil { return nil, nil, err } + if !fastAdd { + err = dag.checkConnectToPastUTXO(p, pastUTXO) + if err != nil { + return nil, nil, err + } + } + diff := NewUTXODiff() acceptedTxData = make([]*TxWithBlockHash, 0, len(pastUTXOaccpetedTxData)+len(p.transactions)) if len(pastUTXOaccpetedTxData) != 0 { diff --git a/blockdag/error.go b/blockdag/error.go index cef33dbb4..0522a1bed 100644 --- a/blockdag/error.go +++ b/blockdag/error.go @@ -205,6 +205,9 @@ const ( // current chain tip. This is not a block validation rule, but is required // for block proposals submitted via getblocktemplate RPC. ErrPrevBlockNotBest + + // ErrWithDiff indicates that there was an error with UTXOSet.WithDiff + ErrWithDiff ) // Map of ErrorCode values back to their constant names for pretty printing. @@ -248,6 +251,7 @@ var errorCodeStrings = map[ErrorCode]string{ ErrPreviousBlockUnknown: "ErrPreviousBlockUnknown", ErrInvalidAncestorBlock: "ErrInvalidAncestorBlock", ErrPrevBlockNotBest: "ErrPrevBlockNotBest", + ErrWithDiff: "ErrWithDiff", } // String returns the ErrorCode as a human-readable name. diff --git a/blockdag/error_test.go b/blockdag/error_test.go index 079e2fc46..4b0f52cba 100644 --- a/blockdag/error_test.go +++ b/blockdag/error_test.go @@ -54,6 +54,7 @@ func TestErrorCodeStringer(t *testing.T) { {ErrPreviousBlockUnknown, "ErrPreviousBlockUnknown"}, {ErrInvalidAncestorBlock, "ErrInvalidAncestorBlock"}, {ErrPrevBlockNotBest, "ErrPrevBlockNotBest"}, + {ErrWithDiff, "ErrWithDiff"}, {0xffff, "Unknown ErrorCode (65535)"}, } diff --git a/blockdag/scriptval.go b/blockdag/scriptval.go index a227afb03..c20a3a871 100644 --- a/blockdag/scriptval.go +++ b/blockdag/scriptval.go @@ -205,15 +205,15 @@ func ValidateTransactionScripts(tx *util.Tx, utxoSet UTXOSet, flags txscript.Scr // checkBlockScripts executes and validates the scripts for all transactions in // the passed block using multiple goroutines. -func checkBlockScripts(block *util.Block, utxoSet UTXOSet, scriptFlags txscript.ScriptFlags, sigCache *txscript.SigCache) error { +func checkBlockScripts(block *provisionalNode, utxoSet UTXOSet, scriptFlags txscript.ScriptFlags, sigCache *txscript.SigCache) error { // Collect all of the transaction inputs and required information for // validation for all transactions in the block into a single slice. numInputs := 0 - for _, tx := range block.Transactions() { + for _, tx := range block.transactions { numInputs += len(tx.MsgTx().TxIn) } txValItems := make([]*txValidateItem, 0, numInputs) - for _, tx := range block.Transactions() { + for _, tx := range block.transactions { for txInIdx, txIn := range tx.MsgTx().TxIn { // Skip coinbases. if txIn.PreviousOutPoint.Index == math.MaxUint32 { @@ -237,7 +237,7 @@ func checkBlockScripts(block *util.Block, utxoSet UTXOSet, scriptFlags txscript. } elapsed := time.Since(start) - log.Tracef("block %v took %v to verify", block.Hash(), elapsed) + log.Tracef("block %v took %v to verify", block.original.hash, elapsed) return nil } diff --git a/blockdag/scriptval_test.go b/blockdag/scriptval_test.go index 3a68ef1f6..725244e8d 100644 --- a/blockdag/scriptval_test.go +++ b/blockdag/scriptval_test.go @@ -40,8 +40,15 @@ func TestCheckBlockScripts(t *testing.T) { return } + node := &provisionalNode{ + original: &blockNode{ + hash: *blocks[0].Hash(), + }, + transactions: blocks[0].Transactions(), + } + scriptFlags := txscript.ScriptNoFlags - err = checkBlockScripts(blocks[0], utxoSet, scriptFlags, nil) + err = checkBlockScripts(node, utxoSet, scriptFlags, nil) if err != nil { t.Errorf("Transaction script validation failed: %v\n", err) return diff --git a/blockdag/utxoset.go b/blockdag/utxoset.go index 2506fbcb0..0faf6a7e3 100644 --- a/blockdag/utxoset.go +++ b/blockdag/utxoset.go @@ -257,7 +257,7 @@ func (d *UTXODiff) WithDiff(diff *UTXODiff) (*UTXODiff, error) { result.toAdd.add(outPoint, utxoEntry) } if diff.toAdd.contains(outPoint) { - return nil, errors.New("WithDiff: transaction both in d.toAdd and in other.toAdd") + return nil, ruleError(ErrWithDiff, "WithDiff: transaction both in d.toAdd and in other.toAdd") } } @@ -270,7 +270,7 @@ func (d *UTXODiff) WithDiff(diff *UTXODiff) (*UTXODiff, error) { result.toRemove.add(outPoint, utxoEntry) } if diff.toRemove.contains(outPoint) { - return nil, errors.New("WithDiff: transaction both in d.toRemove and in other.toRemove") + return nil, ruleError(ErrWithDiff, "WithDiff: transaction both in d.toRemove and in other.toRemove") } } @@ -367,9 +367,9 @@ func diffFromTx(u UTXOSet, tx *wire.MsgTx, containingNode *blockNode) (*UTXODiff if entry, ok := u.Get(txIn.PreviousOutPoint); ok { diff.toRemove.add(txIn.PreviousOutPoint, entry) } else { - return nil, fmt.Errorf( + return nil, ruleError(ErrMissingTxOut, fmt.Sprintf( "Transaction %s is invalid because spends outpoint %s that is not in utxo set", - tx.TxHash(), txIn.PreviousOutPoint) + tx.TxHash(), txIn.PreviousOutPoint)) } } } diff --git a/blockdag/utxoset_test.go b/blockdag/utxoset_test.go index 2855875e8..0ca45c407 100644 --- a/blockdag/utxoset_test.go +++ b/blockdag/utxoset_test.go @@ -848,6 +848,14 @@ func TestApplyUTXOChanges(t *testing.T) { } defer teardownFunc() + dag.dagParams.CoinbaseMaturity = 1 + // Create artificial checkpoint in order to prevent script validation for blocks that comes before it + dag.checkpoints = []dagconfig.Checkpoint{ + { + Height: 1000, + }, + } + cbTx, err := createCoinbaseTx(1, 1) if err != nil { t.Errorf("createCoinbaseTx: %v", err) @@ -878,7 +886,7 @@ func TestApplyUTXOChanges(t *testing.T) { initBlockNode(&node1, blockHeader, setFromSlice(dag.genesis), dagconfig.MainNetParams.K) //Checks that dag.applyUTXOChanges fails because we don't allow a transaction to spend another transaction from the same block - _, _, err = dag.applyUTXOChanges(&node1, block1) + _, _, err = dag.applyUTXOChanges(&node1, block1, false) if err == nil { t.Errorf("applyUTXOChanges expected an error\n") } @@ -905,7 +913,7 @@ func TestApplyUTXOChanges(t *testing.T) { initBlockNode(&node2, blockHeader, setFromSlice(dag.genesis), dagconfig.MainNetParams.K) //Checks that dag.applyUTXOChanges doesn't fail because all of its transaction are dependant on transactions from previous blocks - _, _, err = dag.applyUTXOChanges(&node2, block2) + _, _, err = dag.applyUTXOChanges(&node2, block2, false) if err != nil { t.Errorf("applyUTXOChanges: %v", err) } diff --git a/blockdag/validate.go b/blockdag/validate.go index 9753e476f..075a881e0 100644 --- a/blockdag/validate.go +++ b/blockdag/validate.go @@ -778,11 +778,11 @@ func (dag *BlockDAG) checkBlockContext(block *util.Block, parents blockSet, blue // http://r6.ca/blog/20120206T005236Z.html. // // This function MUST be called with the chain state lock held (for reads). -func (dag *BlockDAG) ensureNoDuplicateTx(node *blockNode, block *util.Block) error { +func ensureNoDuplicateTx(block *provisionalNode, utxoSet UTXOSet) error { // Fetch utxos for all of the transaction ouputs in this block. // Typically, there will not be any utxos for any of the outputs. fetchSet := make(map[wire.OutPoint]struct{}) - for _, tx := range block.Transactions() { + for _, tx := range block.transactions { prevOut := wire.OutPoint{Hash: *tx.Hash()} for txOutIdx := range tx.MsgTx().TxOut { prevOut.Index = uint32(txOutIdx) @@ -793,7 +793,7 @@ func (dag *BlockDAG) ensureNoDuplicateTx(node *blockNode, block *util.Block) err // Duplicate transactions are only allowed if the previous transaction // is fully spent. for outpoint := range fetchSet { - utxo, ok := dag.GetUTXOEntry(outpoint) + utxo, ok := utxoSet.Get(outpoint) if ok { str := fmt.Sprintf("tried to overwrite transaction %v "+ "at block height %d that is not fully spent", @@ -905,12 +905,8 @@ func CheckTransactionInputs(tx *util.Tx, txHeight int32, utxoSet UTXOSet, dagPar return txFeeInSatoshi, nil } -// checkConnectBlock performs several checks to confirm connecting the passed -// block to the chain represented by the passed view does not violate any rules. -// In addition, the passed view is updated to spend all of the referenced -// outputs and add all of the new utxos created by block. Thus, the view will -// represent the state of the chain as if the block were actually connected and -// consequently the best hash for the view is also updated to passed block. +// checkConnectToPastUTXO performs several checks to confirm connecting the passed +// block to the DAG represented by the passed view does not violate any rules. // // An example of some of the checks performed are ensuring connecting the block // would not cause any duplicate transaction hashes for old transactions that @@ -918,29 +914,10 @@ func CheckTransactionInputs(tx *util.Tx, txHeight int32, utxoSet UTXOSet, dagPar // signature operations per block, invalid values in relation to the expected // block subsidy, or fail transaction script validation. // -// The CheckConnectBlockTemplate function makes use of this function to perform -// the bulk of its work. The only difference is this function accepts a node -// which may or may not require reorganization to connect it to the main chain -// whereas CheckConnectBlockTemplate creates a new node which specifically -// connects to the end of the current main chain and then calls this function -// with that node. -// -// This function MUST be called with the chain state lock held (for writes). -func (dag *BlockDAG) checkConnectBlock(node *blockNode, block *util.Block) error { - // If the side chain blocks end up in the database, a call to - // CheckBlockSanity should be done here in case a previous version - // allowed a block that is no longer valid. However, since the - // implementation only currently uses memory for the side chain blocks, - // it isn't currently necessary. +// This function MUST be called with the dag state lock held (for writes). +func (dag *BlockDAG) checkConnectToPastUTXO(block *provisionalNode, pastUTXO UTXOSet) error { - // The coinbase for the Genesis block is not spendable, so just return - // an error now. - if node.hash.IsEqual(dag.dagParams.GenesisHash) { - str := "the coinbase for the genesis block is not spendable" - return ruleError(ErrMissingTxOut, str) - } - - err := dag.ensureNoDuplicateTx(node, block) + err := ensureNoDuplicateTx(block, pastUTXO) if err != nil { return err } @@ -951,9 +928,8 @@ func (dag *BlockDAG) checkConnectBlock(node *blockNode, block *util.Block) error // expands the count to include a precise count of pay-to-script-hash // signature operations in each of the input transaction public key // scripts. - transactions := block.Transactions() totalSigOps := 0 - for i, tx := range transactions { + for i, tx := range block.transactions { numsigOps := CountSigOps(tx) // Since the first (and only the first) transaction has // already been verified to be a coinbase transaction, @@ -961,7 +937,7 @@ func (dag *BlockDAG) checkConnectBlock(node *blockNode, block *util.Block) error // 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, i == 0, dag.virtual.utxoSet) + numP2SHSigOps, err := CountP2SHSigOps(tx, i == 0, pastUTXO) if err != nil { return err } @@ -987,8 +963,8 @@ func (dag *BlockDAG) checkConnectBlock(node *blockNode, block *util.Block) error // against all the inputs when the signature operations are out of // bounds. var totalFees uint64 - for _, tx := range transactions { - txFee, err := CheckTransactionInputs(tx, node.height, dag.virtual.utxoSet, + for _, tx := range block.transactions { + txFee, err := CheckTransactionInputs(tx, block.original.height, pastUTXO, dag.dagParams) if err != nil { return err @@ -1010,10 +986,10 @@ func (dag *BlockDAG) checkConnectBlock(node *blockNode, block *util.Block) error // errors here because those error conditions would have already been // caught by checkTransactionSanity. var totalSatoshiOut uint64 - for _, txOut := range transactions[0].MsgTx().TxOut { + for _, txOut := range block.transactions[0].MsgTx().TxOut { totalSatoshiOut += txOut.Value } - expectedSatoshiOut := CalcBlockSubsidy(node.height, dag.dagParams) + + expectedSatoshiOut := CalcBlockSubsidy(block.original.height, dag.dagParams) + totalFees if totalSatoshiOut > expectedSatoshiOut { str := fmt.Sprintf("coinbase transaction for block pays %v "+ @@ -1030,7 +1006,7 @@ func (dag *BlockDAG) checkConnectBlock(node *blockNode, block *util.Block) error // portion of block handling. checkpoint := dag.LatestCheckpoint() runScripts := true - if checkpoint != nil && node.height <= checkpoint.Height { + if checkpoint != nil && block.original.height <= checkpoint.Height { runScripts = false } @@ -1038,20 +1014,20 @@ func (dag *BlockDAG) checkConnectBlock(node *blockNode, block *util.Block) error // We obtain the MTP of the *previous* block in order to // determine if transactions in the current block are final. - medianTime := node.selectedParent.CalcPastMedianTime() + medianTime := block.original.selectedParent.CalcPastMedianTime() // We also enforce the relative sequence number based // lock-times within the inputs of all transactions in this // candidate block. - for _, tx := range block.Transactions() { + for _, tx := range block.transactions { // A transaction can only be included within a block // once the sequence locks of *all* its inputs are // active. - sequenceLock, err := dag.calcSequenceLock(node, dag.virtual.utxoSet, tx, false) + sequenceLock, err := dag.calcSequenceLock(block.original, pastUTXO, tx, false) if err != nil { return err } - if !SequenceLockActive(sequenceLock, node.height, + if !SequenceLockActive(sequenceLock, block.original.height, medianTime) { str := fmt.Sprintf("block contains " + "transaction whose input sequence " + @@ -1065,7 +1041,7 @@ func (dag *BlockDAG) checkConnectBlock(node *blockNode, block *util.Block) error // expensive ECDSA signature check scripts. Doing this last helps // prevent CPU exhaustion attacks. if runScripts { - err := checkBlockScripts(block, dag.virtual.utxoSet, scriptFlags, dag.sigCache) + err := checkBlockScripts(block, pastUTXO, scriptFlags, dag.sigCache) if err != nil { return err } @@ -1122,6 +1098,9 @@ func (dag *BlockDAG) CheckConnectBlockTemplate(block *util.Block) error { return err } - newNode := newBlockNode(&header, dag.virtual.tips(), dag.dagParams.K) - return dag.checkConnectBlock(newNode, block) + newProvisionalNode := &provisionalNode{ + original: newBlockNode(&header, dag.virtual.tips(), dag.dagParams.K), + transactions: block.Transactions(), + } + return dag.checkConnectToPastUTXO(newProvisionalNode, dag.UTXOSet()) }