decred / dcrstakepool

Stakepool for Decred.
Other
72 stars 75 forks source link

stakepoold: Deprecate wallet RPC ListScripts #606

Closed JoeGruffins closed 4 years ago

JoeGruffins commented 4 years ago

Use UserVotingConfig keys and RPC command ValidateAddress instead.

jrick commented 4 years ago

functionally this looks fine and it works in a pinch when using the most recent wallet but i don't believe this is the best way to go about this. ideally it would be comparing addresses across wallets, and only looking up the scripts for addresses that some wallet was missing. getaddressesbyaccount appears broken on the imported account in dcrwallet right now however...

doesn't stakepoold already have a complete list of all addresses it should be watching for ticket purchases? could that be used instead?

JoeGruffins commented 4 years ago

@jrick We have to check to make sure that each wallet has all user redeem scripts. Are you saying that there's a better way to check than validateaddress? If so we will use that.

jrick commented 4 years ago

there should be, but getaddressesbyaccount seems broken for the imported account currently. if that worked, you could easily check for all p2sh addresses for each script.

in lieu of that, you can perform validateaddress calls checking to see if each result for the p2sh addresses has "ismine": true. if one is not, these can be recorded per-wallet, fetching the script from other sources (e.g. other wallets which had them perhaps), and imported.

JoeGruffins commented 4 years ago

Created an issue #613

Also, I just realized that stakepoold works without a connection to the database. I think that needs to be changed.

JoeGruffins commented 4 years ago

@jrick

in lieu of that, you can perform validateaddress calls checking to see if each result for the p2sh addresses

Are they multisig or p2sh addresses? They are two different things, correct? do multisig need to be followed by a p2sh script to be spent?

jrick commented 4 years ago

Currently, the ticket outputs are p2sh. the p2sh redeem script is a 1-of-2 multisig.

Bare multisig outputs are nonstandard, so they are rarely if ever seen used directly without p2sh.

JoeGruffins commented 4 years ago

@jholdstock @dajohi Unless I'm mistaken, master is currently broken without these changes because #609 has been merged?

itswisdomagain commented 4 years ago

there should be, but getaddressesbyaccount seems broken for the imported account currently. if that worked, you could easily check for all p2sh addresses for each script.

in lieu of that, you can perform validateaddress calls checking to see if each result for the p2sh addresses has "ismine": true. if one is not, these can be recorded per-wallet, fetching the script from other sources (e.g. other wallets which had them perhaps), and imported.

dcrstakepool has the list of p2sh addresses it watches, together with the redeem script for each. Calling validateaddress for each address per stakepoold instance/wallet would be a lot of rpc calls being made when dcrstakepool starts up, all just to determine which wallet is missing which script, if any.

I don't know if it was discussed before, the possibility of passing multiple addresses to the validateaddress rpc method. If that's doable, the number of rpc calls to check for missing scripts would reduce to perhaps 1 per stakepoold instance/wallet.

Might be a better idea to fix getaddressesbyaccount for imported accounts and use that instead, to reduce the number of rpc calls to 1 per wallet (excluding calls to import missing scripts).

jrick commented 4 years ago

I forgot to update here, but getaddressesbyaccount on the imported account is fixed in dcrwallet master.

JoeGruffins commented 4 years ago

will try to solve #613 in another pr, using getaddressesbyaccount.

JoeGruffins commented 4 years ago

619 which uses getaddressesbyaccount should be considered instead of this pr