Add Sweep capability to Ravencoin Core

TronBlack commented 3 years ago

Not a bug - but a new feature.

Add the ability to sweep a private key.

RPC call for sweep [RVN | ]

Creates a transaction to transfer all RVN, and all Assets from a given address -- with only the private key as input.

Default to funding from RVN held in the address, fallback to using RVN held in wallet for transaction fee.

Default to sweeping all assets, but can also all with RVN to sweep only RVN, or to sweep only one asset.

This differs from import because a paper certficate provided with artwork or a one-of-a-kind item can include a paper certficate-of-authenticity. Once swept it the paper certificate can be safely discarded as the token is secured by the new address.

Requirements:

Make API calls for UTXOs for the address associated with the private key (default api.ravencoin.org)
Construct transaction for all RVN and all assets unless only one is specified in RPC call
Use RVN in the address associated with the private key for fees (if RVN exists), otherwise use wallet RVN.
Send to newly derived address in wallet.dat
Sign and broadcast transaction.

RavencoinFoundationTron commented 3 years ago

amount=USD$2000.00 paid_in=RVN status=open

TronBlack commented 3 years ago

In progress

Nicholas

HyperPeek commented 3 years ago

Should be added in #909 will close once confirmed working as expected

jeroz1 commented 3 years ago

This is not working yet on testnet.

Error: No information available for address (code -5)

Method used:

dumpprivkey PUBLICKEY
sweep PRIVATEKEY

jeroz1 commented 3 years ago

Oh... it needs '-assetindex', '-txindex', '-addressindex' ?

That's quite bothersome and unworkable for everyday users.

fdoving commented 3 years ago

Does not fill the requirements, with API calls.

jeroz1 commented 3 years ago

Ok, I can confirm that it works, but it requires assetindex, txindex, and addressindex.

$dumpprivkey mqBd2ZxMXmCS9oP8imLQ7yVGY6y9iiov4m
cQttWw7LkTnQxiCSnrRATi7sRpqi19EbchiBnJvNUySekWrkLK79

$sweep cQttWw7LkTnQxiCSnrRATi7sRpqi19EbchiBnJvNUySekWrkLK79
c75ca60da70d463caaa78e3bea9fb7d684da40e27ce8ce5a7464acb41515734f

https://rvnt.cryptoscope.io/tx/?txid=c75ca60da70d463caaa78e3bea9fb7d684da40e27ce8ce5a7464acb41515734f

The importprivkey function does not need these indices and the wallet is also able to read the contents of it. Would it be an idea to similarly trigger a rescan to get the content from the private key and then use that info to sweep it?

TronBlack commented 3 years ago

importprivkey was a great option prior to moving to a 12-word seed as the backup. After the default became the 12-word seed, then importprivkey adds a new private key to the database that isn't backed up unless the user saves the wallet.dat file. The software will notify the user that wallet.dat needs to be backed up if importprivkey is used.

It isn't ideal to have to add '-assetindex', '-txindex', '-addressindex', but it does keep the core wallet independent of external apis, but as long as use of the sweep function notifies the user that they need to add '-assetindex', '-txindex', '-addressindex', then it is an acceptable solution.

The sweep call can be extended later to use APIs if '-assetindex', '-txindex', '-addressindex' are not set.

Using the internal database is a better solution (if the indexes are set).

jeroz1 commented 3 years ago

importprivkey was a great option prior to moving to a 12-word seed as the backup. After the default became the 12-word seed, then importprivkey adds a new private key to the database that isn't backed up unless the user saves the wallet.dat file. The software will notify the user that wallet.dat needs to be backed up if importprivkey is used.

