Let -zapwallettxes recover transaction meta data #3674

cozz commented at 7:20 PM on February 14, 2014: contributor

zapwallettxes currently loses wallet transaction meta data information like:

vOrderForm
strFromAccount
nOrderPos
...

This pull stores all wallet transaction meta data before erasing the transactions, and then restores this information after rescan. Only to those transactions being recovered by rescan obviously.

gmaxwell commented at 8:34 PM on February 14, 2014: contributor

On one hand this makes it much less disruptive, on the other hand it might be corruption in that data which you're trying to solve. So I think this should also be switchable so that someone can choose to zap harder.

Also— should we try to restore metadata for mutants to the surviving transaction? I don't think using a normalized txid for this is correct, in the case that the wallet may contain some non-sighash-all transactions. So perhaps handling that is too tricky.

jgarzik commented at 8:38 PM on February 14, 2014: contributor

It is intentionally a blunt instrument. Handling the metadata problem is always going to be a partial solution, kinda-sorta handling recovery kinda-sorta well.

Let's get some feedback on need? Unless we have a lot of sites who are not served by this simple procedure, lean towards punting.

Anyone may modify https://github.com/gavinandresen/bitcointools fixwallet.py to handle a specific, uncommon use case.

laanwj commented at 10:27 AM on February 15, 2014: member

It would be useful to have an option to keep metadata with -zapwallettx.

Would it be possible to build this on top of #3671 and re-use Gavin's metadata-copying and duplicate matching? I understand that the problem is different here: you want to match rescanned tranactions against the old contents of the wallet, instead of matching current transactions against each other. So maybe it doesn't help at all...

cozz commented at 12:09 PM on February 15, 2014: contributor

update:

add parameter <mode>, so -zapwallettxes=<mode>
- -zapwallettxes or -zapwallettxes=1 => drop meta data
- -zapwallettxes=2 => keep meta data

change desription from

Clear list of wallet transactions (diagnostic tool; implies -rescan)

to

Clear list of wallet transactions and recover those part of the blockchain
through -rescan on startup (diagnostic tool, default: 0)
(mode:1 = drop transaction meta data, mode:2 = keep transaction meta
data like account owner and payment request information)

copy and pasted Gavin's copyTo=copyFrom, including comments and variable names

I did not put this on top of #3671 , the function SyncMetaData is not reusable for this currently. But the 5 lines of code look identical now, so in the future we can easily make a "copyMeta()" function, used by both.

