Skip to content

wallet: Tidy CWallet::SetUsedDestinationState #17354

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 8, 2019
Merged

wallet: Tidy CWallet::SetUsedDestinationState #17354

merged 2 commits into from
Nov 8, 2019

Conversation

promag
Copy link
Contributor

@promag promag commented Nov 2, 2019 •
edited
Loading

This PR makes 2 distinct changes around CWallet::SetUsedDestinationState:

  • 1st the recursive lock is removed and now it requires the lock to be held;
  • 2nd change is to support, in the best case, just a wallet database flush when transaction is added to the wallet.

@fanquake fanquake added the Wallet label Nov 2, 2019
@promag
Copy link
Contributor Author

promag commented Nov 2, 2019

I've noticed that SetUsedDestinationState is not called with used=false so at the moment half of the function is unused code - is someone planning to change this or should we drop the unused code?

@promag
Copy link
Contributor Author

promag commented Nov 2, 2019

Maybe we should add WalletBatch& argument to all wallet "mutation" methods?

@promag promag force-pushed the 2019-11-setuseddestinationstate branch from db7a4c3 to bd0b305 Compare November 2, 2019 18:42
@promag promag force-pushed the 2019-11-setuseddestinationstate branch from bd0b305 to 0b75a7f Compare November 2, 2019 21:36
@promag
Copy link
Contributor Author

promag commented Nov 7, 2019

@achow101 @meshcollider friendly review request.

@ryanofsky
Copy link
Contributor

ryanofsky commented Nov 7, 2019 •
edited
Loading

Maybe we should add WalletBatch& argument to all wallet "mutation" methods?

