Generate fee addresses in dcrvsp, not dcrwallet (#59)

* Remove unnecessary error handling.

* Generate fee addresses in dcrvsp, not dcrwallet

* Break loop if multiple invalid children are generated.

* Use Mutex instead of RWMutex

README.md

@@ -49,9 +49,9 @@ ticket details + fee to a VSP, and the VSP will take the fee and vote in return.
 
 ## Architecture
 
-- Single server running dcrvsp, dcrwallet and dcrd. dcrd requires txindex so
+- Single server running dcrvsp and dcrd. dcrd requires txindex so
   `getrawtransaction` can be used.
-- Multiple remote "Voting servers", each running dcrwallet and dcrd. dcrwallet
+- Multiple remote voting servers, each running dcrwallet and dcrd. dcrwallet
   on these servers should be constantly unlocked and have voting enabled.
 
 ## MVP Features
@@ -63,9 +63,8 @@ ticket details + fee to a VSP, and the VSP will take the fee and vote in return.
 - Every client request which references a ticket should include a HTTP header
   `VSP-Client-Signature`. The value of this header must be a signature of the
   request body, signed with the commitment address of the referenced ticket.
-- An xpub key is provided to dcrvsp via config. The first time dcrvsp starts, it
-  imports this xpub to create a new wallet account. This account is used to
-  derive addresses for fee payments.
+- An xpub key is provided to dcrvsp via config. dcrvsp will use this key to
+  derive addresses for fee payments. A new address is generated for each fee.
 - VSP API as described in [dcrstakepool #574](https://github.com/decred/dcrstakepool/issues/574)
   - Request fee amount (`GET /fee`)
   - Request fee address (`POST /feeaddress`)
@@ -82,14 +81,11 @@ ticket details + fee to a VSP, and the VSP will take the fee and vote in return.
 - Write database backups to disk periodically.
 - Backup over http.
 - Status check API call as described in [dcrstakepool #628](https://github.com/decred/dcrstakepool/issues/628).
-- Accountability for both client and server changes to voting preferences.
 - Consistency checking across connected wallets.
 
-## Backup and Recovery
+## Backup
 
-- Regular backups of bbolt database.
-- Restore requires manual repair of fee wallet. Import xpub into account "fees",
-  and rescan with a very large gap limit.
+- Regular backups of bbolt database and feexpub.
 
 ## Issue Tracker
 

database/addressindex.go

@@ -0,0 +1,39 @@
+package database
+
+import (
+	"encoding/binary"
+
+	bolt "go.etcd.io/bbolt"
+)
+
+func (vdb *VspDatabase) GetLastAddressIndex() (uint32, error) {
+	var idx uint32
+	err := vdb.db.View(func(tx *bolt.Tx) error {
+		vspBkt := tx.Bucket(vspBktK)
+
+		idxBytes := vspBkt.Get(lastAddressIndexK)
+		if idxBytes == nil {
+			return nil
+		}
+
+		idx = binary.LittleEndian.Uint32(idxBytes)
+
+		return nil
+	})
+
+	return idx, err
+}
+
+func (vdb *VspDatabase) SetLastAddressIndex(idx uint32) error {
+	err := vdb.db.Update(func(tx *bolt.Tx) error {
+		vspBkt := tx.Bucket(vspBktK)
+
+		idxBytes := make([]byte, 4)
+		binary.LittleEndian.PutUint32(idxBytes, idx)
+
+		return vspBkt.Put(lastAddressIndexK, idxBytes)
+
+	})
+
+	return err
+}

database/database.go

@@ -27,8 +27,10 @@ var (
 	ticketBktK = []byte("ticketbkt")
 	// version is the current database version.
 	versionK = []byte("version")
-	// privateKeyK is the private key.
+	// privatekey is the private key.
 	privateKeyK = []byte("privatekey")
+	// lastaddressindex is the index of the last address used for fees.
+	lastAddressIndexK = []byte("lastaddressindex")
 )
 
 // Open initializes and returns an open database. If no database file is found

database/database_test.go

@@ -17,6 +17,7 @@ func exampleTicket() Ticket {
 	return Ticket{
 		Hash:              "Hash",
 		CommitmentAddress: "Address",
+		FeeAddressIndex:   12345,
 		FeeAddress:        "FeeAddress",
 		SDiff:             1,
 		BlockHeight:       2,
@@ -105,6 +106,7 @@ func testGetTicketByHash(t *testing.T) {
 	// Check ticket fields match expected.
 	if retrieved.Hash != ticket.Hash ||
 		retrieved.CommitmentAddress != ticket.CommitmentAddress ||
+		retrieved.FeeAddressIndex != ticket.FeeAddressIndex ||
 		retrieved.FeeAddress != ticket.FeeAddress ||
 		retrieved.SDiff != ticket.SDiff ||
 		retrieved.BlockHeight != ticket.BlockHeight ||

database/ticket.go

@@ -12,6 +12,7 @@ import (
 type Ticket struct {
 	Hash              string            `json:"hash"`
 	CommitmentAddress string            `json:"commitmentaddress"`
+	FeeAddressIndex   uint32            `json:"feeaddressindex"`
 	FeeAddress        string            `json:"feeaddress"`
 	SDiff             float64           `json:"sdiff"`
 	BlockHeight       int64             `json:"blockheight"`
@@ -179,9 +180,5 @@ func (vdb *VspDatabase) CountTickets() (int, int, error) {
 		})
 	})
 
-	if err != nil {
-		return 0, 0, err
-	}
-
-	return total, feePaid, nil
+	return total, feePaid, err
 }

main.go

@@ -14,7 +14,6 @@ import (
 )
 
 const (
-	feeAccountName              = "fees"
 	defaultFeeAddressExpiration = 24 * time.Hour
 )
 
@@ -57,8 +56,9 @@ func run(ctx context.Context) error {
 		return err
 	}
 
-	// Create RPC client for local dcrwallet instance (used for generating fee
-	// addresses and broadcasting fee transactions).
+	// Create RPC client for local dcrwallet instance (used for broadcasting fee transactions).
+	// Dial once just to validate config.
+	// TODO: Replace with dcrd.
 	feeWalletConnect := rpc.Setup(ctx, &shutdownWg, cfg.FeeWalletUser, cfg.FeeWalletPass, cfg.FeeWalletHost, cfg.feeWalletCert)
 	feeWalletConn, err := feeWalletConnect()
 	if err != nil {
@@ -67,7 +67,7 @@ func run(ctx context.Context) error {
 		shutdownWg.Wait()
 		return err
 	}
-	feeWalletClient, err := rpc.FeeWalletClient(ctx, feeWalletConn)
+	_, err = rpc.FeeWalletClient(ctx, feeWalletConn)
 	if err != nil {
 		log.Errorf("Fee wallet client error: %v", err)
 		requestShutdown()
@@ -76,6 +76,7 @@ func run(ctx context.Context) error {
 	}
 
 	// Create RPC client for remote dcrwallet instance (used for voting).
+	// Dial once just to validate config.
 	votingWalletConnect := rpc.Setup(ctx, &shutdownWg, cfg.VotingWalletUser, cfg.VotingWalletPass, cfg.VotingWalletHost, cfg.votingWalletCert)
 	votingWalletConn, err := votingWalletConnect()
 	if err != nil {
@@ -92,6 +93,7 @@ func run(ctx context.Context) error {
 		return err
 	}
 
+	// TODO: This can move into webapi.Start()
 	signKey, pubKey, err := db.KeyPair()
 	if err != nil {
 		log.Errorf("Failed to get keypair: %v", err)
@@ -100,25 +102,16 @@ func run(ctx context.Context) error {
 		return err
 	}
 
-	// Ensure the wallet account for collecting fees exists and matches config.
-	err = setupFeeAccount(feeWalletClient, cfg.FeeXPub)
-	if err != nil {
-		log.Errorf("Fee account error: %v", err)
-		requestShutdown()
-		shutdownWg.Wait()
-		return err
-	}
-
 	// Create and start webapi server.
 	apiCfg := webapi.Config{
 		SignKey:              signKey,
 		PubKey:               pubKey,
 		VSPFee:               cfg.VSPFee,
 		NetParams:            cfg.netParams.Params,
-		FeeAccountName:       feeAccountName,
 		FeeAddressExpiration: defaultFeeAddressExpiration,
 	}
-	err = webapi.Start(ctx, shutdownRequestChannel, &shutdownWg, cfg.Listen, db, feeWalletConnect, votingWalletConnect, cfg.WebServerDebug, apiCfg)
+	err = webapi.Start(ctx, shutdownRequestChannel, &shutdownWg, cfg.Listen, db,
+		feeWalletConnect, votingWalletConnect, cfg.WebServerDebug, cfg.FeeXPub, apiCfg)
 	if err != nil {
 		log.Errorf("Failed to initialize webapi: %v", err)
 		requestShutdown()
@@ -131,35 +124,3 @@ func run(ctx context.Context) error {
 
 	return ctx.Err()
 }
-
-func setupFeeAccount(walletClient *rpc.FeeWalletRPC, feeXpub string) error {
-	// Check if account for fee collection already exists.
-	accounts, err := walletClient.ListAccounts()
-	if err != nil {
-		return fmt.Errorf("ListAccounts error: %v", err)
-	}
-
-	if _, ok := accounts[feeAccountName]; ok {
-		// Account already exists. Check xpub matches xpub from config.
-		existingXPub, err := walletClient.GetMasterPubKey(feeAccountName)
-		if err != nil {
-			return fmt.Errorf("GetMasterPubKey error: %v", err)
-		}
-
-		if existingXPub != feeXpub {
-			return fmt.Errorf("existing account xpub differs from config: %s != %s", existingXPub, feeXpub)
-		}
-
-		log.Debugf("Using existing wallet account %q to collect fees", feeAccountName)
-
-	} else {
-		// Account does not exist. Create it using xpub from config.
-		if err = walletClient.ImportXPub(feeAccountName, feeXpub); err != nil {
-			log.Errorf("ImportXPub error: %v", err)
-			return err
-		}
-		log.Debugf("Created new wallet account %q to collect fees", feeAccountName)
-	}
-
-	return nil
-}

rpc/feewallet.go

@@ -57,37 +57,6 @@ func FeeWalletClient(ctx context.Context, c Caller) (*FeeWalletRPC, error) {
 	return &FeeWalletRPC{c, ctx}, nil
 }
 
-func (c *FeeWalletRPC) ImportXPub(account, xpub string) error {
-	return c.Call(c.ctx, "importxpub", nil, account, xpub)
-}
-
-func (c *FeeWalletRPC) GetMasterPubKey(account string) (string, error) {
-	var pubKey string
-	err := c.Call(c.ctx, "getmasterpubkey", &pubKey, account)
-	if err != nil {
-		return "", err
-	}
-	return pubKey, nil
-}
-
-func (c *FeeWalletRPC) ListAccounts() (map[string]float64, error) {
-	var accounts map[string]float64
-	err := c.Call(c.ctx, "listaccounts", &accounts)
-	if err != nil {
-		return nil, err
-	}
-	return accounts, nil
-}
-
-func (c *FeeWalletRPC) GetNewAddress(account string) (string, error) {
-	var newAddress string
-	err := c.Call(c.ctx, "getnewaddress", &newAddress, account)
-	if err != nil {
-		return "", err
-	}
-	return newAddress, nil
-}
-
 func (c *FeeWalletRPC) GetBlockHeader(blockHash string) (*dcrdtypes.GetBlockHeaderVerboseResult, error) {
 	verbose := true
 	var blockHeader dcrdtypes.GetBlockHeaderVerboseResult

webapi/addressgenerator.go

@@ -0,0 +1,79 @@
+package webapi
+
+import (
+	"errors"
+
+	"github.com/decred/dcrd/chaincfg/v3"
+	"github.com/decred/dcrd/dcrec"
+	"github.com/decred/dcrd/dcrutil/v3"
+	"github.com/decred/dcrd/hdkeychain/v3"
+)
+
+type addressGenerator struct {
+	external      *hdkeychain.ExtendedKey
+	netParams     *chaincfg.Params
+	lastUsedIndex uint32
+}
+
+func newAddressGenerator(xPub string, netParams *chaincfg.Params, lastUsedIdx uint32) (*addressGenerator, error) {
+	xPubKey, err := hdkeychain.NewKeyFromString(xPub, netParams)
+	if err != nil {
+		return nil, err
+	}
+
+	if xPubKey.IsPrivate() {
+		return nil, errors.New("not a public key")
+	}
+
+	// Derive the extended key for the external chain.
+	external, err := xPubKey.Child(0)
+	if err != nil {
+		return nil, err
+	}
+
+	return &addressGenerator{
+		external:      external,
+		netParams:     netParams,
+		lastUsedIndex: lastUsedIdx,
+	}, nil
+}
+
+// NextAddress increments the last used address counter and returns a new
+// address. It will skip any address index which causes an ErrInvalidChild.
+// Not safe for concurrent access.
+func (m *addressGenerator) NextAddress() (string, uint32, error) {
+	var key *hdkeychain.ExtendedKey
+	var err error
+
+	// There is a small chance that generating addresses for a given index can
+	// fail with ErrInvalidChild, so loop until we find an index which works.
+	// See the hdkeychain.ExtendedKey.Child docs for more info.
+	invalidChildren := 0
+	for {
+		m.lastUsedIndex++
+		key, err = m.external.Child(m.lastUsedIndex)
+		if err != nil {
+			if err == hdkeychain.ErrInvalidChild {
+				invalidChildren++
+				log.Warnf("Generating address for index %d failed: %v", m.lastUsedIndex, err)
+				// If this happens 3 times, something is seriously wrong, so
+				// return an error.
+				if invalidChildren > 2 {
+					return "", 0, errors.New("multiple invalid children generated for key")
+				}
+				continue
+			}
+			return "", 0, err
+		}
+		break
+	}
+
+	// Convert to a standard pay-to-pubkey-hash address.
+	pkHash := dcrutil.Hash160(key.SerializedPubKey())
+	addr, err := dcrutil.NewAddressPubKeyHash(pkHash, m.netParams, dcrec.STEcdsaSecp256k1)
+	if err != nil {
+		return "", 0, err
+	}
+
+	return addr.String(), m.lastUsedIndex, nil
+}

webapi/getfeeaddress.go

@@ -3,6 +3,7 @@ package webapi
 import (
 	"encoding/hex"
 	"net/http"
+	"sync"
 	"time"
 
 	"github.com/decred/dcrd/blockchain/stake/v3"
@@ -13,6 +14,30 @@ import (
 	"github.com/jholdstock/dcrvsp/rpc"
 )
 
+// addrMtx protects getNewFeeAddress.
+var addrMtx sync.Mutex
+
+// getNewFeeAddress gets a new address from the address generator and stores the
+// new address index in the database. In order to maintain consistency between
+// the internal counter of address generator and the database, this function
+// cannot be run concurrently.
+func getNewFeeAddress(db *database.VspDatabase, addrGen *addressGenerator) (string, uint32, error) {
+	addrMtx.Lock()
+	defer addrMtx.Unlock()
+
+	addr, idx, err := addrGen.NextAddress()
+	if err != nil {
+		return "", 0, err
+	}
+
+	err = db.SetLastAddressIndex(idx)
+	if err != nil {
+		return "", 0, err
+	}
+
+	return addr, idx, nil
+}
+
 // feeAddress is the handler for "POST /feeaddress".
 func feeAddress(c *gin.Context) {
 
@@ -115,12 +140,9 @@ func feeAddress(c *gin.Context) {
 		return
 	}
 
-	// TODO: Generate this within dcrvsp without an RPC call?
-	newAddress, err := fWalletClient.GetNewAddress(cfg.FeeAccountName)
+	newAddress, newAddressIdx, err := getNewFeeAddress(db, addrGen)
 	if err != nil {
-		log.Errorf("GetNewAddress error: %v", err)
-		sendErrorResponse("unable to generate fee address", http.StatusInternalServerError, c)
-		return
+		log.Errorf("getNewFeeAddress error: %v", err)
 	}
 
 	now := time.Now()
@@ -129,6 +151,7 @@ func feeAddress(c *gin.Context) {
 	dbTicket := database.Ticket{
 		Hash:              ticketHash,
 		CommitmentAddress: commitmentAddress,
+		FeeAddressIndex:   newAddressIdx,
 		FeeAddress:        newAddress,
 		SDiff:             blockHeader.SBits,
 		BlockHeight:       int64(blockHeader.Height),
@@ -144,6 +167,9 @@ func feeAddress(c *gin.Context) {
 		return
 	}
 
+	log.Debugf("Fee address created for new ticket: feeAddrIdx=%d, "+
+		"feeAddr=%s, ticketHash=%s", newAddressIdx, newAddress, ticketHash)
+
 	sendJSONResponse(feeAddressResponse{
 		Timestamp:  now.Unix(),
 		Request:    feeAddressRequest,

webapi/webapi.go

@@ -33,9 +33,10 @@ var cfg Config
 var db *database.VspDatabase
 var feeWalletConnect rpc.Connect
 var votingWalletConnect rpc.Connect
+var addrGen *addressGenerator
 
 func Start(ctx context.Context, requestShutdownChan chan struct{}, shutdownWg *sync.WaitGroup,
-	listen string, vdb *database.VspDatabase, fWalletConnect rpc.Connect, vWalletConnect rpc.Connect, debugMode bool, config Config) error {
+	listen string, vdb *database.VspDatabase, fWalletConnect rpc.Connect, vWalletConnect rpc.Connect, debugMode bool, feeXPub string, config Config) error {
 
 	// Populate template data before starting webserver.
 	var err error
@@ -44,6 +45,17 @@ func Start(ctx context.Context, requestShutdownChan chan struct{}, shutdownWg *s
 		return fmt.Errorf("could not initialize homepage data: %v", err)
 	}
 
+	// Get the last used address index from the database, and use it to
+	// initialize the address generator.
+	idx, err := vdb.GetLastAddressIndex()
+	if err != nil {
+		return fmt.Errorf("GetLastAddressIndex error: %v", err)
+	}
+	addrGen, err = newAddressGenerator(feeXPub, config.NetParams, idx)
+	if err != nil {
+		return fmt.Errorf("failed to initialize fee address generator: %v", err)
+	}
+
 	// Create TCP listener.
 	var listenConfig net.ListenConfig
 	listener, err := listenConfig.Listen(ctx, "tcp", listen)