multi: Improvements to configurable checkpoints.

This contains a bit of cleanup and additional logic to improve the
recently-added ability to specify additional checkpoints via the
--addcheckpoint option.

In particular:
- Improve error messages in the checkpoint parsing
- Correct the mergeCheckpoints function to weed out duplicate height
  checkpoints while using the most-recently provided one as described by
  its comment
- Add an assertion to blockchain.New that the provided checkpoints are
  sorted as required
- Keep comments to 80 columns and use two spaces after periods in them to
  be consistent with the rest of the code base
- Make the entry in doc.go match the actual btcd -h output

blockchain/chain.go

@@ -1640,10 +1640,12 @@ type Config struct {
 	// This field is required.
 	ChainParams *chaincfg.Params
 
-	// Checkpoints hold caller-defined checkpoints that should be added to the
-	// default checkpoints in ChainParams. Checkpoints must be sorted by height.
+	// Checkpoints hold caller-defined checkpoints that should be added to
+	// the default checkpoints in ChainParams.  Checkpoints must be sorted
+	// by height.
 	//
-	// This field can be nil if the caller did not specify any checkpoints.
+	// This field can be nil if the caller does not wish to specify any
+	// checkpoints.
 	Checkpoints []chaincfg.Checkpoint
 
 	// TimeSource defines the median time source to use for things such as
@@ -1693,13 +1695,21 @@ func New(config *Config) (*BlockChain, error) {
 		return nil, AssertError("blockchain.New timesource is nil")
 	}
 
-	// Generate a checkpoint by height map from the provided checkpoints.
+	// Generate a checkpoint by height map from the provided checkpoints
+	// and assert the provided checkpoints are sorted by height as required.
 	var checkpointsByHeight map[int32]*chaincfg.Checkpoint
+	var prevCheckpointHeight int32
 	if len(config.Checkpoints) > 0 {
 		checkpointsByHeight = make(map[int32]*chaincfg.Checkpoint)
 		for i := range config.Checkpoints {
 			checkpoint := &config.Checkpoints[i]
+			if checkpoint.Height <= prevCheckpointHeight {
+				return nil, AssertError("blockchain.New " +
+					"checkpoints are not sorted by height")
+			}
+
 			checkpointsByHeight[checkpoint.Height] = checkpoint
+			prevCheckpointHeight = checkpoint.Height
 		}
 	}
 

blockchain/chain_test.go

@@ -43,7 +43,8 @@ func TestHaveBlock(t *testing.T) {
 	}
 	defer teardownFunc()
 
-	// Since we're not dealing with the real block chain, set the coinbase maturity to 1.
+	// Since we're not dealing with the real block chain, set the coinbase
+	// maturity to 1.
 	chain.TstSetCoinbaseMaturity(1)
 
 	for i := 1; i < len(blocks); i++ {
@@ -129,7 +130,8 @@ func TestCalcSequenceLock(t *testing.T) {
 	}
 	defer teardownFunc()
 
-	// Since we're not dealing with the real block chain, set the coinbase maturity to 1.
+	// Since we're not dealing with the real block chain, set the coinbase
+	// maturity to 1.
 	chain.TstSetCoinbaseMaturity(1)
 
 	// Load all the blocks into our test chain.

blockchain/checkpoints.go

@@ -28,8 +28,8 @@ func newHashFromStr(hexStr string) *chainhash.Hash {
 }
 
 // Checkpoints returns a slice of checkpoints (regardless of whether they are
-// already known).
-// When there are no checkpoints for the chain, it will return nil.
+// already known).  When there are no checkpoints for the chain, it will return
+// nil.
 //
 // This function is safe for concurrent access.
 func (b *BlockChain) Checkpoints() []chaincfg.Checkpoint {
@@ -43,8 +43,8 @@ func (b *BlockChain) HasCheckpoints() bool {
 	return len(b.checkpoints) > 0
 }
 
-// LatestCheckpoint returns the most recent checkpoint (regardless of whether they
-// are already known). When there are no defined checkpoints for the active chain
+// LatestCheckpoint returns the most recent checkpoint (regardless of whether it
+// is already known). When there are no defined checkpoints for the active chain
 // instance, it will return nil.
 //
 // This function is safe for concurrent access.
@@ -56,7 +56,7 @@ func (b *BlockChain) LatestCheckpoint() *chaincfg.Checkpoint {
 }
 
 // verifyCheckpoint returns whether the passed block height and hash combination
-// match the checkpoint data. It also returns true if there is no checkpoint
+// match the checkpoint data.  It also returns true if there is no checkpoint
 // data for the passed block height.
 func (b *BlockChain) verifyCheckpoint(height int32, hash *chainhash.Hash) bool {
 	if !b.HasCheckpoints() {

blockchain/reorganization_test.go

@@ -52,7 +52,8 @@ func TestReorganization(t *testing.T) {
 	}
 	defer teardownFunc()
 
-	// Since we're not dealing with the real block chain set the coinbase maturity to 1.
+	// Since we're not dealing with the real block chain set the coinbase
+	// maturity to 1.
 	chain.TstSetCoinbaseMaturity(1)
 
 	expectedOrphans := map[int]struct{}{5: {}, 6: {}}

blockmanager.go

@@ -1353,25 +1353,27 @@ func (s checkpointSorter) Less(i, j int) bool {
 // default checkpoints, the additional checkpoint will take precedence and
 // overwrite the default one.
 func mergeCheckpoints(defaultCheckpoints, additional []chaincfg.Checkpoint) []chaincfg.Checkpoint {
-	// Create a map of the additional checkpoint heights to detect
-	// duplicates.
-	additionalHeights := make(map[int32]struct{})
+	// Create a map of the additional checkpoints to remove duplicates while
+	// leaving the most recently-specified checkpoint.
+	extra := make(map[int32]chaincfg.Checkpoint)
 	for _, checkpoint := range additional {
-		additionalHeights[checkpoint.Height] = struct{}{}
+		extra[checkpoint.Height] = checkpoint
 	}
 
 	// Add all default checkpoints that do not have an override in the
 	// additional checkpoints.
 	numDefault := len(defaultCheckpoints)
-	checkpoints := make([]chaincfg.Checkpoint, 0, numDefault+len(additional))
+	checkpoints := make([]chaincfg.Checkpoint, 0, numDefault+len(extra))
 	for _, checkpoint := range defaultCheckpoints {
-		if _, exists := additionalHeights[checkpoint.Height]; !exists {
+		if _, exists := extra[checkpoint.Height]; !exists {
 			checkpoints = append(checkpoints, checkpoint)
 		}
 	}
 
 	// Append the additional checkpoints and return the sorted results.
-	checkpoints = append(checkpoints, additional...)
+	for _, checkpoint := range extra {
+		checkpoints = append(checkpoints, checkpoint)
+	}
 	sort.Sort(checkpointSorter(checkpoints))
 	return checkpoints
 }

config.go

@@ -125,7 +125,7 @@ type config struct {
 	TestNet3             bool          `long:"testnet" description:"Use the test network"`
 	RegressionTest       bool          `long:"regtest" description:"Use the regression test network"`
 	SimNet               bool          `long:"simnet" description:"Use the simulation test network"`
-	AddCheckpoints       []string      `long:"addcheckpoint" description:"Add a custom checkpoint. Format: '<height>:<hash>'"`
+	AddCheckpoints       []string      `long:"addcheckpoint" description:"Add a custom checkpoint.  Format: '<height>:<hash>'"`
 	DisableCheckpoints   bool          `long:"nocheckpoints" description:"Disable built-in checkpoints.  Don't do this unless you know what you're doing."`
 	DbType               string        `long:"dbtype" description:"Database backend to use for the Block Chain"`
 	Profile              string        `long:"profile" description:"Enable HTTP profiling on given port -- NOTE port must be between 1024 and 65536"`
@@ -311,20 +311,25 @@ func normalizeAddresses(addrs []string, defaultPort string) []string {
 func newCheckpointFromStr(checkpoint string) (chaincfg.Checkpoint, error) {
 	parts := strings.Split(checkpoint, ":")
 	if len(parts) != 2 {
-		return chaincfg.Checkpoint{}, errors.New("checkpoints must use the " +
-			"syntax '<height>:<hash>'")
+		return chaincfg.Checkpoint{}, fmt.Errorf("unable to parse "+
+			"checkpoint %q -- use the syntax <height>:<hash>",
+			checkpoint)
 	}
 
 	height, err := strconv.ParseInt(parts[0], 10, 32)
 	if err != nil {
-		return chaincfg.Checkpoint{}, fmt.Errorf("unable to parse checkpoint "+
-			"due to malformed height: %s", parts[0])
+		return chaincfg.Checkpoint{}, fmt.Errorf("unable to parse "+
+			"checkpoint %q due to malformed height", checkpoint)
 	}
 
+	if len(parts[1]) == 0 {
+		return chaincfg.Checkpoint{}, fmt.Errorf("unable to parse "+
+			"checkpoint %q due to missing hash", checkpoint)
+	}
 	hash, err := chainhash.NewHashFromStr(parts[1])
 	if err != nil {
-		return chaincfg.Checkpoint{}, fmt.Errorf("unable to parse checkpoint "+
-			"due to malformed hash: %s", parts[1])
+		return chaincfg.Checkpoint{}, fmt.Errorf("unable to parse "+
+			"checkpoint %q due to malformed hash", checkpoint)
 	}
 
 	return chaincfg.Checkpoint{

doc.go

@@ -74,8 +74,7 @@ Application Options:
       --testnet             Use the test network
       --regtest             Use the regression test network
       --simnet              Use the simulation test network
-      --addcheckpoint=      Add ad additional checkpoint.
-                            Format: '<height>:<hash>'
+      --addcheckpoint=      Add a custom checkpoint.  Format: '<height>:<hash>'
       --nocheckpoints       Disable built-in checkpoints.  Don't do this unless
                             you know what you're doing.
       --dbtype=             Database backend to use for the Block Chain (ffldb)