I'd like this convention since it would make it clearer which methods could hit the database. I'd like it even more if it was extended to methods that read and didn't just mutate the database and was extended to KeyMan and SigningProvider classes so the ugly encrypted_batch hack could go away.
(WalletBatch could be a forward declared type so SigningProvider wouldn't depend on wallet code, or SigningProvider methods could just pass a generic context argument.)

ryanofsky
ryanofsky approved these changes Nov 7, 2019
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK 0b75a7f. Code changes looks fine but PR description should be updated to say what benefits of the change are. I might have missed something, but I didn't see a place where multiple batches were used previously and a single batch was used now. So the main benefit of this change appears to be removing a recursive lock? And maybe moving toward a consistent convention for passing batch instances?

@promag
Copy link
Contributor Author

promag commented Nov 7, 2019 •
edited
Loading

@ryanofsky original goal was to remove the recursive lock. Note that now SetUsedDestinationState uses the existing batch in AddToWallet - resulting in just one flush per wallet transaction (IIUC).

Thanks for the review!

Updated OP.

@ryanofsky
Copy link
Contributor

@ryanofsky original goal was to remove the recursive lock. Note that now SetUsedDestinationState uses the existing batch in AddToWallet - resulting in just one flush per wallet transaction (IIUC).

Good to mention the motivation and brag about improvements like this in the PR description. It helps to understand the change and motivate reviews

@promag
Copy link
Contributor Author

promag commented Nov 7, 2019

And maybe moving toward a consistent convention for passing batch instances?

I understand that you support this, if others also then I can follow up - just add 👍 here.

@achow101
Copy link
Member

achow101 commented Nov 7, 2019

ACK 0b75a7f

1 similar comment
@maflcko
Copy link
Member

maflcko commented Nov 7, 2019

ACK 0b75a7f

fanquake added a commit that referenced this pull request Nov 8, 2019
@fanquake
Merge #17354: wallet: Tidy CWallet::SetUsedDestinationState
4a3b6f4 
0b75a7f wallet: Reuse existing batch in CWallet::SetUsedDestinationState (João Barbosa)
01f45dd wallet: Avoid recursive lock in CWallet::SetUsedDestinationState (João Barbosa)

Pull request description:

  This PR makes 2 distinct changes around `CWallet::SetUsedDestinationState`:
   - 1st the recursive lock is removed and now it requires the lock to be held;
   - 2nd change is to support, in the best case, just a wallet database flush when transaction is added to the wallet.

ACKs for top commit:
  achow101:
    ACK 0b75a7f
  MarcoFalke:
    ACK 0b75a7f
  ryanofsky:
    Code review ACK 0b75a7f. Code changes looks fine but PR description should be updated to say what benefits of the change are. I might have missed something, but I didn't see a place where multiple batches were used previously and a single batch was used now. So the main benefit of this change appears to be removing a recursive lock? And maybe moving toward a consistent convention for passing batch instances?

Tree-SHA512: abcf23a5850d29990668db20d6f624cca3e89629cc9ed003e0d05cde1b58ab2ff365034f156684ad13e55764b54c6c0c2bc7d5f96b8af7dc5e45a3be955d6b15
@fanquake fanquake merged commit 0b75a7f into bitcoin:master Nov 8, 2019
@promag promag deleted the 2019-11-setuseddestinationstate branch November 8, 2019 13:56
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 9, 2019
@fanquake
Merge bitcoin#17354: wallet: Tidy CWallet::SetUsedDestinationState
30bd7ef 
0b75a7f wallet: Reuse existing batch in CWallet::SetUsedDestinationState (João Barbosa)
01f45dd wallet: Avoid recursive lock in CWallet::SetUsedDestinationState (João Barbosa)

Pull request description:

  This PR makes 2 distinct changes around `CWallet::SetUsedDestinationState`:
   - 1st the recursive lock is removed and now it requires the lock to be held;
   - 2nd change is to support, in the best case, just a wallet database flush when transaction is added to the wallet.

ACKs for top commit:
  achow101:
    ACK 0b75a7f
  MarcoFalke:
    ACK 0b75a7f
  ryanofsky:
    Code review ACK 0b75a7f. Code changes looks fine but PR description should be updated to say what benefits of the change are. I might have missed something, but I didn't see a place where multiple batches were used previously and a single batch was used now. So the main benefit of this change appears to be removing a recursive lock? And maybe moving toward a consistent convention for passing batch instances?

Tree-SHA512: abcf23a5850d29990668db20d6f624cca3e89629cc9ed003e0d05cde1b58ab2ff365034f156684ad13e55764b54c6c0c2bc7d5f96b8af7dc5e45a3be955d6b15
@kallewoof kallewoof mentioned this pull request Feb 9, 2020
Merged
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 22, 2020
@promag @deadalnix
wallet: Tidy CWallet::SetUsedDestinationState
0b16c6b 
Summary:
 * wallet: Avoid recursive lock in CWallet::SetUsedDestinationState

 * wallet: Reuse existing batch in CWallet::SetUsedDestinationState

This is a backport of Core [[bitcoin/bitcoin#17354 | PR17354]]

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D7529
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
@fanquake
Merge bitcoin#17354: wallet: Tidy CWallet::SetUsedDestinationState
5647cc9 
0b75a7f wallet: Reuse existing batch in CWallet::SetUsedDestinationState (João Barbosa)
01f45dd wallet: Avoid recursive lock in CWallet::SetUsedDestinationState (João Barbosa)

Pull request description:

  This PR makes 2 distinct changes around `CWallet::SetUsedDestinationState`:
   - 1st the recursive lock is removed and now it requires the lock to be held;
   - 2nd change is to support, in the best case, just a wallet database flush when transaction is added to the wallet.

ACKs for top commit:
  achow101:
    ACK 0b75a7f
  MarcoFalke:
    ACK 0b75a7f
  ryanofsky:
    Code review ACK 0b75a7f. Code changes looks fine but PR description should be updated to say what benefits of the change are. I might have missed something, but I didn't see a place where multiple batches were used previously and a single batch was used now. So the main benefit of this change appears to be removing a recursive lock? And maybe moving toward a consistent convention for passing batch instances?

Tree-SHA512: abcf23a5850d29990668db20d6f624cca3e89629cc9ed003e0d05cde1b58ab2ff365034f156684ad13e55764b54c6c0c2bc7d5f96b8af7dc5e45a3be955d6b15
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@promag @ryanofsky @achow101 @maflcko @fanquake