in src/init.cpp:None in afd3ae0d11 outdated

 269 | @@ -270,7 +270,8 @@ bool static Bind(const CService &addr, unsigned int flags) {
 270 |      strUsage += "  -disablewallet         " + _("Do not load the wallet and disable wallet RPC calls") + "\n";
 271 |      strUsage += "  -paytxfee=<amt>        " + _("Fee per kB to add to transactions you send") + "\n";
 272 |      strUsage += "  -rescan                " + _("Rescan the block chain for missing wallet transactions") + "\n";
 273 | -    strUsage += "  -zapwallettxes         " + _("Clear list of wallet transactions (diagnostic tool; implies -rescan)") + "\n";
 274 | +    strUsage += "  -zapwallettxes=<mode>  " + _("Clear list of wallet transactions and recover those part of the blockchain through -rescan on startup (diagnostic tool, default: 0)") + "\n";
 275 | +    strUsage += "                         " + _("(mode:1 = drop transaction meta data, mode:2 = keep transaction meta data like account owner and payment request information)") + "\n";

laanwj commented at 12:11 PM on February 15, 2014:

You default to mode 0, but only document mode 1 and 2. Is there a third mode at all? If not, I'd make the 0 and 1 Edit: oh wait, with 0 you mean "don't zap txes". This is a bit confusing doc IMO :) Maybe: The default is 1 if the option is provided.

cozz commented at 12:43 PM on February 15, 2014:

Indeed confusing, I will change the documentation.

cozz commented at 4:34 PM on February 15, 2014: contributor

update:

removed the "default: 0" from the documentation to avoid confusion there

in src/init.cpp:None in ca5ce3e0bf outdated

 269 | @@ -270,7 +270,8 @@ bool static Bind(const CService &addr, unsigned int flags) {
 270 |      strUsage += "  -disablewallet         " + _("Do not load the wallet and disable wallet RPC calls") + "\n";
 271 |      strUsage += "  -paytxfee=<amt>        " + _("Fee per kB to add to transactions you send") + "\n";
 272 |      strUsage += "  -rescan                " + _("Rescan the block chain for missing wallet transactions") + "\n";
 273 | -    strUsage += "  -zapwallettxes         " + _("Clear list of wallet transactions (diagnostic tool; implies -rescan)") + "\n";
 274 | +    strUsage += "  -zapwallettxes=<mode>  " + _("Clear list of wallet transactions and recover those part of the blockchain through -rescan on startup (diagnostic tool)") + "\n";
 275 | +    strUsage += "                         " + _("(mode:1 = drop transaction meta data, mode:2 = keep transaction meta data like account owner and payment request information)") + "\n";

Diapolo commented at 1:25 AM on February 16, 2014:

I would go for (to stay in line with current strings and also be a little shorter): (default: 1, 1 = drop tx meta data, 2 = keep tx meta data e.g. account owner and payment request information)

Also -salvagewallet doesn't tell it starts a rescan, IMHO this description it too long... what tells diagnostic tool to a normal user? I wouldn't think it is that invasive at all, if I don't know what I'm doing.

laanwj commented at 12:07 PM on February 17, 2014:

Making 'recover metadata' be the default mode would make sense IMO as it is the least invasive. Then if that isn't enough, the user can use the other mode that drops metadata too.

laanwj commented at 12:10 PM on February 17, 2014: member

ACK apart from nit about defaults

cozz commented at 1:57 PM on February 17, 2014: contributor

update:

changed description to diapolos version
default is now to keep meta data

Diapolo commented at 2:48 PM on February 17, 2014: none

Thanks for updating the help string, looks good now.

gavinandresen commented at 5:17 PM on February 17, 2014: contributor

A new regression test (or tests) for this in qa/rpc-tests/ would be spiffy. See #3694 for examples to follow.

cozz commented at 1:14 PM on February 25, 2014: contributor

update:

added qa/rpc-tests/zapwallettxes.sh
- create 2 independent nodes, both:
  - create a confirmed transaction without account information
  - create a confirmed transaction with account information
  - create an unconfirmed transaction
  - node 1 runs zapwallettxes with mode 1
  - node 2 runs zapwallettxes with mode 2
  - check if the confirmed transactions are still there
  - check if the unconfirmed transaction is gone
  - node 1 checks account balance assuming the account information has been kept
  - node 2 checks account balance assuming the account information has been dropped @gavinandresen I added a general CleanUp call to util.sh. Hope you are fine with this. Because currently in error case, the nodes keep running and the data-directory gets not deleted. I always had to manually kill the nodes and remove the directory.

gavinandresen commented at 1:22 PM on February 25, 2014: contributor

Awesome, thanks @cozz !

RE: CleanUp/util.sh : ACK.

laanwj commented at 3:28 PM on June 16, 2014: member

Needs rebase

Let -zapwallettxes recover transaction meta data 77cbd4623e

cozz commented at 7:15 PM on June 16, 2014: contributor

Rebased. I also changed the help-description and do now recover the fields nOrderPos and nTimeReceived. I dont think there is a reason not to recover them. Just executed the zapwallettxes.sh test, still works.

BitcoinPullTester commented at 4:15 PM on June 23, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p3674_77cbd4623e171ee9c48ada8a421295ed2c8e6c7c/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

laanwj merged this on Jun 24, 2014

laanwj closed this on Jun 24, 2014

laanwj referenced this in commit d4392c8989 on Jun 24, 2014

DrahtBot locked this on Sep 8, 2021