I understand but I was suggesting to smartly use the rescan feature (as how it's also used for importing private keys) to avoid having to use all the index parameters.

It isn't ideal to have to add '-assetindex', '-txindex', '-addressindex', but it does keep the core wallet independent of external apis, but as long as use of the sweep function notifies the user that they need to add '-assetindex', '-txindex', '-addressindex', then it is an acceptable solution.

Yes, but the wallet does not give this warning. Instead it gives No information available for address (code -5).

The sweep call can be extended later to use APIs if '-assetindex', '-txindex', '-addressindex' are not set. Using the internal database is a better solution (if the indexes are set).

I agree and would also argue against hard coding external APIs into core.

I would also suggest extending this feature to sweep entire 12-word seed wallets.

fdoving commented 3 years ago

I think this feature is close to useless in its current form.

It all depends on the issue we are trying to solve with this feature. Hacking config files and waiting forever to move some coins is not enough in 2021 imo.

The api version would just move your coins to an address known by your wallet in seconds/minutes. This is what I want. Then the current implementation could be the fall-back if some kind of disaster struck and all available apis went offline.

If the problem we are trying to solve is to help users move their coins around, I would do it as described in the original issue. The API-url does not need to be hard-coded. We could put in a default in the config, and make it overridable on the commandline and/or in some setting.

TronBlack commented 3 years ago

jeroz1 - That's a great idea.

So, if I understand correctly.....

importprivkey for the private key being swept with rescan left as true
Allow the rescan to happen which will add asset UTXOs, and RVN UTXOs to the internal database
Then continue the sweep (as written), but now the internal database has the necessary UTXOs without having to do txindex, assetindex, and addressindex which indexes EVERYONES UTXOs.

I like it.

The side-effect will be one private key left in the wallet.dat, but it should be empty after the sweep.

Addressing the 12-word sweep.... The 12-word sweep could be done the same way, except without using an API would require importingprivkey and indexing (20+<# of used addresses>) * 2 private keys.

20 because you have to watch for an empty gap of 20
Number of used addresses because you have to sweep those.
Times 2 - because you have to do a scan for /0 and /1 for change addresses.

fdoving commented 3 years ago

I suggest that we do not include this code in its current form.

It should be adjusted to work like @jeroz1 suggests and @TronBlack describes in the previous comment. (excluding the 12-word sweep).

At first glance it will require some work, as the current implementation separates the known wallet from the process.

PS: A reindex with all the indexes on testnet is around 3 hours. (HW: NVME SSD, 32G, ryzen 3700x)

jeroz1 commented 3 years ago

The sweep function is unable to sweep https://rvnt.cryptoscope.io/address/?address=mj15TuQH36sA6wxdCaNWJz4GrHMQp6rryv

$sweep PRIVATEKEY
Couldn't decode asset script (code -32603)

jeroz1 commented 3 years ago

Sweep does not work for the combination of RVN plus a restricted asset.

My tests:

- Sweep RESPECTMYAUTHORITY
success
- Sweep $RESPECTMYAUTHORITY
succes
- Sweep RESPECTMYAUTHORITY/1
success
- Sweep RESPECTMYAUTHORITY/1#2
succes
- Sweep RESPECTMYAUTHORITY~MESS
fail
`Funds available does not match funds required. Do you have unsafe transactions? (code -6)`
- Moving funds to a new address
- Sweep RESPECTMYAUTHORITY~MESS
success
- Sweep RESPECTMYAUTHORITY/1~MESS
success
- Sweep #STICK
success
- Sweep #STICK/#BRANCH
success
- Sweep #STICK #STICK/#BRANCH $RESPECTMYAUTHORITY RESPECTMYAUTHORITY RESPECTMYAUTHORITY/1 RESPECTMYAUTHORITY/1#2 RESPECTMYAUTHORITY/1~MESS RESPECTMYAUTHORITY~MESS
fail
`66: min relay fee not met (code -26)`
- Sweep RVN #STICK #STICK/#BRANCH $RESPECTMYAUTHORITY RESPECTMYAUTHORITY RESPECTMYAUTHORITY/1 RESPECTMYAUTHORITY/1#2 RESPECTMYAUTHORITY/1~MESS RESPECTMYAUTHORITY~MESS
fail
Couldn't decode asset script (code -32603)
- Sweep RVN RESPECTMYAUTHORITY
success
- Sweep RVN RESPECTMYAUTHORITY $RESPECTMYAUTHORITY
fail
`Couldn't decode asset script (code -32603)` 
- Sweep RESPECTMYAUTHORITY $RESPECTMYAUTHORITY
success
- Sweep RVN #STICK #STICK/#BRANCH
succes
- Sweep #STICK #STICK/#BRANCH $RESPECTMYAUTHORITY
success
- Sweep RVN #STICK #STICK/#BRANCH $RESPECTMYAUTHORITY
fail
`Couldn't decode asset script (code -32603)` 
- Sweep RVN #STICK/#BRANCH $RESPECTMYAUTHORITY
fail
`Couldn't decode asset script (code -32603)` 
- Sweep RVN $RESPECTMYAUTHORITY
fail
`Couldn't decode asset script (code -32603)`

nicholascioli commented 3 years ago

Hello everyone. Sorry for the delay. I'd like to give a bit of context as to why some of the decisions were made and would like guidance on how to resolve these issues.

When implementing the sweep functionality, I originally tried to tie in to the existing API infrastructure to avoid indexing everything, but at the time the testnet API was down. I then decided to try to make use of the importprivkey RPC call so that I could take advantage of the Wallet helper methods without having to manually create the transaction, but every attempt to import a private key in testnet would result in an RPC timeout error. Only after testing both of those methods did I move towards the current implementation.

Is there a way to do a quick rescan that doesn't trigger the RPC timeout? If so, I wouldn't mind going back and reimplementing the sweep feature to make use of that, as the code would be significantly easier in doing so.

hans-schmidt commented 3 years ago

Nicholas, thank you for supporting the code you provided. Could you please re-try the "importprivkey" RPC call? Maybe testnet was down at that time for some reason. I just did a test on testnet using "raven-cli importprivkey" to import a key from another wallet into a new wallet. The RPC call responded immediately, showing a pop-up % progress bar for the rescan. The testnet rescan completed and the associated imported assets showed up in the new wallet in under 20 seconds.

nicholascioli commented 3 years ago

I've been a bit busy lately, but I'll revisit this soon and see if the issues I had with importing are resolved.

RavenProject / Ravencoin

Add Sweep capability to Ravencoin Core #880