test: Extend salvage test for wallet tool #19078

maflcko · 2020-05-27T12:41:00Z

Extend the test to check the balance remains the same before and after the salvage

jonasschnelli · 2020-05-28T07:39:26Z

Test seems to fail:
https://bitcoinbuilds.org/index.php?job=b27a270b-c15a-4a48-bf46-26b3cc36a464

DrahtBot · 2020-06-02T02:43:35Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

jonatack · 2020-06-05T18:05:47Z

Concept ACK.

Seeing this error: Assertion `new_tx' failed:

2020-06-05T18:01:34.518000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_hf5xwzyh
2020-06-05T18:01:34.975000Z TestFramework (INFO): Testing that various invalid commands raise with specific error messages
2020-06-05T18:01:35.809000Z TestFramework (INFO): Calling wallet tool info, testing output
2020-06-05T18:01:36.535000Z TestFramework (INFO): Generating transaction to mutate wallet
2020-06-05T18:01:39.036000Z TestFramework (INFO): Calling wallet tool info after generating a transaction, testing output
2020-06-05T18:01:39.238000Z TestFramework (INFO): Calling wallet tool create on an existing wallet, testing output
2020-06-05T18:01:41.947000Z TestFramework (INFO): Starting node with arg -wallet=foo
2020-06-05T18:01:42.716000Z TestFramework (INFO): Calling getwalletinfo on a different wallet ("foo"), testing output
2020-06-05T18:01:42.975000Z TestFramework (INFO): Check salvage
2020-06-05T18:01:51.543000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 119, in main
    self.run_test()
  File "test/functional/tool_wallet.py", line 245, in run_test
    self.test_salvage()
  File "test/functional/tool_wallet.py", line 232, in test_salvage
    self.assert_tool_output('', '-wallet=salvage', 'salvage')
  File "test/functional/tool_wallet.py", line 52, in assert_tool_output
    assert_equal(stderr, '')
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/util.py", line 46, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(bitcoin-wallet: wallet/walletdb.cpp:289: auto ReadKeyValue(CWallet *, CDataStream &, CDataStream &, CWalletScanState &, std::string &, std::string &)::(anonymous class)::operator()(CWalletTx &, bool) const: Assertion `new_tx' failed.
 == )
2020-06-05T18:01:51.595000Z TestFramework (INFO): Stopping nodes

achow101 · 2020-07-22T16:36:11Z

test/functional/tool_wallet.py

        self.stop_node(0)

        self.assert_tool_output('', '-wallet=salvage', 'salvage')

+        self.start_node(0, ['-wallet=salvage'])


I had to add self.nodes[0].rescanblockchain(0) to make this pass after merging with #19457

achow101 · 2020-08-25T17:29:48Z

#19805 is a potential solution to the test failure as it just avoids deserializing those records entirely.

#19793 is a another potential solution to the test failure where the assert is removed.

ryanofsky · 2020-08-25T18:56:41Z

#19805 is a potential solution to the test failure as it just avoids deserializing those records entirely.

#19793 is a another potential solution to the test failure where the assert is removed.

Maybe I should look at the test to understand better, but both of these changes are avoiding failures when there are duplicate transactions in the database. But we still don't know how duplicate transactions are winding up in the database to begin with?

achow101 · 2020-08-25T19:03:37Z

But we still don't know how duplicate transactions are winding up in the database to begin with?

The duplicate transactions are showing up in the records the salvage pulls from the database. This doesn't mean the database actually contains duplicate transactions but is actually more of an artifact of the aggressive salvage.

Specifically, the aggressive salvage will likely recover deleted data as such data is not actually overwritten, just marked deleted. The aggressive salvage ignores the deletion flag so it will pull up those records. In this test, the Btree ends up being rebalanced which involves moving data around and this is done via copy and delete. So the aggressive salvage is finding records in the "deleted space".

0bbe26a wallet: filter for keys only before record deser in salvage (Andrew Chow) 544e12a walletdb: Add KeyFilterFn to ReadKeyValue (Andrew Chow) Pull request description: When salvaging a wallet, the only things that matter are the private keys. It is not necessary to attempt to deserialize any other records, especially if those records are corrupted too. This PR adds a `KeyFilterFn` function callback to `ReadKeyValue` that salvage uses to filter for only the records that it wants. Of course doing it this way also lets us do other filters in the future from other places should we so desire. ACKs for top commit: ryanofsky: Code review ACK 0bbe26a. Looks great! This should make the recovery code more robust. Normally it'd be good to have a test case for the problem this fixes, but Marco already wrote one in #19078, so I think we're covered laanwj: Code review ACK 0bbe26a Tree-SHA512: 8e3ee283a22a79273915711c4fb751f3c9b02ce94e6bf08dc468f1cfdf9fac35c693bbfd2435ce43c3a06c601b9b0a67e209621f6814bedfe3bc7a7ccc37bb01

