Merge pull request #576 from wpaulino/birthday-block-candidate

wallet/chainntnfs: set height for new birthday block candidate

wallet/chainntfns.go

@@ -10,13 +10,22 @@ import (
 	"strings"
 	"time"
 
+	"github.com/btcsuite/btcd/chaincfg/chainhash"
 	"github.com/btcsuite/btcd/txscript"
+	"github.com/btcsuite/btcd/wire"
 	"github.com/btcsuite/btcwallet/chain"
 	"github.com/btcsuite/btcwallet/waddrmgr"
 	"github.com/btcsuite/btcwallet/walletdb"
 	"github.com/btcsuite/btcwallet/wtxmgr"
 )
 
+const (
+	// birthdayBlockDelta is the maximum time delta allowed between our
+	// birthday timestamp and our birthday block's timestamp when searching
+	// for a better birthday block candidate (if possible).
+	birthdayBlockDelta = 2 * time.Hour
+)
+
 func (w *Wallet) handleChainNotifications() {
 	defer w.wg.Done()
 
@@ -102,8 +111,14 @@ func (w *Wallet) handleChainNotifications() {
 				// we'll make sure that our birthday block has
 				// been set correctly to potentially prevent
 				// missing relevant events.
-				birthdayBlock, err := w.birthdaySanityCheck()
+				birthdayStore := &walletBirthdayStore{
-				if err != nil {
+					db:      w.db,
+					manager: w.Manager,
+				}
+				birthdayBlock, err := birthdaySanityCheck(
+					chainClient, birthdayStore,
+				)
+				if err != nil && !waddrmgr.IsError(err, waddrmgr.ErrBirthdayBlockNotSet) {
 					err := fmt.Errorf("unable to sanity "+
 						"check wallet birthday block: %v",
 						err)
@@ -345,41 +360,105 @@ func (w *Wallet) addRelevantTx(dbtx walletdb.ReadWriteTx, rec *wtxmgr.TxRecord,
 	return nil
 }
 
-// birthdaySanityCheck is a helper function that ensures our birthday block
+// chainConn is an interface that abstracts the chain connection logic required
-// correctly reflects the birthday timestamp within a reasonable timestamp
+// to perform a wallet's birthday block sanity check.
-// delta. It will be run after the wallet establishes its connection with the
+type chainConn interface {
-// backend, but before it begins syncing.  This is done as the second part to
+	// GetBestBlock returns the hash and height of the best block known to
-// the wallet's address manager migration where we populate the birthday block
+	// the backend.
-// to ensure we do not miss any relevant events throughout rescans.
+	GetBestBlock() (*chainhash.Hash, int32, error)
-func (w *Wallet) birthdaySanityCheck() (*waddrmgr.BlockStamp, error) {
-	// We'll start by acquiring our chain backend client as we'll be
-	// querying it for blocks.
-	chainClient, err := w.requireChainClient()
-	if err != nil {
-		return nil, err
-	}
 
-	// We'll then fetch our wallet's birthday timestamp and block.
+	// GetBlockHash returns the hash of the block with the given height.
+	GetBlockHash(int64) (*chainhash.Hash, error)
+
+	// GetBlockHeader returns the header for the block with the given hash.
+	GetBlockHeader(*chainhash.Hash) (*wire.BlockHeader, error)
+}
+
+// birthdayStore is an interface that abstracts the wallet's sync-related
+// information required to perform a birthday block sanity check.
+type birthdayStore interface {
+	// Birthday returns the birthday timestamp of the wallet.
+	Birthday() time.Time
+
+	// BirthdayBlock returns the birthday block of the wallet. The boolean
+	// returned should signal whether the wallet has already verified the
+	// correctness of its birthday block.
+	BirthdayBlock() (waddrmgr.BlockStamp, bool, error)
+
+	// SetBirthdayBlock updates the birthday block of the wallet to the
+	// given block. The boolean can be used to signal whether this block
+	// should be sanity checked the next time the wallet starts.
+	//
+	// NOTE: This should also set the wallet's synced tip to reflect the new
+	// birthday block. This will allow the wallet to rescan from this point
+	// to detect any potentially missed events.
+	SetBirthdayBlock(waddrmgr.BlockStamp) error
+}
+
+// walletBirthdayStore is a wrapper around the wallet's database and address
+// manager that satisfies the birthdayStore interface.
+type walletBirthdayStore struct {
+	db      walletdb.DB
+	manager *waddrmgr.Manager
+}
+
+var _ birthdayStore = (*walletBirthdayStore)(nil)
+
+// Birthday returns the birthday timestamp of the wallet.
+func (s *walletBirthdayStore) Birthday() time.Time {
+	return s.manager.Birthday()
+}
+
+// BirthdayBlock returns the birthday block of the wallet.
+func (s *walletBirthdayStore) BirthdayBlock() (waddrmgr.BlockStamp, bool, error) {
 	var (
-		birthdayTimestamp     = w.Manager.Birthday()
 		birthdayBlock         waddrmgr.BlockStamp
 		birthdayBlockVerified bool
 	)
-	err = walletdb.View(w.db, func(tx walletdb.ReadTx) error {
+
+	err := walletdb.View(s.db, func(tx walletdb.ReadTx) error {
 		var err error
 		ns := tx.ReadBucket(waddrmgrNamespaceKey)
-		birthdayBlock, birthdayBlockVerified, err = w.Manager.BirthdayBlock(ns)
+		birthdayBlock, birthdayBlockVerified, err = s.manager.BirthdayBlock(ns)
 		return err
 	})
 
-	switch {
+	return birthdayBlock, birthdayBlockVerified, err
-	// If our wallet's birthday block has not been set yet, then this is our
+}
-	// initial sync, so we'll defer setting it until then.
-	case waddrmgr.IsError(err, waddrmgr.ErrBirthdayBlockNotSet):
-		return nil, nil
 
-	// Otherwise, we'll return the error if there was one.
+// SetBirthdayBlock updates the birthday block of the wallet to the
-	case err != nil:
+// given block. The boolean can be used to signal whether this block
+// should be sanity checked the next time the wallet starts.
+//
+// NOTE: This should also set the wallet's synced tip to reflect the new
+// birthday block. This will allow the wallet to rescan from this point
+// to detect any potentially missed events.
+func (s *walletBirthdayStore) SetBirthdayBlock(block waddrmgr.BlockStamp) error {
+	return walletdb.Update(s.db, func(tx walletdb.ReadWriteTx) error {
+		ns := tx.ReadWriteBucket(waddrmgrNamespaceKey)
+		err := s.manager.SetBirthdayBlock(ns, block, true)
+		if err != nil {
+			return err
+		}
+		return s.manager.SetSyncedTo(ns, &block)
+	})
+}
+
+// birthdaySanityCheck is a helper function that ensures a birthday block
+// correctly reflects the birthday timestamp within a reasonable timestamp
+// delta. It's intended to be run after the wallet establishes its connection
+// with the backend, but before it begins syncing. This is done as the second
+// part to the wallet's address manager migration where we populate the birthday
+// block to ensure we do not miss any relevant events throughout rescans.
+// waddrmgr.ErrBirthdayBlockNotSet is returned if the birthday block has not
+// been set yet.
+func birthdaySanityCheck(chainConn chainConn,
+	birthdayStore birthdayStore) (*waddrmgr.BlockStamp, error) {
+
+	// We'll start by fetching our wallet's birthday timestamp and block.
+	birthdayTimestamp := birthdayStore.Birthday()
+	birthdayBlock, birthdayBlockVerified, err := birthdayStore.BirthdayBlock()
+	if err != nil {
 		return nil, err
 	}
 
@@ -404,7 +483,7 @@ func (w *Wallet) birthdaySanityCheck() (*waddrmgr.BlockStamp, error) {
 	// set (this is possible if it was set through the migration, since we
 	// do not store block timestamps).
 	candidate := birthdayBlock
-	header, err := chainClient.GetBlockHeader(&candidate.Hash)
+	header, err := chainConn.GetBlockHeader(&candidate.Hash)
 	if err != nil {
 		return nil, fmt.Errorf("unable to get header for block hash "+
 			"%v: %v", candidate.Hash, err)
@@ -430,18 +509,19 @@ func (w *Wallet) birthdaySanityCheck() (*waddrmgr.BlockStamp, error) {
 
 		// Then, we'll fetch the current candidate's hash and header to
 		// determine if it is valid.
-		hash, err := chainClient.GetBlockHash(newCandidateHeight)
+		hash, err := chainConn.GetBlockHash(newCandidateHeight)
 		if err != nil {
 			return nil, fmt.Errorf("unable to get block hash for "+
 				"height %d: %v", candidate.Height, err)
 		}
-		header, err := chainClient.GetBlockHeader(hash)
+		header, err := chainConn.GetBlockHeader(hash)
 		if err != nil {
 			return nil, fmt.Errorf("unable to get header for "+
 				"block hash %v: %v", candidate.Hash, err)
 		}
 
 		candidate.Hash = *hash
+		candidate.Height = int32(newCandidateHeight)
 		candidate.Timestamp = header.Timestamp
 
 		log.Debugf("Checking next birthday block candidate: "+
@@ -456,12 +536,12 @@ func (w *Wallet) birthdaySanityCheck() (*waddrmgr.BlockStamp, error) {
 	// actual birthday, so we'll make our expected delta to be within two
 	// hours of it to account for the network-adjusted time and prevent
 	// fetching more unnecessary blocks.
-	_, bestHeight, err := chainClient.GetBestBlock()
+	_, bestHeight, err := chainConn.GetBestBlock()
 	if err != nil {
 		return nil, err
 	}
 	timestampDelta := birthdayTimestamp.Sub(candidate.Timestamp)
-	for timestampDelta > 2*time.Hour {
+	for timestampDelta > birthdayBlockDelta {
 		// We'll determine the height for our next candidate and make
 		// sure it is not out of range. If it is, we'll lower our height
 		// delta until finding a height within range.
@@ -481,12 +561,12 @@ func (w *Wallet) birthdaySanityCheck() (*waddrmgr.BlockStamp, error) {
 
 		// We'll fetch the header for the next candidate and compare its
 		// timestamp.
-		hash, err := chainClient.GetBlockHash(int64(newHeight))
+		hash, err := chainConn.GetBlockHash(int64(newHeight))
 		if err != nil {
 			return nil, fmt.Errorf("unable to get block hash for "+
 				"height %d: %v", candidate.Height, err)
 		}
-		header, err := chainClient.GetBlockHeader(hash)
+		header, err := chainConn.GetBlockHeader(hash)
 		if err != nil {
 			return nil, fmt.Errorf("unable to get header for "+
 				"block hash %v: %v", hash, err)
@@ -524,15 +604,7 @@ func (w *Wallet) birthdaySanityCheck() (*waddrmgr.BlockStamp, error) {
 	log.Debugf("Found a new valid wallet birthday block: height=%d, hash=%v",
 		candidate.Height, candidate.Hash)
 
-	err = walletdb.Update(w.db, func(tx walletdb.ReadWriteTx) error {
+	if err := birthdayStore.SetBirthdayBlock(candidate); err != nil {
-		ns := tx.ReadWriteBucket(waddrmgrNamespaceKey)
-		err := w.Manager.SetBirthdayBlock(ns, candidate, true)
-		if err != nil {
-			return err
-		}
-		return w.Manager.SetSyncedTo(ns, &candidate)
-	})
-	if err != nil {
 		return nil, err
 	}
 

wallet/chainntfns_test.go

@@ -0,0 +1,300 @@
+package wallet
+
+import (
+	"fmt"
+	"reflect"
+	"testing"
+	"time"
+
+	"github.com/btcsuite/btcd/chaincfg"
+	"github.com/btcsuite/btcd/chaincfg/chainhash"
+	"github.com/btcsuite/btcd/wire"
+	"github.com/btcsuite/btcwallet/waddrmgr"
+	_ "github.com/btcsuite/btcwallet/walletdb/bdb"
+)
+
+var (
+	// chainParams are the chain parameters used throughout the wallet
+	// tests.
+	chainParams = chaincfg.MainNetParams
+
+	// blockInterval is the time interval between any two blocks in a mocked
+	// chain.
+	blockInterval = 10 * time.Minute
+)
+
+// mockChainConn is a mock in-memory implementation of the chainConn interface
+// that will be used for the birthday block sanity check tests. The struct is
+// capable of being backed by a chain in order to reproduce real-world
+// scenarios.
+type mockChainConn struct {
+	chainTip    uint32
+	blockHashes map[uint32]chainhash.Hash
+	blocks      map[chainhash.Hash]*wire.MsgBlock
+}
+
+var _ chainConn = (*mockChainConn)(nil)
+
+// createMockChainConn creates a new mock chain connection backed by a chain
+// with N blocks. Each block has a timestamp that is exactly 10 minutes after
+// the previous block's timestamp.
+func createMockChainConn(genesis *wire.MsgBlock, n uint32) *mockChainConn {
+	c := &mockChainConn{
+		chainTip:    n,
+		blockHashes: make(map[uint32]chainhash.Hash),
+		blocks:      make(map[chainhash.Hash]*wire.MsgBlock),
+	}
+
+	genesisHash := genesis.BlockHash()
+	c.blockHashes[0] = genesisHash
+	c.blocks[genesisHash] = genesis
+
+	for i := uint32(1); i <= n; i++ {
+		prevTimestamp := c.blocks[c.blockHashes[i-1]].Header.Timestamp
+		block := &wire.MsgBlock{
+			Header: wire.BlockHeader{
+				Timestamp: prevTimestamp.Add(blockInterval),
+			},
+		}
+
+		blockHash := block.BlockHash()
+		c.blockHashes[i] = blockHash
+		c.blocks[blockHash] = block
+	}
+
+	return c
+}
+
+// GetBestBlock returns the hash and height of the best block known to the
+// backend.
+func (c *mockChainConn) GetBestBlock() (*chainhash.Hash, int32, error) {
+	bestHash, ok := c.blockHashes[c.chainTip]
+	if !ok {
+		return nil, 0, fmt.Errorf("block with height %d not found",
+			c.chainTip)
+	}
+
+	return &bestHash, int32(c.chainTip), nil
+}
+
+// GetBlockHash returns the hash of the block with the given height.
+func (c *mockChainConn) GetBlockHash(height int64) (*chainhash.Hash, error) {
+	hash, ok := c.blockHashes[uint32(height)]
+	if !ok {
+		return nil, fmt.Errorf("block with height %d not found", height)
+	}
+
+	return &hash, nil
+}
+
+// GetBlockHeader returns the header for the block with the given hash.
+func (c *mockChainConn) GetBlockHeader(hash *chainhash.Hash) (*wire.BlockHeader, error) {
+	block, ok := c.blocks[*hash]
+	if !ok {
+		return nil, fmt.Errorf("header for block %v not found", hash)
+	}
+
+	return &block.Header, nil
+}
+
+// mockBirthdayStore is a mock in-memory implementation of the birthdayStore interface
+// that will be used for the birthday block sanity check tests.
+type mockBirthdayStore struct {
+	birthday              time.Time
+	birthdayBlock         *waddrmgr.BlockStamp
+	birthdayBlockVerified bool
+	syncedTo              waddrmgr.BlockStamp
+}
+
+var _ birthdayStore = (*mockBirthdayStore)(nil)
+
+// Birthday returns the birthday timestamp of the wallet.
+func (s *mockBirthdayStore) Birthday() time.Time {
+	return s.birthday
+}
+
+// BirthdayBlock returns the birthday block of the wallet.
+func (s *mockBirthdayStore) BirthdayBlock() (waddrmgr.BlockStamp, bool, error) {
+	if s.birthdayBlock == nil {
+		err := waddrmgr.ManagerError{
+			ErrorCode: waddrmgr.ErrBirthdayBlockNotSet,
+		}
+		return waddrmgr.BlockStamp{}, false, err
+	}
+
+	return *s.birthdayBlock, s.birthdayBlockVerified, nil
+}
+
+// SetBirthdayBlock updates the birthday block of the wallet to the given block.
+// The boolean can be used to signal whether this block should be sanity checked
+// the next time the wallet starts.
+func (s *mockBirthdayStore) SetBirthdayBlock(block waddrmgr.BlockStamp) error {
+	s.birthdayBlock = &block
+	s.birthdayBlockVerified = true
+	s.syncedTo = block
+	return nil
+}
+
+// TestBirthdaySanityCheckEmptyBirthdayBlock ensures that a sanity check is not
+// done if the birthday block does not exist in the first place.
+func TestBirthdaySanityCheckEmptyBirthdayBlock(t *testing.T) {
+	t.Parallel()
+
+	chainConn := &mockChainConn{}
+
+	// Our birthday store will reflect that we don't have a birthday block
+	// set, so we should not attempt a sanity check.
+	birthdayStore := &mockBirthdayStore{}
+
+	birthdayBlock, err := birthdaySanityCheck(chainConn, birthdayStore)
+	if !waddrmgr.IsError(err, waddrmgr.ErrBirthdayBlockNotSet) {
+		t.Fatalf("expected ErrBirthdayBlockNotSet, got %v", err)
+	}
+
+	if birthdayBlock != nil {
+		t.Fatalf("expected birthday block to be nil due to not being "+
+			"set, got %v", *birthdayBlock)
+	}
+}
+
+// TestBirthdaySanityCheckVerifiedBirthdayBlock ensures that a sanity check is
+// not performed if the birthday block has already been verified.
+func TestBirthdaySanityCheckVerifiedBirthdayBlock(t *testing.T) {
+	t.Parallel()
+
+	const chainTip = 5000
+	chainConn := createMockChainConn(chainParams.GenesisBlock, chainTip)
+	expectedBirthdayBlock := waddrmgr.BlockStamp{Height: 1337}
+
+	// Our birthday store reflects that our birthday block has already been
+	// verified and should not require a sanity check.
+	birthdayStore := &mockBirthdayStore{
+		birthdayBlock:         &expectedBirthdayBlock,
+		birthdayBlockVerified: true,
+		syncedTo: waddrmgr.BlockStamp{
+			Height: chainTip,
+		},
+	}
+
+	// Now, we'll run the sanity check. We should see that the birthday
+	// block hasn't changed.
+	birthdayBlock, err := birthdaySanityCheck(chainConn, birthdayStore)
+	if err != nil {
+		t.Fatalf("unable to sanity check birthday block: %v", err)
+	}
+	if !reflect.DeepEqual(*birthdayBlock, expectedBirthdayBlock) {
+		t.Fatalf("expected birthday block %v, got %v",
+			expectedBirthdayBlock, birthdayBlock)
+	}
+
+	// To ensure the sanity check didn't proceed, we'll check our synced to
+	// height, as this value should have been modified if a new candidate
+	// was found.
+	if birthdayStore.syncedTo.Height != chainTip {
+		t.Fatalf("expected synced height remain the same (%d), got %d",
+			chainTip, birthdayStore.syncedTo.Height)
+	}
+}
+
+// TestBirthdaySanityCheckLowerEstimate ensures that we can properly locate a
+// better birthday block candidate if our estimate happens to be too far back in
+// the chain.
+func TestBirthdaySanityCheckLowerEstimate(t *testing.T) {
+	t.Parallel()
+
+	// We'll start by defining our birthday timestamp to be around the
+	// timestamp of the 1337th block.
+	genesisTimestamp := chainParams.GenesisBlock.Header.Timestamp
+	birthday := genesisTimestamp.Add(1337 * blockInterval)
+
+	// We'll establish a connection to a mock chain of 5000 blocks.
+	chainConn := createMockChainConn(chainParams.GenesisBlock, 5000)
+
+	// Our birthday store will reflect that our birthday block is currently
+	// set as the genesis block. This value is too low and should be
+	// adjusted by the sanity check.
+	birthdayStore := &mockBirthdayStore{
+		birthday: birthday,
+		birthdayBlock: &waddrmgr.BlockStamp{
+			Hash:      *chainParams.GenesisHash,
+			Height:    0,
+			Timestamp: genesisTimestamp,
+		},
+		birthdayBlockVerified: false,
+		syncedTo: waddrmgr.BlockStamp{
+			Height: 5000,
+		},
+	}
+
+	// We'll perform the sanity check and determine whether we were able to
+	// find a better birthday block candidate.
+	birthdayBlock, err := birthdaySanityCheck(chainConn, birthdayStore)
+	if err != nil {
+		t.Fatalf("unable to sanity check birthday block: %v", err)
+	}
+	if birthday.Sub(birthdayBlock.Timestamp) >= birthdayBlockDelta {
+		t.Fatalf("expected birthday block timestamp=%v to be within "+
+			"%v of birthday timestamp=%v", birthdayBlock.Timestamp,
+			birthdayBlockDelta, birthday)
+	}
+
+	// Finally, our synced to height should now reflect our new birthday
+	// block to ensure the wallet doesn't miss any events from this point
+	// forward.
+	if !reflect.DeepEqual(birthdayStore.syncedTo, *birthdayBlock) {
+		t.Fatalf("expected syncedTo and birthday block to match: "+
+			"%v vs %v", birthdayStore.syncedTo, birthdayBlock)
+	}
+}
+
+// TestBirthdaySanityCheckHigherEstimate ensures that we can properly locate a
+// better birthday block candidate if our estimate happens to be too far in the
+// chain.
+func TestBirthdaySanityCheckHigherEstimate(t *testing.T) {
+	t.Parallel()
+
+	// We'll start by defining our birthday timestamp to be around the
+	// timestamp of the 1337th block.
+	genesisTimestamp := chainParams.GenesisBlock.Header.Timestamp
+	birthday := genesisTimestamp.Add(1337 * blockInterval)
+
+	// We'll establish a connection to a mock chain of 5000 blocks.
+	chainConn := createMockChainConn(chainParams.GenesisBlock, 5000)
+
+	// Our birthday store will reflect that our birthday block is currently
+	// set as the chain tip. This value is too high and should be adjusted
+	// by the sanity check.
+	bestBlock := chainConn.blocks[chainConn.blockHashes[5000]]
+	birthdayStore := &mockBirthdayStore{
+		birthday: birthday,
+		birthdayBlock: &waddrmgr.BlockStamp{
+			Hash:      bestBlock.BlockHash(),
+			Height:    5000,
+			Timestamp: bestBlock.Header.Timestamp,
+		},
+		birthdayBlockVerified: false,
+		syncedTo: waddrmgr.BlockStamp{
+			Height: 5000,
+		},
+	}
+
+	// We'll perform the sanity check and determine whether we were able to
+	// find a better birthday block candidate.
+	birthdayBlock, err := birthdaySanityCheck(chainConn, birthdayStore)
+	if err != nil {
+		t.Fatalf("unable to sanity check birthday block: %v", err)
+	}
+	if birthday.Sub(birthdayBlock.Timestamp) >= birthdayBlockDelta {
+		t.Fatalf("expected birthday block timestamp=%v to be within "+
+			"%v of birthday timestamp=%v", birthdayBlock.Timestamp,
+			birthdayBlockDelta, birthday)
+	}
+
+	// Finally, our synced to height should now reflect our new birthday
+	// block to ensure the wallet doesn't miss any events from this point
+	// forward.
+	if !reflect.DeepEqual(birthdayStore.syncedTo, *birthdayBlock) {
+		t.Fatalf("expected syncedTo and birthday block to match: "+
+			"%v vs %v", birthdayStore.syncedTo, birthdayBlock)
+	}
+}