Correct reading of serialized height in coinbase.

This commit corrects the reading of the serialized height in coinbase
transactions for block height of version 2 or greater.  On mainnet, the
serialized height is always 3 bytes and will continue to be so for
something like  another ~159 years, so there was no issue with mainnet.
However on testnet, there are some version 2 blocks which are low enough
in the chain to only take 2 bytes to serialize.

In addition, this commit adds a full tests for the relavant function
including negative tests and variable length serialized lengths for block
heights.

Closes #1.

internal_test.go

@@ -13,6 +13,7 @@ package btcchain
 
 import (
 	"github.com/conformal/btcutil"
+	"github.com/conformal/btcwire"
 	"time"
 )
 
@@ -33,3 +34,9 @@ func TstSetCoinbaseMaturity(maturity int64) {
 func TstTimeSorter(times []time.Time) timeSorter {
 	return timeSorter(times)
 }
+
+// TstCheckSerializedHeight makes the internal checkSerializedHeight function
+// available to the test package.
+func TstCheckSerializedHeight(coinbaseTx *btcwire.MsgTx, wantHeight int64) error {
+	return checkSerializedHeight(coinbaseTx, wantHeight)
+}

test_coverage.txt

@@ -1,27 +1,28 @@
 
+github.com/conformal/btcchain/validate.go	 checkSerializedHeight			 100.00% (17/17)
 github.com/conformal/btcchain/chain.go		 BlockChain.removeOrphanBlock		 100.00% (16/16)
 github.com/conformal/btcchain/validate.go	 countSigOps				 100.00% (8/8)
 github.com/conformal/btcchain/checkpoints.go	 init					 100.00% (6/6)
 github.com/conformal/btcchain/difficulty.go	 ShaHashToBig				 100.00% (5/5)
 github.com/conformal/btcchain/merkle.go		 hashMerkleBranches			 100.00% (5/5)
-github.com/conformal/btcchain/merkle.go		 nextPowerOfTwo				 100.00% (4/4)
 github.com/conformal/btcchain/chain.go		 newBlockNode				 100.00% (4/4)
+github.com/conformal/btcchain/merkle.go		 nextPowerOfTwo				 100.00% (4/4)
 github.com/conformal/btcchain/process.go	 BlockChain.blockExists			 100.00% (3/3)
 github.com/conformal/btcchain/checkpoints.go	 newShaHashFromStr			 100.00% (2/2)
 github.com/conformal/btcchain/chain.go		 New					 100.00% (2/2)
-github.com/conformal/btcchain/log.go		 DisableLog				 100.00% (1/1)
+github.com/conformal/btcchain/log.go		 init					 100.00% (1/1)
-github.com/conformal/btcchain/validate.go	 calcBlockSubsidy			 100.00% (1/1)
 github.com/conformal/btcchain/timesorter.go	 timeSorter.Less			 100.00% (1/1)
 github.com/conformal/btcchain/timesorter.go	 timeSorter.Swap			 100.00% (1/1)
-github.com/conformal/btcchain/timesorter.go	 timeSorter.Len				 100.00% (1/1)
-github.com/conformal/btcchain/log.go		 init					 100.00% (1/1)
 github.com/conformal/btcchain/params.go		 BlockChain.chainParams			 100.00% (1/1)
 github.com/conformal/btcchain/checkpoints.go	 BlockChain.DisableCheckpoints		 100.00% (1/1)
+github.com/conformal/btcchain/validate.go	 calcBlockSubsidy			 100.00% (1/1)
+github.com/conformal/btcchain/timesorter.go	 timeSorter.Len				 100.00% (1/1)
+github.com/conformal/btcchain/log.go		 DisableLog				 100.00% (1/1)
 github.com/conformal/btcchain/merkle.go		 BuildMerkleTreeStore			 94.12% (16/17)
 github.com/conformal/btcchain/txlookup.go	 disconnectTransactions			 93.75% (15/16)
 github.com/conformal/btcchain/txlookup.go	 fetchTxListMain			 93.33% (14/15)
-github.com/conformal/btcchain/process.go	 BlockChain.processOrphans		 92.86% (13/14)
 github.com/conformal/btcchain/chain.go		 BlockChain.getReorganizeNodes		 92.86% (13/14)
+github.com/conformal/btcchain/process.go	 BlockChain.processOrphans		 92.86% (13/14)
 github.com/conformal/btcchain/chain.go		 BlockChain.connectBestChain		 90.00% (27/30)
 github.com/conformal/btcchain/chain.go		 BlockChain.getPrevNodeFromBlock	 88.89% (8/9)
 github.com/conformal/btcchain/scriptval.go	 ValidateTransactionScripts		 88.24% (30/34)
@@ -64,27 +65,26 @@ github.com/conformal/btcchain/blocklocator.go	 BlockChain.BlockLocatorFromHash
 github.com/conformal/btcchain/checkpoints.go	 BlockChain.IsCheckpointCandidate	 0.00% (0/32)
 github.com/conformal/btcchain/validate.go	 countP2SHSigOps			 0.00% (0/24)
 github.com/conformal/btcchain/chain.go		 BlockChain.GenerateInitialIndex	 0.00% (0/17)
-github.com/conformal/btcchain/txlookup.go	 BlockChain.FetchTransactionStore	 0.00% (0/15)
 github.com/conformal/btcchain/difficulty.go	 BlockChain.calcEasiestDifficulty	 0.00% (0/15)
-github.com/conformal/btcchain/validate.go	 checkSerializedHeight			 0.00% (0/12)
+github.com/conformal/btcchain/txlookup.go	 BlockChain.FetchTransactionStore	 0.00% (0/15)
 github.com/conformal/btcchain/difficulty.go	 BlockChain.findPrevTestNetDifficulty	 0.00% (0/12)
 github.com/conformal/btcchain/chain.go		 BlockChain.removeBlockNode		 0.00% (0/12)
 github.com/conformal/btcchain/chain.go		 BlockChain.GetOrphanRoot		 0.00% (0/11)
 github.com/conformal/btcchain/chain.go		 BlockChain.IsCurrent			 0.00% (0/9)
 github.com/conformal/btcchain/chain.go		 removeChildNode			 0.00% (0/8)
-github.com/conformal/btcchain/chain.go		 BlockChain.HaveInventory		 0.00% (0/7)
 github.com/conformal/btcchain/log.go		 SetLogWriter				 0.00% (0/7)
+github.com/conformal/btcchain/chain.go		 BlockChain.HaveInventory		 0.00% (0/7)
 github.com/conformal/btcchain/blocklocator.go	 BlockChain.LatestBlockLocator		 0.00% (0/6)
-github.com/conformal/btcchain/chain.go		 BlockChain.IsKnownOrphan		 0.00% (0/5)
 github.com/conformal/btcchain/checkpoints.go	 isNonstandardTransaction		 0.00% (0/5)
-github.com/conformal/btcchain/checkpoints.go	 BlockChain.checkpointData		 0.00% (0/4)
+github.com/conformal/btcchain/chain.go		 BlockChain.IsKnownOrphan		 0.00% (0/5)
 github.com/conformal/btcchain/validate.go	 isTransactionSpent			 0.00% (0/4)
-github.com/conformal/btcchain/notifications.go	 NotificationType.String		 0.00% (0/3)
+github.com/conformal/btcchain/checkpoints.go	 BlockChain.checkpointData		 0.00% (0/4)
 github.com/conformal/btcchain/chain.go		 addChildrenWork			 0.00% (0/3)
-github.com/conformal/btcchain/log.go		 newLogClosure				 0.00% (0/1)
+github.com/conformal/btcchain/notifications.go	 NotificationType.String		 0.00% (0/3)
 github.com/conformal/btcchain/log.go		 UseLogger				 0.00% (0/1)
-github.com/conformal/btcchain/chain.go		 BlockChain.DisableVerify		 0.00% (0/1)
+github.com/conformal/btcchain/log.go		 newLogClosure				 0.00% (0/1)
 github.com/conformal/btcchain/log.go		 logClosure.String			 0.00% (0/1)
 github.com/conformal/btcchain/process.go	 RuleError.Error			 0.00% (0/1)
-github.com/conformal/btcchain			 -------------------------------------	 53.97% (625/1158)
+github.com/conformal/btcchain/chain.go		 BlockChain.DisableVerify		 0.00% (0/1)
+github.com/conformal/btcchain			 -------------------------------------	 55.20% (642/1163)
 

validate.go

@@ -498,17 +498,26 @@ func (b *BlockChain) checkBlockSanity(block *btcutil.Block) error {
 // transaction starts with the serialized block height of wantHeight.
 func checkSerializedHeight(coinbaseTx *btcwire.MsgTx, wantHeight int64) error {
 	sigScript := coinbaseTx.TxIn[0].SignatureScript
-	if len(sigScript) < 4 {
+	if len(sigScript) < 1 {
 		str := "the coinbase signature script for blocks of " +
 			"version %d or greater must start with the " +
-			"serialized block height"
+			"length of the serialized block height"
 		str = fmt.Sprintf(str, serializedHeightVersion)
 		return RuleError(str)
 	}
 
-	serializedHeightBytes := make([]byte, 4, 4)
+	serializedLen := int(sigScript[0])
-	copy(serializedHeightBytes, sigScript[1:4])
+	if len(sigScript[1:]) < serializedLen {
-	serializedHeight := binary.LittleEndian.Uint32(serializedHeightBytes)
+		str := "the coinbase signature script for blocks of " +
+			"version %d or greater must start with the " +
+			"serialized block height"
+		str = fmt.Sprintf(str, serializedLen)
+		return RuleError(str)
+	}
+
+	serializedHeightBytes := make([]byte, 8, 8)
+	copy(serializedHeightBytes, sigScript[1:serializedLen+1])
+	serializedHeight := binary.LittleEndian.Uint64(serializedHeightBytes)
 	if int64(serializedHeight) != wantHeight {
 		str := fmt.Sprintf("the coinbase signature script serialized "+
 			"block height is %d when %d was expected",

validate_test.go

@@ -9,7 +9,9 @@ import (
 	"github.com/conformal/btcdb"
 	"github.com/conformal/btcutil"
 	"github.com/conformal/btcwire"
+	"math"
 	"os"
+	"reflect"
 	"testing"
 	"time"
 )
@@ -36,6 +38,54 @@ func TestCheckBlockSanity(t *testing.T) {
 	}
 }
 
+// TestCheckSerializedHeight tests the checkSerializedHeight function with
+// various serialized heights and also does negative tests to ensure errors
+// and handled properly.
+func TestCheckSerializedHeight(t *testing.T) {
+	// Create an empty coinbase template to be used in the tests below.
+	coinbaseOutpoint := btcwire.NewOutPoint(&btcwire.ShaHash{}, math.MaxUint32)
+	coinbaseTx := btcwire.NewMsgTx()
+	coinbaseTx.Version = 2
+	coinbaseTx.AddTxIn(btcwire.NewTxIn(coinbaseOutpoint, nil))
+
+	//
+	tests := []struct {
+		sigScript  []byte // Serialized data
+		wantHeight int64  // Expected height
+		err        error  // Expected error type
+	}{
+		// No serialized height length.
+		{[]byte{}, 0, btcchain.RuleError("")},
+		// Serialized height length with no height bytes.
+		{[]byte{0x02}, 0, btcchain.RuleError("")},
+		// Serialized height length with too few height bytes.
+		{[]byte{0x02, 0x4a}, 0, btcchain.RuleError("")},
+		// Serialized height that needs 2 bytes to encode.
+		{[]byte{0x02, 0x4a, 0x52}, 21066, nil},
+		// Serialized height that needs 2 bytes to encode, but backwards
+		// endianness.
+		{[]byte{0x02, 0x4a, 0x52}, 19026, btcchain.RuleError("")},
+		// Serialized height that needs 3 bytes to encode.
+		{[]byte{0x03, 0x40, 0x0d, 0x03}, 200000, nil},
+		// Serialized height that needs 3 bytes to encode, but backwards
+		// endianness.
+		{[]byte{0x03, 0x40, 0x0d, 0x03}, 1074594560, btcchain.RuleError("")},
+	}
+
+	t.Logf("Running %d tests", len(tests))
+	for i, test := range tests {
+		tx := coinbaseTx.Copy()
+		tx.TxIn[0].SignatureScript = test.sigScript
+
+		err := btcchain.TstCheckSerializedHeight(tx, test.wantHeight)
+		if reflect.TypeOf(err) != reflect.TypeOf(test.err) {
+			t.Errorf("checkSerializedHeight #%d wrong error type "+
+				"got: %v <%T>, want: %T", i, err, err, test.err)
+			continue
+		}
+	}
+}
+
 // Block100000 defines block 100,000 of the block chain.  It is used to
 // test Block operations.
 var Block100000 = btcwire.MsgBlock{