…salvaging 0bbe26a wallet: filter for keys only before record deser in salvage (Andrew Chow) 544e12a walletdb: Add KeyFilterFn to ReadKeyValue (Andrew Chow) Pull request description: When salvaging a wallet, the only things that matter are the private keys. It is not necessary to attempt to deserialize any other records, especially if those records are corrupted too. This PR adds a `KeyFilterFn` function callback to `ReadKeyValue` that salvage uses to filter for only the records that it wants. Of course doing it this way also lets us do other filters in the future from other places should we so desire. ACKs for top commit: ryanofsky: Code review ACK 0bbe26a. Looks great! This should make the recovery code more robust. Normally it'd be good to have a test case for the problem this fixes, but Marco already wrote one in bitcoin#19078, so I think we're covered laanwj: Code review ACK 0bbe26a Tree-SHA512: 8e3ee283a22a79273915711c4fb751f3c9b02ce94e6bf08dc468f1cfdf9fac35c693bbfd2435ce43c3a06c601b9b0a67e209621f6814bedfe3bc7a7ccc37bb01

ryanofsky

Code review ACK fa6154b

Test looks right to me but there is an travis failure on one platform

ryanofsky · 2020-10-21T23:46:27Z

test/functional/tool_wallet.py

+            self.nodes[0].generatetoaddress(1, gna())
+        self.nodes[0].sendmany(amounts={gna(): 0.05 for _ in range(50)})
+        self.nodes[0].sendmany(amounts={gna(): 0.05 for _ in range(50)})
+        balances_before = self.nodes[0].getbalances()
        self.stop_node(0)

        self.assert_tool_output('', '-wallet=salvage', 'salvage')


In commit "test: Add salvage test for wallet tool" (fa6154b)

It seems the test is failing on this line due to stderr output https://travis-ci.org/github/bitcoin/bitcoin/jobs/735969416#L2582:

Salvage: WARNING: Number of keys in data does not match number of values. Salvage: WARNING: Unexpected end of file while reading salvage output. WARNING: WalletBatch::Recover skipping key: CDataStream::read(): end of data: unspecified iostream_category error WARNING: WalletBatch::Recover skipping key: CDataStream::read(): end of data: unspecified iostream_category error WARNING: WalletBatch::Recover skipping key: Error reading wallet database: CPubKey/CPrivKey corrupt

ryanofsky

Code review ACK 66660e4. Only change since last review is rebase. Also travis seems to pass now.

I'm confused about why the test was failing before #19078 (comment) but succeeding now. The bug report #20151 just points back to this PR for steps to reproduce (which I guess no longer work?) and doesn't mention whether the problem happens reliably or is maybe random or platform specific.

maflcko · 2020-11-03T18:40:25Z

Please don't merge this. The failure still happens intermittently.

meshcollider · 2021-09-28T09:33:30Z

@MarcoFalke how often does the intermittent test failure occur? I've rebased this on a branch which incorporates a few other salvage things, which fixes the assert(new_tx) thing again. I haven't been able to hit #20151 yet after multiple re-runs locally.

maflcko · 2021-09-28T10:12:07Z

How often did you try? I am running

while test/functional/tool_wallet.py --legacy-wallet ; do ( echo 1 >> /tmp/runs )  ; done

and it fails after 36 tries:

$ wc -l  /tmp/runs 
36 /tmp/runs

…salvaging 0bbe26a wallet: filter for keys only before record deser in salvage (Andrew Chow) 544e12a walletdb: Add KeyFilterFn to ReadKeyValue (Andrew Chow) Pull request description: When salvaging a wallet, the only things that matter are the private keys. It is not necessary to attempt to deserialize any other records, especially if those records are corrupted too. This PR adds a `KeyFilterFn` function callback to `ReadKeyValue` that salvage uses to filter for only the records that it wants. Of course doing it this way also lets us do other filters in the future from other places should we so desire. ACKs for top commit: ryanofsky: Code review ACK 0bbe26a. Looks great! This should make the recovery code more robust. Normally it'd be good to have a test case for the problem this fixes, but Marco already wrote one in bitcoin#19078, so I think we're covered laanwj: Code review ACK 0bbe26a Tree-SHA512: 8e3ee283a22a79273915711c4fb751f3c9b02ce94e6bf08dc468f1cfdf9fac35c693bbfd2435ce43c3a06c601b9b0a67e209621f6814bedfe3bc7a7ccc37bb01

