Add per-ticket treasury key and tspend policies #2053

jrick · 2021-05-19T16:08:17Z

The four JSON-RPC methods that deal with setting and returning
approval policies for tspend transactions (settreasurypolicy,
settspendpolicy, treasurypolicy, tspendpolicy) gain an additional
optional parameter for a ticket hash. When this hash is provided, the
policies set or returned by these methods are bound to a specific
ticket.

This functionality will be used by vspd to set per-ticket tspend
approval policies on its voting wallets. Solo voters should never
need to use the per-ticket policies.

Closes #2051.

Note to reviewers: this contains a database upgrade.

jholdstock · 2021-05-21T06:52:43Z

I don't think the VSP... prefix should be used on names throughout this PR. There are other potential uses for per-ticket voting preferences.

For example, a single wallet may hold tickets belonging to different people with different voting preferences - eg. a married couple. Or even a single individual may want to have their tickets voting different ways for some reason.

Its a generic feature, it doesn't need to be tied to VSPs through naming.

jholdstock · 2021-05-21T09:14:02Z

internal/rpchelp/helpdescs_en_US.go

 	"tspendpolicy--condition0": "no tspend hash provided",
 	"tspendpolicy--condition1": "tspend hash specified",
 	"tspendpolicy--result0":    "Array of all non-abstaining policies for known tspends",
 	"tspendpolicy--result1":    "Voting policy for a particular tspend hash",

 	"tspendpolicyresult-hash":   "Treasury spend transaction hash",
 	"tspendpolicyresult-policy": "Voting policy description (abstain, yes, or no)",
+	"tspendpolicyresult-ticket": "Ticket hash fo a per-ticket tspend approval policy",


jholdstock · 2021-05-21T09:19:31Z

wallet/udb/upgrades.go

+
+	metadataBucket := tx.ReadWriteBucket(unifiedDBMetadata{}.rootBucketKey())
+
+	// Assert that this function is only called on version 21 databases.


jholdstock · 2021-05-21T09:21:23Z

internal/rpc/jsonrpc/rpcserverhelp.go

-		"treasurypolicy":            "treasurypolicy (\"key\")\n\nReturn voting policies for treasury spend transactions by key\n\nArguments:\n1. key (string, optional) Return the policy for a particular key\n\nResult (no key provided):\n[{\n \"key\": \"value\",    (string) Treasury key associated with a policy\n \"policy\": \"value\", (string) Voting policy description (abstain, yes, or no)\n},...]\n\nResult (key specified):\n{\n \"key\": \"value\",    (string) Treasury key associated with a policy\n \"policy\": \"value\", (string) Voting policy description (abstain, yes, or no)\n}                   \n",
-		"tspendpolicy":              "tspendpolicy (\"hash\")\n\nReturn voting policies for treasury spend transactions\n\nArguments:\n1. hash (string, optional) Return the policy for a particular tspend hash\n\nResult (no tspend hash provided):\n[{\n \"hash\": \"value\",   (string) Treasury spend transaction hash\n \"policy\": \"value\", (string) Voting policy description (abstain, yes, or no)\n},...]\n\nResult (tspend hash specified):\n{\n \"hash\": \"value\",   (string) Treasury spend transaction hash\n \"policy\": \"value\", (string) Voting policy description (abstain, yes, or no)\n}                   \n",
+		"treasurypolicy":            "treasurypolicy (\"key\" \"ticket\")\n\nReturn voting policies for treasury spend transactions by key\n\nArguments:\n1. key    (string, optional) Return the policy for a particular key\n2. ticket (string, optional) Return policies used by a specific ticket hash\n\nResult (no key provided):\n[{\n \"key\": \"value\",    (string) Treasury key associated with a policy\n \"policy\": \"value\", (string) Voting policy description (abstain, yes, or no)\n \"ticket\": \"value\", (string) Ticket hash of a per-ticket treasury key approval policy\n},...]\n\nResult (key specified):\n{\n \"key\": \"value\",    (string) Treasury key associated with a policy\n \"policy\": \"value\", (string) Voting policy description (abstain, yes, or no)\n \"ticket\": \"value\", (string) Ticket hash of a per-ticket treasury key approval policy\n}                   \n",
+		"tspendpolicy":              "tspendpolicy (\"hash\" \"ticket\")\n\nReturn voting policies for treasury spend transactions\n\nArguments:\n1. hash   (string, optional) Return the policy for a particular tspend hash\n2. ticket (string, optional) Return policies used by a specific ticket hash\n\nResult (no tspend hash provided):\n[{\n \"hash\": \"value\",   (string) Treasury spend transaction hash\n \"policy\": \"value\", (string) Voting policy description (abstain, yes, or no)\n \"ticket\": \"value\", (string) Ticket hash fo a per-ticket tspend approval policy\n},...]\n\nResult (tspend hash specified):\n{\n \"hash\": \"value\",   (string) Treasury spend transaction hash\n \"policy\": \"value\", (string) Voting policy description (abstain, yes, or no)\n \"ticket\": \"value\", (string) Ticket hash fo a per-ticket tspend approval policy\n}                   \n",


