wallet: cleanup cached amount and input mine check code #25543

pull furszy wants to merge 7 commits into bitcoin:master from furszy:2022_wallet_cleanup_1 changing 3 files +20 −48

furszy commented at 1:27 PM on July 5, 2022: member
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.
wallet: remove always true 'fUseCache' from CachedTxGetImmatureCredit da8f62de2c
wallet: remove always true 'fUseCache' from CachedTxGetImmatureWatchOnlyCredit 47b1012677
wallet: remove always false 'recalculate' arg from GetCachableAmount 4f0ca9bff6
wallet: remove always true 'fUseCache' arg from CachedTxGetAvailableCredit 04c6423f7b
wallet: unify CachedTxGetImmatureCredit and CachedTxGetImmatureWatchOnlyCredit 0cb177263c
wallet: clean InputIsMine code, use GetWalletTx bf310b0e8c
fanquake added the label Wallet on Jul 5, 2022
DrahtBot commented at 7:57 PM on July 5, 2022: contributor


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.

in src/wallet/test/wallet_tests.cpp:359 in ba2fa8302d outdated

 356 |      // Invalidate the cached value, add the key, and make sure a new immature
 357 |      // credit amount is calculated.
 358 |      wtx.MarkDirty();
 359 |      AddKey(wallet, coinbaseKey);
 360 | -    BOOST_CHECK_EQUAL(CachedTxGetImmatureCredit(wallet, wtx), 50*COIN);
 361 | +    BOOST_CHECK_EQUAL(CachedTxGetImmatureCredit(wallet, wtx, ISMINE_SPENDABLE), 50*COIN);

aureleoules commented at 6:13 PM on July 6, 2022:

nit

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

aureleoules commented at 6:13 PM on July 6, 2022: member

ACK ba2fa8302dc3816d5b9584907135f91789846d65. Looks better! I checked that the refactor does not change the behavior of the code.
theStack approved
theStack commented at 8:36 PM on July 6, 2022: contributor

Code-review ACK ba2fa8302dc3816d5b9584907135f91789846d65

Nice refactoring!
wallet: clean AllInputsMine code, use InputIsMine internally
Instead of duplicate the exact same code twice.
47ea70fbb8
in src/wallet/receive.cpp:27 in ba2fa8302d outdated
```
  42 | -            return false; // invalid input!
  43 | -
  44 | -        if (!(wallet.IsMine(prev.tx->vout[txin.prevout.n]) & filter))
  45 | +    for (const CTxIn& txin : tx.vin) {
  46 | +        if (!(InputIsMine(wallet, txin) & filter))
  47 |              return false;
```
jonatack commented at 8:44 PM on July 6, 2022:

While here, could you add the missing brackets to the conditional per the developer notes, or hoist return false; to the same line?

We require brackets with conditionals over one line due to CVE-2014-1266 (the Apple "goto fail" vulnerability, see also https://dwheeler.com/essays/apple-goto-fail).

furszy commented at 9:09 PM on July 6, 2022:

pushed
furszy force-pushed on Jul 6, 2022
furszy commented at 9:11 PM on July 6, 2022: member

per feedback, have inlined AllInputsMine return statement.
theStack approved
theStack commented at 10:58 AM on July 7, 2022: contributor

re-ACK 47ea70fbb85fefeb4de9d3142a11596d292eab9b
furszy commented at 2:42 PM on July 20, 2022: member

hey @jonatack @aureleoules, friendly ping here.
aureleoules commented at 2:45 PM on July 20, 2022: member

re-ACK 47ea70fbb85fefeb4de9d3142a11596d292eab9b
achow101 commented at 8:49 PM on July 20, 2022: member

ACK 47ea70fbb85fefeb4de9d3142a11596d292eab9b
achow101 merged this on Jul 20, 2022
achow101 closed this on Jul 20, 2022
sidhujag referenced this in commit 52c3930767 on Jul 21, 2022
furszy deleted the branch on May 27, 2023
bitcoin locked this on May 26, 2024

Contributors

Labels

wallet: cleanup cached amount and input mine check code #25543

Conflicts