wallet: Added function to remove transactions that are no longer in wallet #15130

pull benthecarman wants to merge 1 commits into bitcoin:master from benthecarman:wallet_remove_old_txs changing 2 files +21 −1

benthecarman commented at 4:30 PM on January 8, 2019: contributor

When rescanning a wallet for transactions, it will remove transactions that are no longer in the wallet.

Required for #15129
fanquake added the label Wallet on Jan 8, 2019
wallet: Added function to remove transactions that are no longer in wallet f41906cd01
benthecarman force-pushed on Jan 8, 2019

in src/wallet/wallet.cpp:880 in f41906cd01

 874 | @@ -875,6 +875,22 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
 875 |      return true;
 876 |  }
 877 |  
 878 | +bool CWallet::RemoveFromWallet(const CTransaction& tx)
 879 | +{
 880 | +    LOCK(cs_wallet);

promag commented at 12:01 AM on January 9, 2019:

Prefer AssertLockHeld?

in src/wallet/wallet.cpp:969 in f41906cd01

 962 | @@ -947,7 +963,10 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CBlockI
 963 |              if (pIndex != nullptr)
 964 |                  wtx.SetMerkleBranch(pIndex, posInBlock);
 965 |  
 966 | -            return AddToWallet(wtx, false);
 967 | +            if (IsMine(tx) || IsFromMe(tx))
 968 | +                return AddToWallet(wtx, false);
 969 | +
 970 | +            return RemoveFromWallet(tx);

promag commented at 12:01 AM on January 9, 2019:

Does it make sense to return the result of RemoveFromWallet in AddToWalletIfInvolvingMe?

in src/wallet/wallet.cpp:889 in f41906cd01

 884 | +    }
 885 | +
 886 | +    WalletLogPrintf("RemoveFromWallet %s\n", tx.GetHash().ToString());
 887 | +
 888 | +    // Notify UI of removed transaction
 889 | +    NotifyTransactionChanged(this, tx.GetHash(), CT_DELETED);

promag commented at 12:03 AM on January 9, 2019:

Unused notification?

promag commented at 12:08 AM on January 9, 2019: member

This looks very inefficient. RemoveWallet is called for all non-wallet transactions. The lack of tests doesn't support this change.
maflcko commented at 2:07 PM on January 9, 2019: member

Required for #15129

This might be required for another pull, but makes no sense on its own IIUC
promag commented at 2:11 PM on January 9, 2019: member

Yes, when I suggested to split I meant to make this one rebased on that.
benthecarman closed this on Jan 9, 2019
bitcoin locked this on Dec 16, 2021
benthecarman deleted the branch on Feb 17, 2024

Contributors

Labels

Linked (view graph)

#15129 rpc: Added ability to remove watch only addresses