Refactor to integrate pkg ticketbuyer for automated ticket purchases #374

Merged
merged 46 commits into from Dec 16, 2016

Projects

None yet

3 participants

@tuxcanfly
Member
tuxcanfly commented Nov 4, 2016 edited

This PR integrates the newly refactor pkg ticketbuyer in dcrticketbuyer to make automated ticket purchases.

decred/dcrticketbuyer#70

Refs #387

config.go
@@ -110,6 +130,25 @@ type config struct {
ProxyUser string `long:"proxyuser" description:"Username for proxy server"`
ProxyPass string `long:"proxypass" default-mask:"-" description:"Password for proxy server"`
+ // Automatic ticket buyer settings
@jrick
jrick Dec 5, 2016 Member

I think all of these settings can be removed, as long as we're ok with using a client to enable the ticket purchasing with the specific parameters. For "easy" (parameterless) ticket buying these aren't needed.

@tuxcanfly
tuxcanfly Dec 9, 2016 Member

OK, removed. Passing defaults for all the configuration options instead. Config can be updated later, with provided specific parameters, if necessary.

config.go
@@ -573,6 +632,11 @@ func loadConfig() (*config, []string, error) {
}
}
+ // Always prompt passphrase if stake mining is enabled
@jrick
jrick Dec 5, 2016 Member

Instead of modifying the config, it just leave the config as is and when cfg.EnableStakeMining is true it can just run the password prompt at startup anyways.

@tuxcanfly
tuxcanfly Dec 9, 2016 Member

OK, updated.

dcrwallet.go
@@ -137,6 +137,9 @@ func walletMain() error {
startPromptPass(w)
}
startWalletRPCServices(w, rpcs, legacyRPCServer)
+ if w.StakeMiningEnabled {
+ go startTicketPurchase(w)
@jrick
jrick Dec 5, 2016 Member

I think this will need some additional parameters.

We need the private passphrase because ticket purchasing requires an unlocked wallet. When not running the wallet with stake mining already enabled (--enablestakemining), it should still be possible to turn on (and off!) the ticket purchaser. When used from the grpc interface, the passphrase will need to be passed in as a parameter.

It should also pass in some sort of default options here for the "easy" --enablestakemining option. When started via RPC, more advanced parameters can be used instead of the defaults.

@tuxcanfly
tuxcanfly Dec 9, 2016 Member

OK, started passing in the default options here.

For toggling ticket purchaser, I think we could have rpc similar to setgenerate. Will be adding that.

As for passphrase, would it make sense to let the caller make sure the wallet is unlocked before turning on purchasing? If it's not unlocked, the toggle purchase rpc call can return an error.

@jrick
jrick Dec 9, 2016 Member

well in the gRPC server there is no notion of an unlocked wallet. the private passphrase is passed as a field in every RPC request message that requires it. And we want this usable over grpc

@tuxcanfly
tuxcanfly Dec 15, 2016 Member

Updated to use passphrase if any and unlock the wallet.

ticketbuyer.go
+)
+
+// getWalletRPCClient returns a rpc client connected to the local wallet instance.
+func getWalletRPCClient() (*dcrrpcclient.Client, error) {
@jrick
jrick Dec 5, 2016 Member

Already discussed in chat, but the only use of RPC should be to begin/stop ticket buying, and changing the purchase parameters. There should be no internal use of RPC.

@tuxcanfly
tuxcanfly Dec 9, 2016 Member

Thanks, updated to not depend on wallet rpc.

log.go
@@ -13,6 +13,7 @@ import (
"github.com/btcsuite/seelog"
"github.com/decred/dcrrpcclient"
+ "github.com/decred/dcrticketbuyer/ticketbuyer"
@jrick
jrick Dec 5, 2016 Member

ticketbuyer should be a dcrwallet package, not part of the dcrticketbuyer repo. When dcrticketbuyer is updated to use the wallet's built in ticket purchasing, it can simply be an rpc client that interacts with the wallet's ticket purchaser over RPC.

@tuxcanfly
tuxcanfly Dec 15, 2016 Member

Done. The pkg ticketbuyer is now part of wallet.

ticketbuyer.go
+ PriceTarget: cfg.PriceTarget,
+ TicketAddress: cfg.TicketAddress,
+ TxFee: cfg.TicketFee,
+ }, dcrdClient, dcrwClient, activeNet.Params)
@jrick
jrick Dec 5, 2016 Member

