From 6828f623b4a9b9a38951a3055ff0ebdb9d2004ac Mon Sep 17 00:00:00 2001 From: stasatdaglabs <39559713+stasatdaglabs@users.noreply.github.com> Date: Wed, 13 Nov 2019 12:28:52 +0200 Subject: [PATCH] [NOD-395] Fix a crash in diffFromAcceptanceData caused by wrong order of iteration over blocks (463) * [NOD-395] Write a test for the diffFromAcceptanceData crash. * [NOD-395] Converted MultiBlockTxsAcceptanceData into a slice. * [NOD-395] Fix failing test. * [NOD-395] Populate multiBlockTxsAcceptanceData bottom-to-top. * [NOD-395] Add comment to FindAcceptanceData. * [NOD-395] Remove no-longer relevant note about probability in TestOrderInDiffFromAcceptanceData. --- blockdag/coinbase.go | 10 ++-- blockdag/dag.go | 54 +++++++++++------ blockdag/external_dag_test.go | 72 +++++++++++++++++++++++ blockdag/indexers/acceptanceindex.go | 41 ++++++++----- blockdag/indexers/acceptanceindex_test.go | 25 ++++---- blockdag/indexers/txindex.go | 14 ++--- server/rpc/common.go | 8 +-- 7 files changed, 163 insertions(+), 61 deletions(-) diff --git a/blockdag/coinbase.go b/blockdag/coinbase.go index 26f4dd094..aea329d19 100644 --- a/blockdag/coinbase.go +++ b/blockdag/coinbase.go @@ -220,7 +220,7 @@ func coinbaseInputAndOutputForBlueBlock(dag *BlockDAG, blueBlock *blockNode, txsAcceptanceData MultiBlockTxsAcceptanceData, feeData map[daghash.Hash]compactFeeData) ( *wire.TxIn, *wire.TxOut, error) { - blockTxsAcceptanceData, ok := txsAcceptanceData[*blueBlock.hash] + blockTxsAcceptanceData, ok := txsAcceptanceData.FindAcceptanceData(blueBlock.hash) if !ok { return nil, nil, errors.Errorf("No txsAcceptanceData for block %s", blueBlock.hash) } @@ -229,10 +229,10 @@ func coinbaseInputAndOutputForBlueBlock(dag *BlockDAG, blueBlock *blockNode, return nil, nil, errors.Errorf("No feeData for block %s", blueBlock.hash) } - if len(blockTxsAcceptanceData) != blockFeeData.Len() { + if len(blockTxsAcceptanceData.TxAcceptanceData) != blockFeeData.Len() { return nil, nil, errors.Errorf( "length of accepted transaction data(%d) and fee data(%d) is not equal for block %s", - len(blockTxsAcceptanceData), blockFeeData.Len(), blueBlock.hash) + len(blockTxsAcceptanceData.TxAcceptanceData), blockFeeData.Len(), blueBlock.hash) } txIn := &wire.TxIn{ @@ -247,7 +247,7 @@ func coinbaseInputAndOutputForBlueBlock(dag *BlockDAG, blueBlock *blockNode, totalFees := uint64(0) feeIterator := blockFeeData.iterator() - for _, txAcceptanceData := range blockTxsAcceptanceData { + for _, txAcceptanceData := range blockTxsAcceptanceData.TxAcceptanceData { fee, err := feeIterator.next() if err != nil { return nil, nil, errors.Errorf("Error retrieving fee from compactFeeData iterator: %s", err) @@ -264,7 +264,7 @@ func coinbaseInputAndOutputForBlueBlock(dag *BlockDAG, blueBlock *blockNode, } // the ScriptPubKey for the coinbase is parsed from the coinbase payload - scriptPubKey, _, err := DeserializeCoinbasePayload(blockTxsAcceptanceData[0].Tx.MsgTx()) + scriptPubKey, _, err := DeserializeCoinbasePayload(blockTxsAcceptanceData.TxAcceptanceData[0].Tx.MsgTx()) if err != nil { return nil, nil, err } diff --git a/blockdag/dag.go b/blockdag/dag.go index 6d924390b..f7a86e741 100644 --- a/blockdag/dag.go +++ b/blockdag/dag.go @@ -493,10 +493,10 @@ func (dag *BlockDAG) addBlock(node *blockNode, parentNodes blockSet, return chainUpdates, nil } -func calculateAcceptedIDMerkleRoot(txsAcceptanceData MultiBlockTxsAcceptanceData) *daghash.Hash { +func calculateAcceptedIDMerkleRoot(multiBlockTxsAcceptanceData MultiBlockTxsAcceptanceData) *daghash.Hash { var acceptedTxs []*util.Tx - for _, blockTxsAcceptanceData := range txsAcceptanceData { - for _, txAcceptance := range blockTxsAcceptanceData { + for _, blockTxsAcceptanceData := range multiBlockTxsAcceptanceData { + for _, txAcceptance := range blockTxsAcceptanceData.TxAcceptanceData { if !txAcceptance.IsAccepted { continue } @@ -951,11 +951,11 @@ func (node *blockNode) diffFromTxs(pastUTXO UTXOSet, transactions []*util.Tx) (* // diffFromAccpetanceData creates a diff that "updates" the blue scores of the given // UTXOSet with the node's blueScore according to the given acceptance data. -func (node *blockNode) diffFromAcceptanceData(pastUTXO UTXOSet, blockTxsAcceptanceDatas MultiBlockTxsAcceptanceData) (*UTXODiff, error) { +func (node *blockNode) diffFromAcceptanceData(pastUTXO UTXOSet, multiBlockTxsAcceptanceData MultiBlockTxsAcceptanceData) (*UTXODiff, error) { diff := NewUTXODiff() - for _, blockTxsAcceptanceData := range blockTxsAcceptanceDatas { - for _, txAcceptanceData := range blockTxsAcceptanceData { + for _, blockTxsAcceptanceData := range multiBlockTxsAcceptanceData { + for _, txAcceptanceData := range blockTxsAcceptanceData.TxAcceptanceData { if txAcceptanceData.IsAccepted { acceptanceDiff, err := pastUTXO.diffFromAcceptedTx(txAcceptanceData.Tx.MsgTx(), node.blueScore) if err != nil { @@ -1029,13 +1029,26 @@ type TxAcceptanceData struct { IsAccepted bool } -// BlockTxsAcceptanceData stores all transactions in a block with an indication +// BlockTxsAcceptanceData stores all transactions in a block with an indication // if they were accepted or not by some other block -type BlockTxsAcceptanceData []TxAcceptanceData +type BlockTxsAcceptanceData struct { + BlockHash daghash.Hash + TxAcceptanceData []TxAcceptanceData +} // MultiBlockTxsAcceptanceData stores data about which transactions were accepted by a block -// It's a map from the block's blues block IDs to the transaction acceptance data -type MultiBlockTxsAcceptanceData map[daghash.Hash]BlockTxsAcceptanceData +// It's a slice of the block's blues block IDs and their transaction acceptance data +type MultiBlockTxsAcceptanceData []BlockTxsAcceptanceData + +// FindAcceptanceData finds the BlockTxsAcceptanceData that matches blockHash +func (data MultiBlockTxsAcceptanceData) FindAcceptanceData(blockHash *daghash.Hash) (*BlockTxsAcceptanceData, bool) { + for _, acceptanceData := range data { + if acceptanceData.BlockHash.IsEqual(blockHash) { + return &acceptanceData, true + } + } + return nil, false +} func genesisPastUTXO(virtual *virtualBlock) UTXOSet { // The genesis has no past UTXO, so we create an empty UTXO @@ -1076,14 +1089,21 @@ func (node *blockNode) fetchBlueBlocks(db database.DB) ([]*util.Block, error) { // Purposefully ignoring failures - these are just unaccepted transactions // Writing down which transactions were accepted or not in txsAcceptanceData func (node *blockNode) applyBlueBlocks(selectedParentUTXO UTXOSet, blueBlocks []*util.Block) ( - pastUTXO UTXOSet, txsAcceptanceData MultiBlockTxsAcceptanceData, err error) { + pastUTXO UTXOSet, multiBlockTxsAcceptanceData MultiBlockTxsAcceptanceData, err error) { pastUTXO = selectedParentUTXO - txsAcceptanceData = MultiBlockTxsAcceptanceData{} + multiBlockTxsAcceptanceData = MultiBlockTxsAcceptanceData{} - for _, blueBlock := range blueBlocks { + // Add blueBlocks to multiBlockTxsAcceptanceData bottom-to-top instead of + // top-to-bottom. This is so that anyone who iterates over it would process + // blocks (and transactions) in their order of appearance in the DAG. + for i := len(blueBlocks) - 1; i >= 0; i-- { + blueBlock := blueBlocks[i] transactions := blueBlock.Transactions() - blockTxsAcceptanceData := make(BlockTxsAcceptanceData, len(transactions)) + blockTxsAcceptanceData := BlockTxsAcceptanceData{ + BlockHash: *blueBlock.Hash(), + TxAcceptanceData: make([]TxAcceptanceData, len(transactions)), + } isSelectedParent := blueBlock.Hash().IsEqual(node.selectedParent.hash) for i, tx := range blueBlock.Transactions() { var isAccepted bool @@ -1095,12 +1115,12 @@ func (node *blockNode) applyBlueBlocks(selectedParentUTXO UTXOSet, blueBlocks [] return nil, nil, err } } - blockTxsAcceptanceData[i] = TxAcceptanceData{Tx: tx, IsAccepted: isAccepted} + blockTxsAcceptanceData.TxAcceptanceData[i] = TxAcceptanceData{Tx: tx, IsAccepted: isAccepted} } - txsAcceptanceData[*blueBlock.Hash()] = blockTxsAcceptanceData + multiBlockTxsAcceptanceData = append(multiBlockTxsAcceptanceData, blockTxsAcceptanceData) } - return pastUTXO, txsAcceptanceData, nil + return pastUTXO, multiBlockTxsAcceptanceData, nil } // updateParents adds this block to the children sets of its parents diff --git a/blockdag/external_dag_test.go b/blockdag/external_dag_test.go index 94ca4c21d..a39ad7cf2 100644 --- a/blockdag/external_dag_test.go +++ b/blockdag/external_dag_test.go @@ -314,6 +314,78 @@ func TestChainedTransactions(t *testing.T) { } } +// TestOrderInDiffFromAcceptanceData makes sure that the order of transactions in +// dag.diffFromAcceptanceData is such that if txA is spent by txB then txA is processed +// before txB. +func TestOrderInDiffFromAcceptanceData(t *testing.T) { + // Create a new database and DAG instance to run tests against. + params := dagconfig.SimNetParams + params.K = math.MaxUint32 + dag, teardownFunc, err := blockdag.DAGSetup("TestOrderInDiffFromAcceptanceData", blockdag.Config{ + DAGParams: ¶ms, + }) + if err != nil { + t.Fatalf("Failed to setup DAG instance: %v", err) + } + defer teardownFunc() + dag.TestSetCoinbaseMaturity(0) + + createBlock := func(previousBlock *util.Block) *util.Block { + // Prepare a transaction that spends the previous block's coinbase transaction + var txs []*wire.MsgTx + if !previousBlock.IsGenesis() { + previousCoinbaseTx := previousBlock.MsgBlock().Transactions[0] + signatureScript, err := txscript.PayToScriptHashSignatureScript(blockdag.OpTrueScript, nil) + if err != nil { + t.Fatalf("TestOrderInDiffFromAcceptanceData: Failed to build signature script: %s", err) + } + txIn := &wire.TxIn{ + PreviousOutpoint: wire.Outpoint{TxID: *previousCoinbaseTx.TxID(), Index: 0}, + SignatureScript: signatureScript, + Sequence: wire.MaxTxInSequenceNum, + } + txOut := &wire.TxOut{ + ScriptPubKey: blockdag.OpTrueScript, + Value: uint64(1), + } + txs = append(txs, wire.NewNativeMsgTx(wire.TxVersion, []*wire.TxIn{txIn}, []*wire.TxOut{txOut})) + } + + // Create the block + msgBlock, err := mining.PrepareBlockForTest(dag, ¶ms, []*daghash.Hash{previousBlock.Hash()}, txs, false) + if err != nil { + t.Fatalf("TestOrderInDiffFromAcceptanceData: Failed to prepare block: %s", err) + } + + // Add the block to the DAG + newBlock := util.NewBlock(msgBlock) + isOrphan, delay, err := dag.ProcessBlock(newBlock, blockdag.BFNoPoWCheck) + if err != nil { + t.Errorf("TestOrderInDiffFromAcceptanceData: %s", err) + } + if delay != 0 { + t.Fatalf("TestOrderInDiffFromAcceptanceData: block is too far in the future") + } + if isOrphan { + t.Fatalf("TestOrderInDiffFromAcceptanceData: block got unexpectedly orphaned") + } + return newBlock + } + + // Create two block chains starting from the genesis block. Every time a block is added + // one of the chains is selected as the selected parent chain while all the blocks in + // the other chain (and their transactions) get accepted by the new virtual. If the + // transactions in the non-selected parent chain get processed in the wrong order then + // diffFromAcceptanceData panics. + blockAmountPerChain := 100 + chainATip := util.NewBlock(params.GenesisBlock) + chainBTip := chainATip + for i := 0; i < blockAmountPerChain; i++ { + chainATip = createBlock(chainATip) + chainBTip = createBlock(chainBTip) + } +} + // TestGasLimit tests the gas limit rules func TestGasLimit(t *testing.T) { params := dagconfig.SimNetParams diff --git a/blockdag/indexers/acceptanceindex.go b/blockdag/indexers/acceptanceindex.go index b4c8a1460..f3c677082 100644 --- a/blockdag/indexers/acceptanceindex.go +++ b/blockdag/indexers/acceptanceindex.go @@ -167,23 +167,29 @@ type serializableTxAcceptanceData struct { IsAccepted bool } -type serializableBlockTxsAcceptanceData []serializableTxAcceptanceData +type serializableBlockTxsAcceptanceData struct { + BlockHash daghash.Hash + TxAcceptanceData []serializableTxAcceptanceData +} -type serializableMultiBlockTxsAcceptanceData map[daghash.Hash]serializableBlockTxsAcceptanceData +type serializableMultiBlockTxsAcceptanceData []serializableBlockTxsAcceptanceData func serializeMultiBlockTxsAcceptanceData( - txsAcceptanceData blockdag.MultiBlockTxsAcceptanceData) ([]byte, error) { + multiBlockTxsAcceptanceData blockdag.MultiBlockTxsAcceptanceData) ([]byte, error) { // Convert MultiBlockTxsAcceptanceData to a serializable format - serializableData := make(serializableMultiBlockTxsAcceptanceData, len(txsAcceptanceData)) - for hash, blockTxsAcceptanceData := range txsAcceptanceData { - serializableBlockData := make(serializableBlockTxsAcceptanceData, len(blockTxsAcceptanceData)) - for i, txAcceptanceData := range blockTxsAcceptanceData { - serializableBlockData[i] = serializableTxAcceptanceData{ + serializableData := make(serializableMultiBlockTxsAcceptanceData, len(multiBlockTxsAcceptanceData)) + for i, blockTxsAcceptanceData := range multiBlockTxsAcceptanceData { + serializableBlockData := serializableBlockTxsAcceptanceData{ + BlockHash: blockTxsAcceptanceData.BlockHash, + TxAcceptanceData: make([]serializableTxAcceptanceData, len(blockTxsAcceptanceData.TxAcceptanceData)), + } + for i, txAcceptanceData := range blockTxsAcceptanceData.TxAcceptanceData { + serializableBlockData.TxAcceptanceData[i] = serializableTxAcceptanceData{ MsgTx: *txAcceptanceData.Tx.MsgTx(), IsAccepted: txAcceptanceData.IsAccepted, } } - serializableData[hash] = serializableBlockData + serializableData[i] = serializableBlockData } // Serialize @@ -208,18 +214,21 @@ func deserializeMultiBlockTxsAcceptanceData( } // Convert serializable format to MultiBlockTxsAcceptanceData - txsAcceptanceData := make(blockdag.MultiBlockTxsAcceptanceData, len(serializedData)) - for hash, serializableBlockData := range serializedData { - blockTxsAcceptanceData := make(blockdag.BlockTxsAcceptanceData, len(serializableBlockData)) - for i, txData := range serializableBlockData { + multiBlockTxsAcceptanceData := make(blockdag.MultiBlockTxsAcceptanceData, len(serializedData)) + for i, serializableBlockData := range serializedData { + blockTxsAcceptanceData := blockdag.BlockTxsAcceptanceData{ + BlockHash: serializableBlockData.BlockHash, + TxAcceptanceData: make([]blockdag.TxAcceptanceData, len(serializableBlockData.TxAcceptanceData)), + } + for i, txData := range serializableBlockData.TxAcceptanceData { msgTx := txData.MsgTx - blockTxsAcceptanceData[i] = blockdag.TxAcceptanceData{ + blockTxsAcceptanceData.TxAcceptanceData[i] = blockdag.TxAcceptanceData{ Tx: util.NewTx(&msgTx), IsAccepted: txData.IsAccepted, } } - txsAcceptanceData[hash] = blockTxsAcceptanceData + multiBlockTxsAcceptanceData[i] = blockTxsAcceptanceData } - return txsAcceptanceData, nil + return multiBlockTxsAcceptanceData, nil } diff --git a/blockdag/indexers/acceptanceindex_test.go b/blockdag/indexers/acceptanceindex_test.go index 5ebbeca5d..be8a6572e 100644 --- a/blockdag/indexers/acceptanceindex_test.go +++ b/blockdag/indexers/acceptanceindex_test.go @@ -18,8 +18,6 @@ import ( ) func TestAcceptanceIndexSerializationAndDeserialization(t *testing.T) { - txsAcceptanceData := blockdag.MultiBlockTxsAcceptanceData{} - // Create test data hash, _ := daghash.NewHashFromStr("1111111111111111111111111111111111111111111111111111111111111111") txIn1 := &wire.TxIn{SignatureScript: []byte{1}, PreviousOutpoint: wire.Outpoint{Index: 1}, Sequence: 0} @@ -27,19 +25,22 @@ func TestAcceptanceIndexSerializationAndDeserialization(t *testing.T) { txOut1 := &wire.TxOut{ScriptPubKey: []byte{1}, Value: 10} txOut2 := &wire.TxOut{ScriptPubKey: []byte{2}, Value: 20} blockTxsAcceptanceData := blockdag.BlockTxsAcceptanceData{ - { - Tx: util.NewTx(wire.NewNativeMsgTx(wire.TxVersion, []*wire.TxIn{txIn1}, []*wire.TxOut{txOut1})), - IsAccepted: true, - }, - { - Tx: util.NewTx(wire.NewNativeMsgTx(wire.TxVersion, []*wire.TxIn{txIn2}, []*wire.TxOut{txOut2})), - IsAccepted: false, + BlockHash: *hash, + TxAcceptanceData: []blockdag.TxAcceptanceData{ + { + Tx: util.NewTx(wire.NewNativeMsgTx(wire.TxVersion, []*wire.TxIn{txIn1}, []*wire.TxOut{txOut1})), + IsAccepted: true, + }, + { + Tx: util.NewTx(wire.NewNativeMsgTx(wire.TxVersion, []*wire.TxIn{txIn2}, []*wire.TxOut{txOut2})), + IsAccepted: false, + }, }, } - txsAcceptanceData[*hash] = blockTxsAcceptanceData + multiBlockTxsAcceptanceData := blockdag.MultiBlockTxsAcceptanceData{blockTxsAcceptanceData} // Serialize - serializedTxsAcceptanceData, err := serializeMultiBlockTxsAcceptanceData(txsAcceptanceData) + serializedTxsAcceptanceData, err := serializeMultiBlockTxsAcceptanceData(multiBlockTxsAcceptanceData) if err != nil { t.Fatalf("TestAcceptanceIndexSerializationAndDeserialization: serialization failed: %s", err) } @@ -51,7 +52,7 @@ func TestAcceptanceIndexSerializationAndDeserialization(t *testing.T) { } // Check that they're the same - if !reflect.DeepEqual(txsAcceptanceData, deserializedTxsAcceptanceData) { + if !reflect.DeepEqual(multiBlockTxsAcceptanceData, deserializedTxsAcceptanceData) { t.Fatalf("TestAcceptanceIndexSerializationAndDeserialization: original data and deseralize data aren't equal") } } diff --git a/blockdag/indexers/txindex.go b/blockdag/indexers/txindex.go index b4fb473c7..df216588e 100644 --- a/blockdag/indexers/txindex.go +++ b/blockdag/indexers/txindex.go @@ -153,7 +153,7 @@ func dbFetchFirstTxRegion(dbTx database.Tx, txID *daghash.TxID) (*database.Block // dbAddTxIndexEntries uses an existing database transaction to add a // transaction index entry for every transaction in the passed block. -func dbAddTxIndexEntries(dbTx database.Tx, block *util.Block, blockID uint64, txsAcceptanceData blockdag.MultiBlockTxsAcceptanceData) error { +func dbAddTxIndexEntries(dbTx database.Tx, block *util.Block, blockID uint64, multiBlockTxsAcceptanceData blockdag.MultiBlockTxsAcceptanceData) error { // The offset and length of the transactions within the serialized // block. txLocs, err := block.TxLoc() @@ -179,12 +179,12 @@ func dbAddTxIndexEntries(dbTx database.Tx, block *util.Block, blockID uint64, tx includingBlocksOffset += includingBlocksIndexKeyEntrySize } - for includingBlockHash, blockTxsAcceptanceData := range txsAcceptanceData { + for _, blockTxsAcceptanceData := range multiBlockTxsAcceptanceData { var includingBlockID uint64 - if includingBlockHash.IsEqual(block.Hash()) { + if blockTxsAcceptanceData.BlockHash.IsEqual(block.Hash()) { includingBlockID = blockID } else { - includingBlockID, err = blockdag.DBFetchBlockIDByHash(dbTx, &includingBlockHash) + includingBlockID, err = blockdag.DBFetchBlockIDByHash(dbTx, &blockTxsAcceptanceData.BlockHash) if err != nil { return err } @@ -192,7 +192,7 @@ func dbAddTxIndexEntries(dbTx database.Tx, block *util.Block, blockID uint64, tx serializedIncludingBlockID := blockdag.SerializeBlockID(includingBlockID) - for _, txAcceptanceData := range blockTxsAcceptanceData { + for _, txAcceptanceData := range blockTxsAcceptanceData.TxAcceptanceData { err = dbPutAcceptingBlocksEntry(dbTx, txAcceptanceData.Tx.ID(), blockID, serializedIncludingBlockID) if err != nil { return err @@ -207,13 +207,13 @@ func updateTxsAcceptedByVirtual(virtualTxsAcceptanceData blockdag.MultiBlockTxsA // Initialize a new txsAcceptedByVirtual entries := 0 for _, blockTxsAcceptanceData := range virtualTxsAcceptanceData { - entries += len(blockTxsAcceptanceData) + entries += len(blockTxsAcceptanceData.TxAcceptanceData) } txsAcceptedByVirtual = make(map[daghash.TxID]bool, entries) // Copy virtualTxsAcceptanceData to txsAcceptedByVirtual for _, blockTxsAcceptanceData := range virtualTxsAcceptanceData { - for _, txAcceptanceData := range blockTxsAcceptanceData { + for _, txAcceptanceData := range blockTxsAcceptanceData.TxAcceptanceData { txsAcceptedByVirtual[*txAcceptanceData.Tx.ID()] = true } } diff --git a/server/rpc/common.go b/server/rpc/common.go index cbbe9f1cd..714e93d81 100644 --- a/server/rpc/common.go +++ b/server/rpc/common.go @@ -310,15 +310,15 @@ func collectChainBlocks(s *Server, hashes []*daghash.Hash) ([]btcjson.ChainBlock } acceptedBlocks := make([]btcjson.AcceptedBlock, 0, len(acceptanceData)) - for blockHash, blockAcceptanceData := range acceptanceData { - acceptedTxIds := make([]string, 0, len(blockAcceptanceData)) - for _, txAcceptanceData := range blockAcceptanceData { + for _, blockAcceptanceData := range acceptanceData { + acceptedTxIds := make([]string, 0, len(blockAcceptanceData.TxAcceptanceData)) + for _, txAcceptanceData := range blockAcceptanceData.TxAcceptanceData { if txAcceptanceData.IsAccepted { acceptedTxIds = append(acceptedTxIds, txAcceptanceData.Tx.ID().String()) } } acceptedBlock := btcjson.AcceptedBlock{ - Hash: blockHash.String(), + Hash: blockAcceptanceData.BlockHash.String(), AcceptedTxIDs: acceptedTxIds, } acceptedBlocks = append(acceptedBlocks, acceptedBlock)