jrick · 2021-05-21T15:57:04Z

udb really should be an internal package and we've made some progress towards that, but unfortunately it is not there yet because there are still some exposed types from the package that are returned by the wallet package. all of the VSP naming in the api in this diff is there, not in the wallet package.

However when this eventually gets backported to the 1.6 branch, in order to not break compatibility, we'll have to introduce these wallet package methods under some new names. I was still going to use VSP there just because it is short and to the point.

Even with that contrived example, I don't see harm in using "VSP". One individual is still providing a voting service to another, even if they are not doing it for a fee.

jholdstock · 2021-05-22T04:33:07Z

Agree its contrived, and its definitely not a showstopper, but I will always advocate for making a feature generic if possible, especially if it comes at no additional cost (as in this case).

wallet/udb/treasury.go

matheusd

Noticed while testing that SetTSpendPolicy, SetTreasuryKeyPolcy and SetVSPTreasuryKeyPolcy calls are also missing the early return in the if policy == stake.TreasuryVoteInvalid branch.

matheusd · 2021-06-03T22:04:41Z

wallet/wallet.go

+		if ok {
+			return policy
+		}
+	} else if policy, ok := w.tspendPolicy[*tspendHash]; ok {


This can't be an else if, it needs to be a separate if, otherwise if you set a global policy for a specific tspend (that is, without specifying a ticket hash) the wallet won't vote for it because the ticketHash argument is always filled now.

matheusd · 2021-06-05T14:22:32Z

wallet/wallet.go

@@ -557,7 +557,8 @@ func (w *Wallet) TSpendPolicy(tspendHash, ticketHash *chainhash.Hash) stake.Trea
 		if ok {
 			return policy
 		}
-	} else if policy, ok := w.tspendPolicy[*tspendHash]; ok {
+	}
+	if policy, ok := w.tspendPolicy[*tspendHash]; ok {


Re-testing after the review fixes, I only have one test scenario where the result was unexpected but not necessarily wrong, so I'm going to point it out but will defer to what you or @jholdstock feels is most appropriate.

As currently implemented, the priority for deciding the voting policy is:

ticket + tspend policy
global tspend policy
ticket + key policy
global key policy

This means a vspd voting wallet configured with a global tspend policy (that is, a policy configured with settspendpolicy <tspend> <yes|no>) will override a ticket-specific key policy (one configured with settreasurypolicy <pi key> <yes|no> <ticket>).

Obviously, vspd voting wallets shouldn't have global policies set IMO (either per pi key or per tspend) so this could be a moot point anyway, but I wanted to surface it since it was unexpected to me.

A vspd voting wallet should either use the per-ticket peference, or abstain. I dont think it should ever use a global policy.

vspd already does some checks on wallet config to ensure things like enablevoting=true and unlocked=true. If wallet were to support it, vspd could also perform checks to validate that no global policies are set.

We could even go as far as to have vspd remove any global policies from its voting wallets (again, if wallet were to support that).

[...] have vspd remove any global policies from its voting wallets (again, if wallet were to support that).

This is supported in this PR by calling settreasurypolicy [key] abstain or settspendpolicy [tspend] abstain. This removes the configured global policy.

vspd could also perform checks to validate that no global policies are set.

I guess this could be marginally useful, though there's no guarantee the vsp operator won't just modify the global policy after vspd performs this check.

Ultimately, what needs to happen is for the wallet to be able to check the actual votes issued by a vsp and compare that to what the wallet configured the vsp to do so that we can it can build some stats on whether the vsp is actually voting according to user preferences and report that somewhere. Plenty of edge cases to deal with that though.

Ultimately, what needs to happen is for the wallet to be able to check the actual votes issued by a vsp and compare that to what the wallet configured the vsp to do so that we can it can build some stats on whether the vsp is actually voting according to user preferences and report that somewhere. Plenty of edge cases to deal with that though.

doing this properly would also require us to save all of the vsp api responses so all updates for a ticket can be demonstrated without relying on easily faked user statistics. that's out of scope for this pr.

So I think the important thing for now then is just to ensure that a per-ticket setting will override a global setting.

The four JSON-RPC methods that deal with setting and returning approval policies for tspend transactions (settreasurypolicy, settspendpolicy, treasurypolicy, tspendpolicy) gain an additional optional parameter for a ticket hash. When this hash is provided, the policies set or returned by these methods are bound to a specific ticket. This functionality will be used by vspd to set per-ticket tspend approval policies on its voting wallets. Solo voters should never need to use the per-ticket policies.

jholdstock reviewed May 21, 2021

View reviewed changes

matheusd reviewed May 27, 2021

View reviewed changes

wallet/udb/treasury.go Show resolved Hide resolved

jrick force-pushed the pertickettreasurypolicy branch 3 times, most recently from 2b3710c to ccdc76a Compare May 28, 2021 14:05

matheusd reviewed Jun 3, 2021

View reviewed changes

matheusd reviewed Jun 5, 2021

View reviewed changes

jrick force-pushed the pertickettreasurypolicy branch 3 times, most recently from 4e73041 to f9f9360 Compare June 7, 2021 14:29

jrick force-pushed the pertickettreasurypolicy branch from f9f9360 to c4e4033 Compare September 13, 2021 14:54

jrick force-pushed the pertickettreasurypolicy branch from c4e4033 to 07152b1 Compare November 11, 2021 14:05

alexlyp approved these changes Dec 7, 2021

View reviewed changes

jrick force-pushed the pertickettreasurypolicy branch from 07152b1 to 623c1fc Compare December 7, 2021 17:58

jrick merged commit 623c1fc into decred:master Dec 7, 2021

jrick deleted the pertickettreasurypolicy branch December 7, 2021 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add per-ticket treasury key and tspend policies #2053

Add per-ticket treasury key and tspend policies #2053

jrick commented May 19, 2021 •

edited

jholdstock commented May 21, 2021

jholdstock May 21, 2021

jholdstock May 21, 2021

jholdstock May 21, 2021

jrick commented May 21, 2021

jholdstock commented May 22, 2021

matheusd left a comment

matheusd Jun 3, 2021

matheusd Jun 5, 2021

jholdstock Jun 6, 2021

matheusd Jun 7, 2021

jrick Jun 7, 2021

jholdstock Jun 8, 2021


		metadataBucket := tx.ReadWriteBucket(unifiedDBMetadata{}.rootBucketKey())

		// Assert that this function is only called on version 21 databases.

Add per-ticket treasury key and tspend policies #2053

Add per-ticket treasury key and tspend policies #2053

Conversation

jrick commented May 19, 2021 • edited

jholdstock commented May 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrick commented May 21, 2021

jholdstock commented May 22, 2021

matheusd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrick commented May 19, 2021 •

edited