Skip to content

Conversation

@furszy
Copy link
Member

@furszy furszy commented Jul 5, 2022

Another wallet's code garbage collector work. Part of the mapWallet encapsulation goal.

Focused on the following points:

  1. Remove always true fUseCache argument from CachedTxGetImmatureCredit, CachedTxGetImmatureWatchOnly and CachedTxGetAvailableCredit.
  2. Remove always false recalculate argument from GetCachableAmount.
  3. Merge CachedTxGetImmatureCredit and CachedTxGetImmatureWatchOnlyCredit as they do share the exact same code.
  4. Clean InputIsMine method; use GetWalletTx instead of access the wallet's map directly.
  5. Clean AllInputsMine method; use InputIsMine instead of duplicate the exact same code internally.

@fanquake fanquake added the Wallet label Jul 5, 2022
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 5, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24699 (wallet: Improve AvailableCoins performance by reducing duplicated operations by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
BOOST_CHECK_EQUAL(CachedTxGetImmatureCredit(wallet, wtx, ISMINE_SPENDABLE), 50*COIN);
BOOST_CHECK_EQUAL(CachedTxGetImmatureCredit(wallet, wtx, ISMINE_SPENDABLE), 50 * COIN);

Copy link
Contributor

@aureleoules aureleoules left a comment

Choose a reason for hiding this comment

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

ACK ba2fa8302dc3816d5b9584907135f91789846d65.
Looks better!
I checked that the refactor does not change the behavior of the code.

Copy link
Contributor

@theStack theStack 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 ba2fa8302dc3816d5b9584907135f91789846d65

Nice refactoring!

Instead of duplicate the exact same code twice.
@furszy furszy force-pushed the 2022_wallet_cleanup_1 branch from ba2fa83 to 47ea70f Compare July 6, 2022 21:04
@furszy
Copy link
Member Author

furszy commented Jul 6, 2022

per feedback, have inlined AllInputsMine return statement.

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

re-ACK 47ea70f

@furszy
Copy link
Member Author

furszy commented Jul 20, 2022

hey @jonatack @aureleoules, friendly ping here.

@aureleoules
Copy link
Contributor

re-ACK 47ea70f

@achow101
Copy link
Member

ACK 47ea70f

@achow101 achow101 merged commit d1e4265 into bitcoin:master Jul 20, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 21, 2022
…ck code

47ea70f wallet: clean AllInputsMine code, use InputIsMine internally (furszy)
bf310b0 wallet: clean InputIsMine code, use GetWalletTx (furszy)
0cb1772 wallet: unify CachedTxGetImmatureCredit and CachedTxGetImmatureWatchOnlyCredit (furszy)
04c6423 wallet: remove always true 'fUseCache' arg from CachedTxGetAvailableCredit (furszy)
4f0ca9b wallet: remove always false 'recalculate' arg from GetCachableAmount (furszy)
47b1012 wallet: remove always true 'fUseCache' from CachedTxGetImmatureWatchOnlyCredit (furszy)
da8f62d wallet: remove always true 'fUseCache' from CachedTxGetImmatureCredit (furszy)

Pull request description:

  Another wallet's code garbage collector work. Part of the `mapWallet` encapsulation goal.

  Focused on the following points:

  1) Remove always true `fUseCache` argument from `CachedTxGetImmatureCredit`, `CachedTxGetImmatureWatchOnly` and `CachedTxGetAvailableCredit`.
  2) Remove always false `recalculate` argument from `GetCachableAmount`.
  3) Merge `CachedTxGetImmatureCredit` and `CachedTxGetImmatureWatchOnlyCredit` as they do share the exact same code.
  4) Clean `InputIsMine` method; use `GetWalletTx` instead of access the wallet's map directly.
  5) Clean `AllInputsMine` method; use `InputIsMine` instead of duplicate the exact same code internally.

ACKs for top commit:
  aureleoules:
    re-ACK 47ea70f
  achow101:
    ACK 47ea70f
  theStack:
    re-ACK 47ea70f

Tree-SHA512: e9b64b57de7be6165c5e5552e28cd8a03d4736b0a3707d29d129e3a0a3db6a855c2abf47a24917236060835a297b564a97b66d4c8b178d6bdafb93a12a7c0b40
@furszy furszy deleted the 2022_wallet_cleanup_1 branch May 27, 2023 01:49
@bitcoin bitcoin locked and limited conversation to collaborators May 26, 2024
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.

7 participants