I guess this should be obvious from the rest of my comments, but when this package is moved to this repo, it should interact with the wallet directly using Go apis, and not use any RPC.

@tuxcanfly
tuxcanfly Dec 9, 2016 Member

Thanks, this has been updated. The package is now a part of this repo and the caller can pass in wallet functions, so now it is not dependent on RPC.

dcrwallet.go
@@ -137,6 +138,30 @@ func walletMain() error {
startPromptPass(w)
}
startWalletRPCServices(w, rpcs, legacyRPCServer)
+ if w.StakeMiningEnabled {
+ ticketbuyerCfg := &ticketbuyer.Config{
+ AccountName: "default",
@jrick
jrick Dec 9, 2016 Member

Think this should use cfg.PurchaseAccount

dcrwallet.go
+ PoolAddress: "",
+ MaxFee: 1.0,
+ MinFee: 0.01,
+ FeeSource: "mean",
@jrick
jrick Dec 9, 2016 edited Member

I'm not really familiar with the original code being moved over from dcrticketbuyer, but I want these as declared constant values (declared in the ticketbuyer package, of course) instead of magic strings. Same with "vwap" below.

@tuxcanfly
tuxcanfly Dec 15, 2016 Member

Updated to use consts for defaults.

ticketbuyer.go
+// startTicketPurchase launches ticketbuyer to start purchasing tickets.
+func startTicketPurchase(w *wallet.Wallet, ticketbuyerCfg *ticketbuyer.Config) {
+ retryDuration := time.Second * 5
+ for {
@jrick
jrick Dec 9, 2016 edited Member

instead of looping over this just pass in the rpc client as a parameter. The caller has to ensure it exists before this feature is useful at all.

rule of thumb is to not call w.ChainClient() at all in any new code. Plan is to remove it down the road and replace it with an external synchronization service.

@tuxcanfly
tuxcanfly Dec 9, 2016 Member

Agreed, will do that.

@tuxcanfly
tuxcanfly Dec 15, 2016 Member

Done. Caller needs to pass ChainClient, moved the caller to an appropriate location in dcrwallet.go

ticketbuyer.go
+ tkbyLog.Debugf("Retrying chain client connection in %v", retryDuration)
+ }
+ dcrdClient := w.ChainClient().Client
+ walletCfg := &ticketbuyer.WalletCfg{
@jrick
jrick Dec 9, 2016 Member

From my initial perusal through this code, this type looks unneeded. We have access to the wallet so we don't need an extra level of indirection to call methods from it. Looks like rpc leftovers.

@tuxcanfly
tuxcanfly Dec 9, 2016 Member

This is required to make sure the pkg ticketbuyer works both standalone (with dcrwallet rpc client) as well as being used here. For example, here's the caller in dcrticketbuyer:

https://github.com/tuxcanfly/dcrticketbuyer/blob/pkg-ticketbuyer/main.go#L251

@jrick
jrick Dec 9, 2016 Member

oh, I see. So dcrticketbuyer is still doing all the purchasing logic then. Makes sense because there's no rpc interface yet to start the purchaser or change its parameters. Once that is in I think this can be removed.

@jrick
jrick Dec 12, 2016 edited Member

actually I think we could go ahead and clean this up here now. dcrticketbuyer can be left alone for now (keeping its own old purchasing logic) and only after the RPC interfaces are added will we want to update it to use those. Changing dcrticketbuyer before then is just a waste of time.

@tuxcanfly
tuxcanfly Dec 12, 2016 Member

OK, working on that. I guess it would involve some code duplication but that should be fixed when we phase out the purchase logic in dcrticketbuyer.

@jrick
jrick Dec 12, 2016 Member

yeah, we can afford some code duplication during the transition, that's no problem.

@tuxcanfly
tuxcanfly Dec 15, 2016 Member

Done, using the wallet directly in ticketbuyer now.

ticketbuyer/example_test.go
+// Use of this source code is governed by an ISC
+// license that can be found in the LICENSE file.
+
+package ticketbuyer_test
@jrick
jrick Dec 9, 2016 Member

if there's nothing here let's remove the file.

@tuxcanfly
tuxcanfly Dec 15, 2016 Member

Removed.

wallet/chainntfns.go
@@ -38,6 +38,22 @@ func (w *Wallet) handleConsensusRPCNotifications(chainClient *chain.RPCClient) {
err = walletdb.Update(w.db, func(tx walletdb.ReadWriteTx) error {
return w.onBlockConnected(tx, n.BlockHeader, n.Transactions)
})
+
+ if w.StakeMiningEnabled && w.purchaser != nil {
@jrick
jrick Dec 9, 2016 Member

read of w.purchaser is not safe for concurrent access.

@jrick
jrick Dec 9, 2016 Member

I think this also shows that the current design is backwards. wallet should not call the ticket purchaser. it should emit an event that causes the started ticket purchaser goroutine (if any) to begin purchasing tickets.

@tuxcanfly
tuxcanfly Dec 9, 2016 edited Member

Yeah, agreed. That will be useful for toggling purchases as well.

@tuxcanfly
tuxcanfly Dec 15, 2016 Member

Done. Using TransactionNotifications to trigger purchases now.

tuxcanfly added some commits Nov 3, 2016
@tuxcanfly tuxcanfly Updated glide 138f01d
@tuxcanfly tuxcanfly Import and initialize ticketbuyer c1670d9
@tuxcanfly tuxcanfly Integrate ticketbuyer 748b3b4
@tuxcanfly tuxcanfly Added config options, pass to NewTicketBuyer ecd83a2
@tuxcanfly tuxcanfly Use ticketbuyer Purchase in connectBlock 492f677
@tuxcanfly tuxcanfly Restore config defaults d76ce43
@tuxcanfly tuxcanfly Only start ticketbuyer if stake mining d8be827
@tuxcanfly tuxcanfly Handle wallet chain client properly 25956f4
@tuxcanfly tuxcanfly Move purchase rpc call outside walletdb tx 51c6291
@tuxcanfly tuxcanfly Fix typo 3eb80d2
@tuxcanfly tuxcanfly Rebased onto latest wallet changes 1135a9f
@tuxcanfly tuxcanfly Updated for dcrticketbuyer usage 55c4e58
@tuxcanfly tuxcanfly Updated comments in ticketbuyer cfcf536
@tuxcanfly tuxcanfly Fix TODO on using config for rpc server 4df54f2
@tuxcanfly tuxcanfly Updated dcrticketbuyer version 8911b4f
@tuxcanfly tuxcanfly Clarify comment on SetTicketPurchaser 35786cf
@tuxcanfly tuxcanfly Log ticket purchase errs fd9867d
@tuxcanfly tuxcanfly Use https git remote 404cd41
@tuxcanfly tuxcanfly Updated ticketbuyer 5ef2105
@tuxcanfly tuxcanfly Limit err scope in purchase 173a2a7
@tuxcanfly tuxcanfly Prompt passphrase if stake mining is enabled 33ba129
@tuxcanfly tuxcanfly Add pkg ticketbuyer 23b25dd
@tuxcanfly tuxcanfly Fix issues found by golint 04678c6
@tuxcanfly tuxcanfly Use new ticketbuyer pkg 398cf1e
@tuxcanfly tuxcanfly Add wallet config 3020b7d
@tuxcanfly tuxcanfly Use defaults for purchase config d6d2fde
@tuxcanfly tuxcanfly Pass ticketbuyer config defaults from main b548ef3
@tuxcanfly tuxcanfly Allow caller to set config cfe6eae
@tuxcanfly tuxcanfly Address review items:
* Use cfg.PurchaseAccount instead of hardcoded default
* Remove unused file
* Use exported consts instead of hardcoded strs
* Use chain client when ready
abb4daf
@tuxcanfly tuxcanfly Rename and export fee, price target consts d3c7c23
@tuxcanfly tuxcanfly Refactor ticketbuyer to use wallet a068420
@tuxcanfly tuxcanfly Use TransactionNotifications to hook ticketbuyer 5e2eafd
@tuxcanfly tuxcanfly Move err check before using purchase hashes f300d89
@tuxcanfly tuxcanfly Avoid blocking new tx notifications f8d9972
@tuxcanfly tuxcanfly Restore tx fee config ec8aef7
@tuxcanfly tuxcanfly ticketbuyer config - Use consts for defaults f0aa1ff
@tuxcanfly tuxcanfly Updated defaults to match dcrticketbuyer conf 51d4b34
@tuxcanfly tuxcanfly Refactor startTicketPurchase initialize 21ccf8c
@@ -0,0 +1,96 @@
+package ticketbuyer
@jrick
jrick Dec 15, 2016 Member

please add a copyright header

@tuxcanfly
tuxcanfly Dec 15, 2016 Member

OK, added.

ticketbuyer/price.go
+// when the chain has not yet sufficiently synced to the chain tip.
+var vwapHeightOffsetErrStr = "beyond blockchain tip height"
+
+// containsHeightOffsetError specifies whether or not the reponse
@jrick
jrick Dec 15, 2016 Member

missing rest of comment

@tuxcanfly
tuxcanfly Dec 15, 2016 Member

Updated. Looks like it was missing from the first commit.

ticketbuyer.go
+
+// purchaseManager is the main handler of websocket notifications to
+// pass to the purchaser and internal quit notifications.
+type purchaseManager struct {
@jrick
jrick Dec 15, 2016 Member

I would really like to see this not part of the main package. Any reason in particular it was added here?

@tuxcanfly
tuxcanfly Dec 15, 2016 Member

Nothing in particular, will try to move it to the pkg.

@tuxcanfly
tuxcanfly Dec 16, 2016 Member

Done, moved to the pkg. Also passing in wallet, to make purchasing dynamic based on setgenerate.

ticketbuyer.go
+ case v := <-p.ntfnChan:
+ if v != nil {
+ for _, block := range v.AttachedBlocks {
+ go p.purchase(int64(block.Height))
@jrick
jrick Dec 15, 2016 edited Member

I think that it should purchase tickets whenever the main chain tip changes, and not several times for each new block in the main chain after a reorganize. Can that be done or will this throw off calculations done by the purchaser?

@jrick
jrick Dec 15, 2016 Member

Safest behavior would be to continue matching dcrticketbuyer, so if it did it this way, and we're not completely sure changing it is correct, then we'll leave it for now.

@tuxcanfly
tuxcanfly Dec 15, 2016 Member

I think dcrticketbuyer was getting block notifications from dcrd, so it might have been only purchasing at the tip as you mentioned. Will cross check that.

@tuxcanfly
tuxcanfly Dec 16, 2016 Member

Leaving it for now, since I see dcrticketbuyer was making purchases for every block received in the notification.

dcrwallet.go
func startPromptPass(w *wallet.Wallet) {
- promptPass := cfg.PromptPass
+ promptPass := cfg.PromptPass || cfg.EnableStakeMining
@jrick
jrick Dec 15, 2016 Member

on second thought this is probably a bad idea since it will break scripts. Probably want to just continue requiring the user to manually unlock the wallet without timeout.

@tuxcanfly
tuxcanfly Dec 15, 2016 Member

OK, restored as before. Purchasing requires unlocked wallet, manually.

@tuxcanfly
Member

Thanks, updated as per the above suggestions. Please review.

Also added the ability to read ticketbuyer.conf from appdata, so that users migrating from dcrticketbuyer can just copy their old conf file. Some of the options from the old conf are superseded by newer options in wallet conf, added deprecation warnings for them. Since this is only intended for migrating users, we're not parsing the command line flags for these new options.

ticketbuyer.go
+
+ // A config file in the current directory takes precedence.
+ exists := false
+ if _, err := os.Stat(defaultTicketBuyerConfigFilename); !os.IsNotExist(err) {
@jrick
jrick Dec 16, 2016 edited Member

I'd remove all this. We don't want stuff in the current directory ever taking precedence. Also doesn't seem to any affect on behavior since if it does exist, it just sets the config file option to the same as the default.

If the config file is missing just return the default config.

if _, err := os.Stat(fileName); os.IsNotExist(err) {
    return cfg, nil
}

I also think this should occur after the main program's config parsing, so if an alternate appdata dir was set, the config will be looked there and not using the default appdata setting. Please pass that value in as a parameter here and make the default relative to that directory.

@tuxcanfly
tuxcanfly Dec 16, 2016 Member

Thanks, will do that.

@tuxcanfly
tuxcanfly Dec 16, 2016 Member

OK, using path relative to appdata and returning default if missing.

ticketbuyer.go
+
+ // Check deprecated aliases.
+ if cfg.AccountName != defaultAccountName {
+ fmt.Fprintln(os.Stderr, "accountname option has been replaced by "+
@jrick
jrick Dec 16, 2016 Member

Since this parsing should be done after the main program config's parsing, and logging should already be set up and ready to use, print these warnings to the log.

@tuxcanfly
tuxcanfly Dec 16, 2016 Member

Thanks, writing log warn now.

ticketbuyer.go
+ "wallet option ticketfee -- please update your config")
+ }
+
+ // Create the home directory if it doesn't already exist.
@jrick
jrick Dec 16, 2016 Member

definitely not needed. if the files exists, the directory it is in already exists too

@tuxcanfly
tuxcanfly Dec 16, 2016 Member

OK, removed.

ticketbuyer/purchase.go
+}
+
+// NotificationHalder handles notifications, which trigger ticket purchases.
+func (p *PurchaseManager) NotificationHalder() {
@jrick
jrick Dec 16, 2016 Member

this is misspelled

@tuxcanfly
tuxcanfly Dec 16, 2016 Member

Thanks, fixed.

dcrwallet.go
+ tcfg, err := loadTicketBuyerConfig()
+ if err != nil {
+ log.Errorf("Unable to load ticket buyer config: %v", err)
+ return
@jrick
jrick Dec 16, 2016 Member

rather than returning, just don't start the purchaser.

@tuxcanfly
tuxcanfly Dec 16, 2016 Member

OK, done.

glide.lock
imports:
- name: github.com/boltdb/bolt
version: 583e8937c61f1af6513608ccc75c97b6abdf4ff9
- name: github.com/btcsuite/btclog
version: 73889fb79bd687870312b6e40effcecffbd57d30
- name: github.com/btcsuite/fastsha256
version: 637e656429416087660c84436a2a035d69d54e2e
+- name: github.com/btcsuite/go-flags
@jrick
jrick Dec 16, 2016 Member

I don't see addition of this to the glide.yaml.. but anyways this shouldn't be added. We switched to upstream repo pkg path. You can see github.com/jessevdk/go-flags further down in the lockfile. Please change all uses of btcsuite/go-flags to jessevdk/go-flags to avoid pulling the same package twice under different names.

@tuxcanfly
tuxcanfly Dec 16, 2016 Member

Thanks, done.

@tuxcanfly tuxcanfly Address review items:
* Remove unnecessary appdata dir create
* Rename misspelled method
* Log warnings
* Don't start purchaser in case of config err
* Use jessevdk/go-flags, remove glide.lock entry
* Remove current directory precedence
* Check missing file relative to appdata
9bd8a50
@@ -285,6 +286,36 @@ func rpcClientConnectLoop(legacyRPCServer *legacyrpc.Server, loader *wallet.Load
if legacyRPCServer != nil {
legacyRPCServer.SetChainServer(chainClient)
}
+ tcfg, err := loadTicketBuyerConfig(cfg.AppDataDir)
+ if err != nil {
+ log.Errorf("Unable to load ticket buyer config: %v", err)
@jrick
jrick Dec 16, 2016 Member

Can you also add to this message that it won't purchase tickets?

@tuxcanfly
tuxcanfly Dec 16, 2016 Member

Thanks, added.

dcrwallet.go
@@ -289,6 +289,7 @@ func rpcClientConnectLoop(legacyRPCServer *legacyrpc.Server, loader *wallet.Load
tcfg, err := loadTicketBuyerConfig(cfg.AppDataDir)
if err != nil {
log.Errorf("Unable to load ticket buyer config: %v", err)
+ log.Info("Ticket purchase is disabled")
@jrick
jrick Dec 16, 2016 Member

purchasing

@tuxcanfly tuxcanfly Address review items:
* Add ticket purchase disabled message
e25e148
@dajohi
Member
dajohi commented Dec 16, 2016

Tested OK with 2 wallets sharing a seed.

@jrick
jrick approved these changes Dec 16, 2016 View changes
@jrick jrick merged commit d1bc925 into decred:master Dec 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment