Remove CWalletTx merging logic from AddToWallet by ryanofsky · Pull Request #9381 · bitcoin/bitcoin

ryanofsky · 2016-12-19T23:00:20Z

This is a pure refactoring, no behavior is changing.

Instead of AddToWallet taking a temporary CWalletTx object and then potentially merging it with a pre-existing CWalletTx, have it take a callback so callers can update the pre-existing CWalletTx directly.

This makes AddToWallet simpler because now it is only has to be concerned with saving CWalletTx objects and not merging them.

This makes AddToWallet calls clearer because they can now make direct updates to CWalletTx entries without having to make temporary objects and then worry about how they will be merged.

Motivation for this change came from the bumpfee PR #8456 where we wanted to be able to call AddToWallet to make a simple update to an existing transaction, but were reluctant to, because the existing CWalletTx merging logic did not apply and seemed dangerous try to update as part of that PR. After this refactoring, the bumpfee PR could call AddToWallet safely instead of implementing a duplicate AddToWallet function.

This also allows getting rid of the CWalletTx copy constructor to prevent unintentional copying.

ryanofsky · 2017-01-25T18:30:33Z

Rebased ed457a3 -> b1ac3cf to resolve merge conflicts with bumpfee (#8456).

ryanofsky · 2017-01-25T18:48:42Z

I'm thinking of splitting this up into two commits to make it easier to review. First commit would change CreateTransaction to return a CTransactionRef instead of CWalletTx. Second commit would change AddToWallet to accept a CTransactionRef instead of a CWalletTx.

If any reviewers would prefer this you can let me know.

TheBlueMatt · 2017-01-25T19:26:39Z

Splitting commits into logical breaks is always appreciated.

…

On 01/25/17 18:48, Russell Yanofsky wrote: I'm thinking of splitting this up into two commits to make it easier to review. First commit would change CreateTransaction to return a CTransactionRef instead of CWalletTx. Second commit would change AddToWallet to accept a CTransactionRef instead of a CWalletTx. If any reviewers would prefer this you can let me know. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#9381 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnoHu0xZqwtESQp3_rXWoR3OurDnNzgks5rV5kPgaJpZM4LRR4W>.

TheBlueMatt · 2017-01-25T22:17:34Z

Hum, reading this (finally) now, I'm not sure if I'm a big fan of this approach. The call-function-which-callsback-to-let-you-fill-in-things-that-really-should-have-been-arguments pattern really sucks. On the flip side, passing in CWalletTxes, and copying those into mapWallet can also lead to issues if you try to do anything to the object you just passed in thinking it is what got stored in the wallet (a mistake I made recently).

Maybe we should adapt some of these functions (AddToWallet/CommitTransaction/whatever) to just always take an rvalue to a CWalletTx. This leaves only AddToWalletIfInvolvingMe (I think) which has to update-or-add, and that can just use an internal version of AddToWallet which takes the CWalletDB as an argument instead of creating its own.

Preserves comment strings, order form and account strings from the original wallet transaction. Also sets fTimeReceivedIsTxTime and fFromMe fields (which are set in CWallet::CreateTransaction) which don't actually influence wallet behavior, but record that the transaction originated in the wallet (instead of coming from the network or sendrawtransaction). Setting these fields also simplifies CWalletTx cleanup in bitcoin#9381.

ryanofsky · 2017-02-02T23:51:36Z

The call-function-which-callsback-to-let-you-fill-in-things-that-really-should-have-been-arguments pattern really sucks.

If you are adamant about this, could you say more about why it sucks? I think a callback is exactly the thing you want when you are doing an in-place update to a data structure.

In any case, I agree that CommitTransaction shouldn't take a callback, so I changed it to just take a transaction ref. So now there are way fewer callbacks (I think only 3 left in non-test code).

TheBlueMatt · 2017-02-03T18:46:10Z

I'm just generally not a fan of callbacks making ownership models inconsistent. eg this kind of thing is where people always fuck up lockordering (though admittedly less so in this particular case, more the general case of callbacks going in both directions between modules).

So I do like this version much better, but still not sure the call back is required to be publicly exposed...Can we just leave MarkReplaced and add a similar function for importpruned funds to use (I dont believe the double-disk-sync in importprunedfunds will make for inconsistent/bad on-disk state?). Then we can make AddToWallet(..., callback) private and use AddToWalletIfInvolvingMe (which should probably also take a CTransactionRef, not a CTransaction, though maybe thats for a separate PR) publicly.

ryanofsky · 2017-02-03T19:10:56Z

Can we just leave MarkReplaced and add a similar function for importpruned funds to use

Will try that. In the meantime I moved the non-AddToWallet cleanup to #9680 so it can be considered separately. By the way #9369 is another PR which significantly simplifies AddToWallet.

ryanofsky · 2020-02-25T17:40:14Z

Rebased dff3c12 -> 93510bc (pr/atw-nomerge.74 -> pr/atw-nomerge.75, compare) due to minor conflict with #13339

Sjors

ACK 93510bc

Sjors · 2020-02-26T09:23:30Z

src/wallet/wallet.cpp

 }

-bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
+bool CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmation& confirm, const UpdateWalletTxFn& update_wtx, bool fFlushOnClose)


In 03c5ac0 Remove CWalletTx merging logic from AddToWallet: it might make sense to have a seperate commit that adds the confirm argument.
(maybe not worth confusing existing reviewers)

re: #9381 (comment)

In 03c5ac0 Remove CWalletTx merging logic from AddToWallet: it might make sense to have a seperate commit that adds the confirm argument.
(maybe not worth confusing existing reviewers)

This is a good thought, and I started to implement it, but the problem is if I just make a commit adding the confirm argument, then there are two confirm objects being passed to AddToWallet (the other one embedded in the CWalletTx object), which is confusing because it's unclear which confirmation should take precedence. If I change the CWalletTx argument to a TransactionRef to solve this problem, then there is no way for callers to set to the other CWalletTx fields (mapValue, vOrderForm, etc), and I have to add the update_wtx argument too, which is already the whole current commit

Sjors · 2020-02-26T09:37:39Z

src/wallet/wallet.h

+    //! Callback for updating transaction metadata in mapWallet.
+    //!
+    //! @param wtx - reference to mapWallet transaction to update
+    //! @param new_tx - true if wtx is newly inserted, false it it previously existed


Nit: "false if it"

re: #9381 (comment)

Nit: "false if it"

Thank you, fixed!

Sjors · 2020-02-26T09:54:32Z

src/wallet/wallet.cpp

 {
    auto locked_chain = chain().lock();
    LOCK(cs_wallet);
+    WalletLogPrintf("CommitTransaction:\n%s", tx->ToString()); /* Continued */


Why the /* Continued */ comment?

re: #9381 (comment)

Why the /* Continued */ comment?

This is unchanged from previous code, but it's needed to print a lint error since the log format string doesn't end in \n

Sjors · 2020-02-26T10:06:34Z

src/wallet/wallet.cpp

    // otherwise just for transaction history.
-    AddToWallet(wtxNew);
+    AddToWallet(std::move(tx), {}, [&](CWalletTx& new_wtx, bool new_tx) {
+        assert(new_tx);


Make this a runtime exception? Perhaps there's some weird edge case, I haven't tried, where a user restores their wallet from a backup on a node with a fresh mempool. The user then recreates the same* transaction in the UI and as they look at the confirmation screen, the transaction re-enters the mempool. Once they click OK this assert should be hit.

* = IIUC it's only a duplicate if they use the same inputs, outputs, fee, if we shuffle them the same way and if the block height hasn't changed

re: #9381 (comment)

Make this a runtime exception? Perhaps there's some weird edge case, I haven't tried, where a user restores their wallet from a backup on a node with a fresh mempool. The user then recreates the same* transaction in the UI and as they look at the confirmation screen, the transaction re-enters the mempool. Once they click OK this assert should be hit.

Good point. Since there can be an arbitrary delay before the GUI commits the transaction, it isn't safe to assert assert the transaction is new. I removed the assert and just replaced it with nonfatal checks to make sure there aren't conflicting mapValue and order form values.

Sjors · 2020-02-26T10:57:18Z

src/wallet/walletdb.cpp

+                return true;
+            };
+            if (!pwallet->LoadToWallet(hash, update_wtx)) {
+                return false;


In 754eb21 Avoid copying CWalletTx in LoadToWallet

The "result" of (formerly void) LoadToWallet has always been ignored. Perhaps that's because before https://github.com/bitcoin/bitcoin/pull/3671/files#diff-3b5a9b7d780ff672241548edf2888fcdL385 we just updated the transaction map directly.

Can you think of any silent failures that would now result in ReadKeyValue failing, and IIUC trigger a rescan? (cc @achow101)

bitcoin/src/wallet/walletdb.cpp

Lines 469 to 483 in 2d6e76a

if (!ReadKeyValue(pwallet, ssKey, ssValue, wss, strType, strErr))

{

// losing keys is considered a catastrophic error, anything else

// we assume the user can live with:

if (IsKeyType(strType) || strType == DBKeys::DEFAULTKEY) {

result = DBErrors::CORRUPT;

} else if (strType == DBKeys::FLAGS) {

// reading the wallet flags can only fail if unknown flags are present

result = DBErrors::TOO_NEW;

} else {

// Leave other errors alone, if we try to fix them we might make things worse.

fNoncriticalErrors = true; // ... but do warn the user there is something wrong.

if (strType == DBKeys::TX)

// Rescan if there is a bad transaction record:

gArgs.SoftSetBoolArg("-rescan", true);

re: #9381 (comment)

In 754eb21 Avoid copying CWalletTx in LoadToWallet

The "result" of (formerly void) LoadToWallet has always been ignored. Perhaps that's because before https://github.com/bitcoin/bitcoin/pull/3671/files#diff-3b5a9b7d780ff672241548edf2888fcdL385 we just updated the transaction map directly.

Can you think of any silent failures that would now result in ReadKeyValue failing, and IIUC trigger a rescan? (cc @achow101)

It might be worth thinking about how to improve error handling in this code, but behavior shouldn't be changing here. LoadToWallet will only return false if fill_wtx returns false, and fill_wtx only returns false if wtx.GetHash() != hash. So ReadKeyValue should be returning true all the times it used to return true and false and the times it used to return false

src/wallet/walletdb.cpp

src/wallet/wallet.cpp

src/wallet/walletdb.cpp

ryanofsky

Thanks for reviewing!

Updated 93510bc -> fc4df9d (pr/atw-nomerge.75 -> pr/atw-nomerge.76, compare) with suggestions and a few tweaks to make diffs a little smaller

ryanofsky · 2020-02-26T14:43:57Z

src/wallet/wallet.cpp

 }

-bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
+bool CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmation& confirm, const UpdateWalletTxFn& update_wtx, bool fFlushOnClose)


re: #9381 (comment)

In 03c5ac0 Remove CWalletTx merging logic from AddToWallet: it might make sense to have a seperate commit that adds the confirm argument.
(maybe not worth confusing existing reviewers)

This is a good thought, and I started to implement it, but the problem is if I just make a commit adding the confirm argument, then there are two confirm objects being passed to AddToWallet (the other one embedded in the CWalletTx object), which is confusing because it's unclear which confirmation should take precedence. If I change the CWalletTx argument to a TransactionRef to solve this problem, then there is no way for callers to set to the other CWalletTx fields (mapValue, vOrderForm, etc), and I have to add the update_wtx argument too, which is already the whole current commit

ryanofsky · 2020-02-26T14:44:04Z

src/wallet/wallet.cpp

 {
    auto locked_chain = chain().lock();
    LOCK(cs_wallet);
+    WalletLogPrintf("CommitTransaction:\n%s", tx->ToString()); /* Continued */


re: #9381 (comment)

Why the /* Continued */ comment?

This is unchanged from previous code, but it's needed to print a lint error since the log format string doesn't end in \n

ryanofsky · 2020-02-26T14:44:14Z

src/wallet/wallet.cpp

    // otherwise just for transaction history.
-    AddToWallet(wtxNew);
+    AddToWallet(std::move(tx), {}, [&](CWalletTx& new_wtx, bool new_tx) {
+        assert(new_tx);


re: #9381 (comment)

Make this a runtime exception? Perhaps there's some weird edge case, I haven't tried, where a user restores their wallet from a backup on a node with a fresh mempool. The user then recreates the same* transaction in the UI and as they look at the confirmation screen, the transaction re-enters the mempool. Once they click OK this assert should be hit.

Good point. Since there can be an arbitrary delay before the GUI commits the transaction, it isn't safe to assert assert the transaction is new. I removed the assert and just replaced it with nonfatal checks to make sure there aren't conflicting mapValue and order form values.

ryanofsky · 2020-02-26T14:44:21Z

src/wallet/wallet.h

+    //! Callback for updating transaction metadata in mapWallet.
+    //!
+    //! @param wtx - reference to mapWallet transaction to update
+    //! @param new_tx - true if wtx is newly inserted, false it it previously existed


re: #9381 (comment)

Nit: "false if it"

Thank you, fixed!

src/wallet/wallet.cpp

src/wallet/walletdb.cpp

ryanofsky · 2020-02-26T14:44:58Z

src/wallet/walletdb.cpp

+                return true;
+            };
+            if (!pwallet->LoadToWallet(hash, update_wtx)) {
+                return false;


re: #9381 (comment)

In 754eb21 Avoid copying CWalletTx in LoadToWallet

The "result" of (formerly void) LoadToWallet has always been ignored. Perhaps that's because before https://github.com/bitcoin/bitcoin/pull/3671/files#diff-3b5a9b7d780ff672241548edf2888fcdL385 we just updated the transaction map directly.

Can you think of any silent failures that would now result in ReadKeyValue failing, and IIUC trigger a rescan? (cc @achow101)

It might be worth thinking about how to improve error handling in this code, but behavior shouldn't be changing here. LoadToWallet will only return false if fill_wtx returns false, and fill_wtx only returns false if wtx.GetHash() != hash. So ReadKeyValue should be returning true all the times it used to return true and false and the times it used to return false

ryanofsky · 2020-03-24T20:34:06Z

Rebased fc4df9d -> f1fc789 (pr/atw-nomerge.76 -> pr/atw-nomerge.77, compare) due to conflict with #18278

Sjors · 2020-03-31T15:16:44Z

utACK f1fc789

ryanofsky · 2020-04-15T21:41:54Z

Rebased f1fc789 -> ece5d1c (pr/atw-nomerge.77 -> pr/atw-nomerge.78, compare) due to conflict with #17954

Sjors · 2020-04-19T14:17:23Z

re-utACK ece5d1c (rebased)

Instead of AddToWallet taking a temporary CWalletTx object and then potentially merging it with a pre-existing CWalletTx, have it take a callback so callers can update the pre-existing CWalletTx directly. This makes AddToWallet simpler because now it is only has to be concerned with saving CWalletTx objects and not merging them. This makes AddToWallet calls clearer because they can now make direct updates to CWalletTx entries without having to make temporary objects and then worry about how they will be merged. This is a pure refactoring, no behavior is changing.

The change in walletdb.cpp is easier to review ignoring whitespace. This change is need to get rid of CWalletTx copy constructor.

Disable copying of CWalletTx objects to prevent bugs where instances get copied in and out of the mapWallet map and fields are updated in the wrong copy.

CWalletTx initialization has been fixed so it's no longer necessary to change which wallet a transaction is bound to.

ryanofsky · 2020-05-01T15:00:34Z

Rebased ece5d1c -> f2892d8 (pr/atw-nomerge.78 -> pr/atw-nomerge.79, compare) due to conflict with #16426

Sjors · 2020-05-04T10:21:31Z

Light re-utACK f2892d8, looks like a reasonably straight-forward rebase.

meshcollider

utACK f2892d8

maflcko

ACK f2892d8

Just some questions

maflcko · 2020-05-05T15:12:48Z

src/wallet/wallet.cpp

+    CWalletTx* wtx = AddToWallet(std::move(tx), {}, [&](CWalletTx& wtx, bool new_tx) {
+        CHECK_NONFATAL(wtx.mapValue.empty());
+        CHECK_NONFATAL(wtx.vOrderForm.empty());
+        wtx.mapValue = std::move(mapValue);


Does the std::move still work after going through a lambda capture by reference?

Yes. mapValue is a reference to the same thing it was outside of the lambda. std::move casts it to an rvalue reference.

Oh, this has nothing to do with lambdas. I didn't know that references (as long as they are not const) can be moved.

re: #9381 (comment)

Oh, this has nothing to do with lambdas. I didn't know that references (as long as they are not const) can be moved.

Yep, std::move isn't affected by outside things like this. std::move(mapValue) is only a type cast from mapValue_t& to mapValue_t&& that makes the compiler favor the operator=(mapValue_t&&) assignment overload instead of the operator=(const mapValue_t&) one for wtx.mapValue =

Even const ones, actually - though unless they have mutable fields, there is no effect.

With "no effect" you mean const& will stay const&, right? See also the example, which does not compile, because the const& copy constructor is deleted:

#include <memory> struct Test{ const std::unique_ptr<int>b; Test(const std::unique_ptr<int>& a):b{std::move(a)}{} };

std::move(a) where a is const T&, returns something of type const T&&. If T has for example a operator=(const T&&), then that one would be selected in assignment. If there is no such operator, operator=(const T&) will be selected instead. In general a const T&& assignment/constructor operator only makes sense if a class has mutable fields.

TIL that const T&& exists.

maflcko · 2020-05-05T15:13:25Z

src/wallet/wallet.cpp


    // Notify that old coins are spent
-    for (const CTxIn& txin : wtxNew.tx->vin) {
+    for (const CTxIn& txin : wtx->tx->vin) {


Doesn't this line crash the node when the wallet can not write the tx to disk?

re: #9381 (comment)

Doesn't this line crash the node when the wallet can not write the tx to disk?

Wow, good catch! I'm not sure current behavior is ideal either but it wasn't my intention to change it. Updated PR

ryanofsky

Updated f2892d8 -> 28b112e (pr/atw-nomerge.79 -> pr/atw-nomerge.80, compare) reverting some CommitTransaction changes to avoid changing behavior and crashing on failure

ryanofsky · 2020-05-05T16:30:13Z

src/wallet/wallet.cpp


    // Notify that old coins are spent
-    for (const CTxIn& txin : wtxNew.tx->vin) {
+    for (const CTxIn& txin : wtx->tx->vin) {


re: #9381 (comment)

Doesn't this line crash the node when the wallet can not write the tx to disk?

Wow, good catch! I'm not sure current behavior is ideal either but it wasn't my intention to change it. Updated PR

ryanofsky · 2020-05-05T16:39:28Z

src/wallet/wallet.cpp

+    CWalletTx* wtx = AddToWallet(std::move(tx), {}, [&](CWalletTx& wtx, bool new_tx) {
+        CHECK_NONFATAL(wtx.mapValue.empty());
+        CHECK_NONFATAL(wtx.vOrderForm.empty());
+        wtx.mapValue = std::move(mapValue);


re: #9381 (comment)

Oh, this has nothing to do with lambdas. I didn't know that references (as long as they are not const) can be moved.

Yep, std::move isn't affected by outside things like this. std::move(mapValue) is only a type cast from mapValue_t& to mapValue_t&& that makes the compiler favor the operator=(mapValue_t&&) assignment overload instead of the operator=(const mapValue_t&) one for wtx.mapValue =

maflcko · 2020-05-05T17:29:37Z

Ah sorry, no need to revert your changes. If the wallet can't write to disk, a simple CHECK_NONFATAL(wtx) should be sufficient

ryanofsky · 2020-05-05T17:54:24Z

Ah sorry, no need to revert your changes. If the wallet can't write to disk, a simple CHECK_NONFATAL(wtx) should be sufficient

I don't think CHECK_NONFATAL would be right, because this is checking a runtime error, not an internal assertion about how code works. I can go back to the earlier version and add this if that's your preference. But actually I like the update better because it makes the overall diff smaller and avoids changing behavior in a refactoring PR. I think ideally if different error handling is needed here, it would be handled in a standalone PR with a unit test.

maflcko · 2020-05-05T17:58:36Z

Well, our assumption in the code is that the wallet can write to disk. If that assumption is violated it seems fine to assert or do that CHECK thingy. But I also see your point to make it a separate runtime error.

Anyway, re-ACK 28b112e

meshcollider · 2020-05-05T23:37:03Z

re-utACK 28b112e

fanquake added Refactoring Wallet labels Dec 19, 2016

ryanofsky force-pushed the pr/atw-nomerge branch from d988ec9 to ed457a3 Compare January 6, 2017 17:08

ryanofsky mentioned this pull request Jan 13, 2017

[RPC] Simplified bumpfee command. #8456

Merged

ryanofsky force-pushed the pr/atw-nomerge branch from ed457a3 to b1ac3cf Compare January 25, 2017 18:29

ryanofsky force-pushed the pr/atw-nomerge branch from b1ac3cf to b78f2eb Compare February 2, 2017 23:23

ryanofsky mentioned this pull request Feb 2, 2017

Set correct metadata on bumpfee wallet transactions #9673

Merged

ryanofsky force-pushed the pr/atw-nomerge branch from b78f2eb to d45e95e Compare February 2, 2017 23:34

ryanofsky changed the title ~~Remove CWalletTx merging logic from AddToWallet~~ Remove CWalletTx merging logic from AddToWallet (on top of #9673) Feb 2, 2017

ryanofsky force-pushed the pr/atw-nomerge branch from d45e95e to 31ec1e6 Compare February 3, 2017 16:26

ryanofsky mentioned this pull request Feb 3, 2017

Unify CWalletTx construction #9680

Merged

ryanofsky changed the title ~~Remove CWalletTx merging logic from AddToWallet (on top of #9673)~~ Remove CWalletTx merging logic from AddToWallet (on top of #9680) Feb 3, 2017

ryanofsky force-pushed the pr/atw-nomerge branch from 31ec1e6 to 98cf53c Compare February 6, 2017 17:47

ryanofsky changed the title ~~Remove CWalletTx merging logic from AddToWallet (on top of #9680)~~ WIP: Remove CWalletTx merging logic from AddToWallet (on top of #9680) Feb 6, 2017

ryanofsky force-pushed the pr/atw-nomerge branch from 98cf53c to 3b022f6 Compare March 3, 2017 19:38

ryanofsky force-pushed the pr/atw-nomerge branch from 3b022f6 to 2764551 Compare June 2, 2017 16:05

ryanofsky force-pushed the pr/atw-nomerge branch from 2764551 to c80a2bf Compare June 19, 2017 19:24

ryanofsky force-pushed the pr/atw-nomerge branch from c80a2bf to 90847c4 Compare July 18, 2017 18:50

ryanofsky force-pushed the pr/atw-nomerge branch 2 times, most recently from 777c297 to 4a20b3c Compare August 16, 2017 17:36

ryanofsky force-pushed the pr/atw-nomerge branch from 4a20b3c to 4e8ce01 Compare August 23, 2017 19:46

Sjors approved these changes Feb 26, 2020

View reviewed changes

ryanofsky commented Feb 26, 2020

View reviewed changes

DrahtBot mentioned this pull request Mar 6, 2020

interfaces: Describe and follow some code conventions #18278

Merged

DrahtBot mentioned this pull request Mar 25, 2020

Use shared pointers only in validation interface #18354

Closed

DrahtBot mentioned this pull request Apr 1, 2020

Wallet passive startup #15719

Closed

DrahtBot mentioned this pull request Apr 11, 2020

rpc: replace raw pointers with shared_ptrs #18592

Closed

DrahtBot mentioned this pull request Apr 17, 2020

Reverse cs_main, cs_wallet lock order and reduce cs_main locking #16426

Merged

ryanofsky added 5 commits May 1, 2020 05:59

Get rid of unneeded CWalletTx::Init parameter

bd2fbc7

Avoid copying CWalletTx in LoadToWallet

65b9d8f

The change in walletdb.cpp is easier to review ignoring whitespace. This change is need to get rid of CWalletTx copy constructor.

Disable CWalletTx copy constructor

d002f9d

Disable copying of CWalletTx objects to prevent bugs where instances get copied in and out of the mapWallet map and fields are updated in the wrong copy.

Get rid of BindWallet

28b112e

CWalletTx initialization has been fixed so it's no longer necessary to change which wallet a transaction is bound to.

meshcollider reviewed May 5, 2020

View reviewed changes

maflcko approved these changes May 5, 2020

View reviewed changes

ryanofsky commented May 5, 2020

View reviewed changes

	if (!ReadKeyValue(pwallet, ssKey, ssValue, wss, strType, strErr))
	{
	// losing keys is considered a catastrophic error, anything else
	// we assume the user can live with:
	if (IsKeyType(strType) \|\| strType == DBKeys::DEFAULTKEY) {
	result = DBErrors::CORRUPT;
	} else if (strType == DBKeys::FLAGS) {
	// reading the wallet flags can only fail if unknown flags are present
	result = DBErrors::TOO_NEW;
	} else {
	// Leave other errors alone, if we try to fix them we might make things worse.
	fNoncriticalErrors = true; // ... but do warn the user there is something wrong.
	if (strType == DBKeys::TX)
	// Rescan if there is a bad transaction record:
	gArgs.SoftSetBoolArg("-rescan", true);

Conversation

ryanofsky commented Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Jan 25, 2017

Uh oh!

ryanofsky commented Jan 25, 2017

Uh oh!

TheBlueMatt commented Jan 25, 2017 via email

Uh oh!

TheBlueMatt commented Jan 25, 2017

Uh oh!

ryanofsky commented Feb 2, 2017

Uh oh!

TheBlueMatt commented Feb 3, 2017

Uh oh!

ryanofsky commented Feb 3, 2017

Uh oh!

ryanofsky commented Feb 25, 2020

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Feb 26, 2020 • edited by ryanofsky Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Mar 24, 2020

Uh oh!

Sjors commented Mar 31, 2020

Uh oh!

ryanofsky commented Apr 15, 2020

Uh oh!

Sjors commented Apr 19, 2020

Uh oh!

ryanofsky commented May 1, 2020

Uh oh!

Sjors commented May 4, 2020

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

ryanofsky commented Dec 19, 2016 •

edited

Loading

Sjors Feb 26, 2020 •

edited by ryanofsky

Loading

sipa May 5, 2020 •

edited

Loading

maflcko May 5, 2020 •

edited

Loading