Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

benthecarman
Copy link
Contributor

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

Required for #15129

Copy link
Member

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very inefficient. RemoveWallet is called for all non-wallet transactions. The lack of tests doesn't support this change.

@@ -875,6 +875,22 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
return true;
}

bool CWallet::RemoveFromWallet(const CTransaction& tx)
{
LOCK(cs_wallet);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer AssertLockHeld?

if (IsMine(tx) || IsFromMe(tx))
return AddToWallet(wtx, false);

return RemoveFromWallet(tx);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

WalletLogPrintf("RemoveFromWallet %s\n", tx.GetHash().ToString());

// Notify UI of removed transaction
NotifyTransactionChanged(this, tx.GetHash(), CT_DELETED);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused notification?

@maflcko
Copy link
Member

maflcko commented Jan 9, 2019

Required for #15129

This might be required for another pull, but makes no sense on its own IIUC

@promag
Copy link
Member

promag commented Jan 9, 2019

Yes, when I suggested to split I meant to make this one rebased on that.

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
@benthecarman benthecarman deleted the wallet_remove_old_txs branch February 17, 2024 14:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants