Improve CWallet::IsAllFromMe for false results #12297

pull promag wants to merge 2 commits into bitcoin:master from promag:2018-01-isallfromme changing 1 files +23 −10

promag commented at 11:34 PM on January 29, 2018: member
Break the function into 2 loops so that:
- the fast test is executed first for all inputs.
- IsMine(txout) is only executed after all inputs are checked.
This is relevant since after #12296 CWallet::IsAllFromMe is called from the UI.
fanquake added the label Wallet on Jan 29, 2018
promag force-pushed on Jan 29, 2018
promag force-pushed on Jan 30, 2018
laanwj commented at 8:54 AM on January 31, 2018: member

This needs benchmarking/profliling results. As discussed on IRC, it's not clear how to evaluate changes like this right now.
in src/wallet/wallet.cpp:1425 in 8ee4bd5837 outdated
```
1429 | +        outs.push_back(&prev.tx->vout[txin.prevout.n]);
1430 |      }
1431 | +
1432 | +    assert(outs.size() == tx.vin.size());
1433 | +
1434 | +    // then check if every isminetype matches filter
```
ryanofsky commented at 10:30 PM on February 5, 2018:

Would suggest adding a comment that the code is intentionally written to use 2 passes for better performance. Otherwise it's unclear why the code would be organized this way.

promag commented at 11:15 PM on February 5, 2018:

Done. PR description also explains the idea behind the 2 loops.
ryanofsky commented at 10:31 PM on February 5, 2018: member

utACK 8ee4bd5837d22304c194b38293fbc85b4899cba9
[wallet] Improve IsAllFromMe for false results a9eb90c0d4
[wallet] Replace lock with assertion in CWallet::IsAllFromMe 6037efd7c5
promag force-pushed on Feb 5, 2018
promag commented at 11:16 PM on February 5, 2018: member

If necessary I can work out some benchmarking. Please let me know.
promag commented at 11:36 PM on February 5, 2018: member

@ryanofsky also took the opportunity to push a commit replacing LOCK(cs_wallet) with AssertLockHeld(cs_wallet). I can push to a new PR if preferable.
jimpo commented at 11:49 PM on May 17, 2018: contributor

Agree on this needing benchmarks.
DrahtBot closed this on Jul 20, 2018
DrahtBot commented at 8:29 PM on July 20, 2018: member

The last travis run for this pull request was 164 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
DrahtBot reopened this on Jul 20, 2018
Empact commented at 4:12 AM on August 3, 2018: member

How about splitting the lock change out, and making use of EXCLUSIVE_LOCKS_REQUIRED?
DrahtBot commented at 1:26 PM on September 21, 2018: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #12508 (IsAllFromMe by kallewoof)
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/wallet.cpp:1409 in 6037efd7c5

1407 | -    for (const CTxIn& txin : tx.vin)
1408 | -    {
1409 | +    std::vector<const CTxOut*> outs;
1410 | +    outs.reserve(tx.vin.size());
1411 | +
1412 | +    // The folllwing is a 2 pass algorithm. Both passes can early return if an

practicalswift commented at 8:29 PM on October 16, 2018:

Should be "following" :-)

meshcollider commented at 11:10 AM on November 11, 2018: contributor

The code changes look sane, light code review utACK https://github.com/bitcoin/bitcoin/pull/12297/commits/6037efd7c5087b1e53f426c0e48595dc073da9d3 modulo benchmarking as mentioned above
promag commented at 3:50 PM on November 26, 2018: member

CWallet::IsAllFromMe is only called from feebumper::CreateTransaction. I guess this performance improvement is not worth it.
promag closed this on Nov 26, 2018
promag deleted the branch on Nov 26, 2018
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#12296 wallet: Only fee-bump non-conflicted/non-confirmed txes

Improve CWallet::IsAllFromMe for false results #12297

Conflicts