[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

@@ -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
 	}

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 _, blockTxsAcceptanceData := range multiBlockTxsAcceptanceData {
-		for _, txAcceptance := range blockTxsAcceptanceData {
+		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 _, blockTxsAcceptanceData := range multiBlockTxsAcceptanceData {
-		for _, txAcceptanceData := range blockTxsAcceptanceData {
+		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
+// It's a slice of the block's blues block IDs and their transaction acceptance data
-type MultiBlockTxsAcceptanceData map[daghash.Hash]BlockTxsAcceptanceData
+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

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: &params,
+	})
+	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, &params, []*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

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))
+	serializableData := make(serializableMultiBlockTxsAcceptanceData, len(multiBlockTxsAcceptanceData))
-	for hash, blockTxsAcceptanceData := range txsAcceptanceData {
+	for i, blockTxsAcceptanceData := range multiBlockTxsAcceptanceData {
-		serializableBlockData := make(serializableBlockTxsAcceptanceData, len(blockTxsAcceptanceData))
+		serializableBlockData := serializableBlockTxsAcceptanceData{
-		for i, txAcceptanceData := range blockTxsAcceptanceData {
+			BlockHash:        blockTxsAcceptanceData.BlockHash,
-			serializableBlockData[i] = serializableTxAcceptanceData{
+			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))
+	multiBlockTxsAcceptanceData := make(blockdag.MultiBlockTxsAcceptanceData, len(serializedData))
-	for hash, serializableBlockData := range serializedData {
+	for i, serializableBlockData := range serializedData {
-		blockTxsAcceptanceData := make(blockdag.BlockTxsAcceptanceData, len(serializableBlockData))
+		blockTxsAcceptanceData := blockdag.BlockTxsAcceptanceData{
-		for i, txData := range serializableBlockData {
+			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
 }

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{
-		{
+		BlockHash: *hash,
-			Tx:         util.NewTx(wire.NewNativeMsgTx(wire.TxVersion, []*wire.TxIn{txIn1}, []*wire.TxOut{txOut1})),
+		TxAcceptanceData: []blockdag.TxAcceptanceData{
-			IsAccepted: true,
+			{
-		},
+				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,
+			{
+				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")
 	}
 }

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
 		}
 	}

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 {
+		for _, blockAcceptanceData := range acceptanceData {
-			acceptedTxIds := make([]string, 0, len(blockAcceptanceData))
+			acceptedTxIds := make([]string, 0, len(blockAcceptanceData.TxAcceptanceData))
-			for _, txAcceptanceData := range blockAcceptanceData {
+			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)