From 20e24e3a238ce2e59576978762e479e994d21822 Mon Sep 17 00:00:00 2001 From: Ori Newman Date: Mon, 1 Apr 2019 13:03:13 +0300 Subject: [PATCH] [NOD-83] modify blocks parents children only after validation (#235) * [NOD-83] Modify block parents' children only after validation * [NOD-83] Creating CheckConnectBlockTemplateWithLock with RLock * [NOD-83] Put updateParentsChildren inside updateParents * [NOD-83] create updateParentsDiffs function --- blockdag/accept.go | 10 ---------- blockdag/blocknode.go | 15 ++++----------- blockdag/dag.go | 24 ++++++++++++------------ blockdag/validate.go | 14 ++++++++++---- blockdag/validate_test.go | 5 ----- mining/mining.go | 13 +------------ server/rpc/rpcserver.go | 2 +- 7 files changed, 28 insertions(+), 55 deletions(-) diff --git a/blockdag/accept.go b/blockdag/accept.go index 789c174b1..7cfcc5a45 100644 --- a/blockdag/accept.go +++ b/blockdag/accept.go @@ -62,14 +62,6 @@ func (dag *BlockDAG) maybeAcceptBlock(block *util.Block, flags BehaviorFlags) er blockHeader := &block.MsgBlock().Header newNode := newBlockNode(blockHeader, parents, dag.dagParams.K) newNode.status = statusDataStored - // newBlockNode adds node into children maps of its parents. So it must be - // removed in case of error. - isOk := false - defer func() { - if !isOk { - newNode.detachFromParents() - } - }() dag.index.AddNode(newNode) err = dag.index.flushToDB() @@ -91,8 +83,6 @@ func (dag *BlockDAG) maybeAcceptBlock(block *util.Block, flags BehaviorFlags) er dag.sendNotification(NTBlockAdded, block) dag.dagLock.Lock() - isOk = true - return nil } diff --git a/blockdag/blocknode.go b/blockdag/blocknode.go index 6263cd86d..fe38aff41 100644 --- a/blockdag/blocknode.go +++ b/blockdag/blocknode.go @@ -139,11 +139,6 @@ func initBlockNode(node *blockNode, blockHeader *wire.BlockHeader, parents block node.timestamp = blockHeader.Timestamp.Unix() node.hashMerkleRoot = blockHeader.HashMerkleRoot node.idMerkleRoot = blockHeader.IDMerkleRoot - - // update parents to point to new node - for _, p := range parents { - p.children[node.hash] = node - } } if len(parents) > 0 { @@ -174,12 +169,10 @@ func newBlockNode(blockHeader *wire.BlockHeader, parents blockSet, phantomK uint return &node } -// newBlockNode adds node into children maps of its parents. So it must be -// removed in case of error. -func (node *blockNode) detachFromParents() { - // remove node from parents - for _, p := range node.parents { - delete(p.children, node.hash) +// updateParentsChildren updates the node's parents to point to new node +func (node *blockNode) updateParentsChildren() { + for _, parent := range node.parents { + parent.children.add(node) } } diff --git a/blockdag/dag.go b/blockdag/dag.go index 6495ae38b..c2f4eb35a 100644 --- a/blockdag/dag.go +++ b/blockdag/dag.go @@ -731,24 +731,21 @@ func (dag *BlockDAG) NextBlockFeeTransaction() (*wire.MsgTx, error) { func (dag *BlockDAG) applyDAGChanges(node *blockNode, block *util.Block, newBlockUTXO UTXOSet, fastAdd bool) ( virtualUTXODiff *UTXODiff, err error) { - // Clone the virtual block so that we don't modify the existing one. - virtualClone := dag.virtual.clone() - - if err = node.updateParents(virtualClone, newBlockUTXO); err != nil { + if err = node.updateParents(dag.virtual, newBlockUTXO); err != nil { return nil, fmt.Errorf("failed updating parents of %s: %s", node, err) } - // Update the virtual block's children (the DAG tips) to include the new block. - virtualClone.AddTip(node) + // Update the virtual block's parents (the DAG tips) to include the new block. + dag.virtual.AddTip(node) // Build a UTXO set for the new virtual block - newVirtualUTXO, _, err := virtualClone.blockNode.pastUTXO(virtualClone, dag.db) + newVirtualUTXO, _, err := dag.virtual.blockNode.pastUTXO(dag.virtual, dag.db) if err != nil { - return nil, fmt.Errorf("could not restore past UTXO for virtual %s: %s", virtualClone, err) + return nil, fmt.Errorf("could not restore past UTXO for virtual %s: %s", dag.virtual, err) } // Apply new utxoDiffs to all the tips - err = updateTipsUTXO(virtualClone.parents, virtualClone, newVirtualUTXO) + err = updateTipsUTXO(dag.virtual.parents, dag.virtual, newVirtualUTXO) if err != nil { return nil, fmt.Errorf("failed updating the tips' UTXO: %s", err) } @@ -765,9 +762,6 @@ func (dag *BlockDAG) applyDAGChanges(node *blockNode, block *util.Block, newBloc } dag.index.SetStatusFlags(node, statusValid) - // It is now safe to apply the new virtual block - dag.virtual = virtualClone - // And now we can update the finality point of the DAG (if required) dag.lastFinalityPoint = dag.newFinalityPoint(node) @@ -960,6 +954,12 @@ func (node *blockNode) restoreUTXO(virtual *virtualBlock) (UTXOSet, error) { // updateParents adds this block to the children sets of its parents // and updates the diff of any parent whose DiffChild is this block func (node *blockNode) updateParents(virtual *virtualBlock, newBlockUTXO UTXOSet) error { + node.updateParentsChildren() + return node.updateParentsDiffs(virtual, newBlockUTXO) +} + +// updateParentsDiffs updates the diff of any parent whose DiffChild is this block +func (node *blockNode) updateParentsDiffs(virtual *virtualBlock, newBlockUTXO UTXOSet) error { virtualDiffFromNewBlock, err := virtual.utxoSet.diffFrom(newBlockUTXO) if err != nil { return err diff --git a/blockdag/validate.go b/blockdag/validate.go index 3a2ea644d..2d90784b9 100644 --- a/blockdag/validate.go +++ b/blockdag/validate.go @@ -1183,14 +1183,21 @@ func countSpentOutputs(block *util.Block) int { return numSpent } -// CheckConnectBlockTemplate fully validates that connecting the passed block to +// CheckConnectBlockTemplateWithLock fully validates that connecting the passed block to // the DAG does not violate any consensus rules, aside from the proof of // work requirement. The block must connect to the current tip of the main dag. // // This function is safe for concurrent access. +func (dag *BlockDAG) CheckConnectBlockTemplateWithLock(block *util.Block) error { + dag.dagLock.RLock() + defer dag.dagLock.RUnlock() + return dag.CheckConnectBlockTemplate(block) +} + +// CheckConnectBlockTemplate fully validates that connecting the passed block to +// the DAG does not violate any consensus rules, aside from the proof of +// work requirement. The block must connect to the current tip of the main dag. func (dag *BlockDAG) CheckConnectBlockTemplate(block *util.Block) error { - dag.dagLock.Lock() - defer dag.dagLock.Unlock() // Skip the proof of work check as this is just a block template. flags := BFNoPoWCheck @@ -1222,7 +1229,6 @@ func (dag *BlockDAG) CheckConnectBlockTemplate(block *util.Block) error { } templateNode := newBlockNode(&header, dag.virtual.tips(), dag.dagParams.K) - defer templateNode.detachFromParents() _, err = dag.checkConnectToPastUTXO(templateNode, dag.UTXOSet(), block.Transactions(), false) diff --git a/blockdag/validate_test.go b/blockdag/validate_test.go index 11ee54dc6..8a491795c 100644 --- a/blockdag/validate_test.go +++ b/blockdag/validate_test.go @@ -121,11 +121,6 @@ func TestCheckConnectBlockTemplate(t *testing.T) { "block 4: %v", err) } - blockNode3 := dag.index.LookupNode(blocks[3].Hash()) - if blockNode3.children.containsHash(blocks[4].Hash()) { - t.Errorf("Block 4 wasn't successfully detached as a child from block3") - } - // Block 3a should fail to connect since does not build on chain tip. err = dag.CheckConnectBlockTemplate(blocks[5]) if err == nil { diff --git a/mining/mining.go b/mining/mining.go index 22ab4be65..5da8018a5 100644 --- a/mining/mining.go +++ b/mining/mining.go @@ -385,15 +385,7 @@ func NewBlkTmplGenerator(policy *Policy, params *dagconfig.Params, // ----------------------------------- -- func (g *BlkTmplGenerator) NewBlockTemplate(payToAddress util.Address) (*BlockTemplate, error) { g.dag.RLock() - isDagLocked := true - // We need to read-unlock the DAG before calling CheckConnectBlockTemplate - // Therefore the deferred function is only relevant in cases where an - // error is returned before calling CheckConnectBlockTemplate - defer func() { - if isDagLocked { - g.dag.RUnlock() - } - }() + defer g.dag.RUnlock() // Extend the most recently known best block. nextBlockHeight := g.dag.Height() + 1 @@ -683,9 +675,6 @@ func (g *BlkTmplGenerator) NewBlockTemplate(payToAddress util.Address) (*BlockTe block := util.NewBlock(&msgBlock) block.SetHeight(nextBlockHeight) - g.dag.RUnlock() - isDagLocked = false - if err := g.dag.CheckConnectBlockTemplate(block); err != nil { return nil, err } diff --git a/server/rpc/rpcserver.go b/server/rpc/rpcserver.go index f57533550..521a42f26 100644 --- a/server/rpc/rpcserver.go +++ b/server/rpc/rpcserver.go @@ -2153,7 +2153,7 @@ func handleGetBlockTemplateProposal(s *Server, request *btcjson.TemplateRequest) return "bad-parentblk", nil } - if err := s.cfg.DAG.CheckConnectBlockTemplate(block); err != nil { + if err := s.cfg.DAG.CheckConnectBlockTemplateWithLock(block); err != nil { if _, ok := err.(blockdag.RuleError); !ok { errStr := fmt.Sprintf("Failed to process block proposal: %s", err) log.Error(errStr)