achow101 · 2022-10-12T18:08:58Z

Are you still working on this?

achow101 · 2022-11-10T17:12:12Z

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

maflcko · 2022-11-11T12:23:18Z

Ok, I'll rebase

maflcko · 2022-11-11T12:41:22Z

The test still fails locally after a sufficient number of iterations, so I guess it is best to remove the command along with the removal of bdb

fanquake added the Tests label May 27, 2020

maflcko mentioned this pull request May 27, 2020

wallet tool salvage always fails with "Assertion `new_tx' failed" #19079

Closed

DrahtBot mentioned this pull request Jun 2, 2020

wallettool: Add dump and createfromdump commands #19137

Merged

maflcko closed this Jun 5, 2020

maflcko reopened this Jun 5, 2020

DrahtBot closed this Jun 5, 2020

DrahtBot reopened this Jun 5, 2020

maflcko force-pushed the 2005-testWalletToolSalvage branch from 166187b to 3650390 Compare June 5, 2020 17:42

maflcko force-pushed the 2005-testWalletToolSalvage branch 2 times, most recently from b53ccb5 to fab4fa1 Compare July 2, 2020 11:39

achow101 mentioned this pull request Jul 22, 2020

wallet: Cleanup wallettool salvage and walletdb extraneous declarations #19457

Merged

achow101 reviewed Jul 22, 2020

View reviewed changes

maflcko force-pushed the 2005-testWalletToolSalvage branch from fab4fa1 to 9670752 Compare August 15, 2020 08:27

ryanofsky mentioned this pull request Aug 24, 2020

bitcoin-wallet salvage: Return false instead of asserting when a loaded tx isn't new #19793

Closed

maflcko force-pushed the 2005-testWalletToolSalvage branch from 9670752 to fa6154b Compare October 15, 2020 08:16

maflcko mentioned this pull request Oct 15, 2020

salvage wallet warns "Number of keys in data does not match number of values" on uncorrupt wallet file #20151

Closed

DrahtBot mentioned this pull request Oct 19, 2020

wallet: Make -wallet setting not create wallets #20186

Merged

ryanofsky approved these changes Oct 21, 2020

View reviewed changes

DrahtBot added the Needs rebase label Oct 29, 2020

maflcko force-pushed the 2005-testWalletToolSalvage branch 2 times, most recently from 96239e8 to 66660e4 Compare October 30, 2020 10:52

DrahtBot removed the Needs rebase label Oct 30, 2020

ryanofsky approved these changes Nov 3, 2020

View reviewed changes

DrahtBot mentioned this pull request Nov 13, 2020

wallettool: add parameter to create descriptors wallet #20365

Merged

DrahtBot added the Needs rebase label Dec 16, 2020

maflcko force-pushed the 2005-testWalletToolSalvage branch from 66660e4 to 8d600d6 Compare September 28, 2021 10:10

DrahtBot removed the Needs rebase label Sep 28, 2021

DrahtBot mentioned this pull request Oct 2, 2022

Bump minimum python version to 3.7 #26226

Merged

achow101 closed this Nov 10, 2022

maflcko changed the title ~~test: Add salvage test for wallet tool~~ test: Extend salvage test for wallet tool Nov 11, 2022

test: Extend salvage test for wallet tool

fa60b70

maflcko reopened this Nov 11, 2022

maflcko force-pushed the 2005-testWalletToolSalvage branch from 8d600d6 to fa60b70 Compare November 11, 2022 12:23

maflcko closed this Nov 11, 2022

maflcko deleted the 2005-testWalletToolSalvage branch November 11, 2022 13:56

bitcoin locked and limited conversation to collaborators Nov 11, 2023

test: Extend salvage test for wallet tool #19078

test: Extend salvage test for wallet tool #19078

Uh oh!

Conversation

maflcko commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented May 28, 2020

Uh oh!

DrahtBot commented Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonatack commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 commented Aug 25, 2020

Uh oh!

ryanofsky commented Aug 25, 2020

Uh oh!

achow101 commented Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 3, 2020

Uh oh!

meshcollider commented Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Sep 28, 2021

Uh oh!

achow101 commented Oct 12, 2022

Uh oh!

achow101 commented Nov 10, 2022

Uh oh!

maflcko commented Nov 11, 2022

Uh oh!

maflcko commented Nov 11, 2022

Uh oh!

Uh oh!

maflcko commented May 27, 2020 •

edited

Loading

DrahtBot commented Jun 2, 2020 •

edited

Loading

jonatack commented Jun 5, 2020 •

edited

Loading

achow101 commented Aug 25, 2020 •

edited

Loading

meshcollider commented Sep 28, 2021 •

edited

Loading