wallet: add cachable amounts for caching credit/debit values #15780

kallewoof · 2019-04-10T02:42:38Z

This is a refactoring that will make #13756 a lot cleaner and straight-forward, since it adds another combination to the pile (watch-only * spendable * reused).

It's also a nice change in general.

src/script/ismine.h

src/wallet/wallet.cpp

gwillen · 2019-04-10T05:04:37Z

src/script/ismine.h

I know it seems obvious now, but it would probably help future readers a bit to comment the fact that these correspond to the four values of isminetype. (Or you could add ISMINE_ENUM_ELEMENTS or the like and use that instead of the constant 4.)

In fact, actually, after studying the code -- we aren't really using a bitfield of IsMine types here. Only ISMINE_SPENDABLE and ISMINE_WATCHONLY are valid. Neither ISMINE_NO nor ISMINE_ALL are things we would ever want to cache here. Although I see that in #13756 you are maybe making more use of the fact that this is a bitfield, and I haven't read that one, so I assume that informs your choices here.

Yes, there is a tiny bit of waste due to ISMINE_NO and _ALL, and yep I am preparing to use this more extensively for the avoid_reuse feature in #13756. I could make a "bridge" that makes use of these more efficiently, but I kind of enjoy the simplicity of the direct mapping.

Should add that I adopted the ISMINE_ENUM_ELEMENTS idea.

src/wallet/wallet.cpp

kallewoof · 2019-04-10T07:12:31Z

@gwillen Thank you for reviewing! I have addressed your comments, I believe.

DrahtBot · 2019-04-10T07:21:43Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#13756 (wallet: "avoid_reuse" wallet flag for improved privacy 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.

src/script/ismine.h

laanwj · 2019-04-10T08:50:44Z

Concept ACK! this groups similar functionality together making it, overall, easier to understand.
I was afraid for a bit with the word "accounts" though (having just finally deprecated that).

kallewoof · 2019-04-10T09:36:40Z

Thanks for review @promag. I believe I addressed all your comments.

kallewoof · 2019-04-10T09:37:58Z

@laanwj Maybe I should have picked a different name! I think "account" works in this case, though... :)

laanwj · 2019-04-10T12:11:41Z

@laanwj Maybe I should have picked a different name! I think "account" works in this case, though... :)

Yes to be clear it was not a suggestion to change it 🙂 Would only have been a potential concern if it would end up in the RPC API.

gwillen · 2019-04-10T21:13:12Z

utACK 3b9a9a4

src/script/ismine.h

kallewoof · 2019-04-11T14:34:12Z

@promag @MarcoFalke Thank you for the review! I believe I addressed all your feedback.

promag

utACK fda0ade00739f30bdfa9d576371a350c1c4ade5f.

promag · 2019-04-14T10:09:26Z

src/script/ismine.h

nit, use http://www.cplusplus.com/reference/bitset/bitset/set/:

m_cached.set(filter)

promag · 2019-04-14T10:11:37Z

src/script/ismine.h

Since CachableAccount members are public you could ditch Reset and call m_cached.reset() directly. Otherwise you could make members private and add IsCached(), Get() and Type() accessors.

I think leaving it public is ok, but I can be convinced to make it private. For now, I made Reset inline, which basically has the same effect as what you are suggesting without exposing too many innards.

meshcollider

utACK 1603501, I like this cleanup :)

meshcollider · 2019-04-15T06:51:55Z

src/wallet/wallet.cpp

Why not simplify to just return GetCachable(m_credit_immature, ISMINE_SPENDABLE, !fUseCache);? (same in GetImmatureWatchOnlyCredit)

Good point! Fixed.

promag · 2019-04-15T07:16:41Z

utACK 18f23ae0c5599f2246e58e5293f7426471b73e9c, only change is to use GetCachable in GetImmatureCredit and
GetImmatureWatchOnlyCredit`.

maflcko · 2019-04-17T15:46:39Z

Would be nice if the benchmark could make it in first, so that we are not accidentally regressing on performance.

test: Add wallet_balance benchmark test: Add wallet_balance benchmark #15779

ryanofsky

utACK d8fdf6b670e179282faff4ff57e37a6047efac9c. Thanks for the changes!

Would be nice if the benchmark could make it in first, so that we are not accidentally regressing on performance.

I'll review the benchmark, but I'd be pretty surprised if this affects performance significantly, since it is a basically a fancy variable rename, and doesn't really change existing logic.

ryanofsky · 2019-04-17T15:55:51Z

src/script/ismine.h

Note: Could add doxygen comment suggested previously #15780 (comment)

//! Cached amount subdivided into watchonly and spendable parts.

Oops, I missed that part. Thanks, done. (I used /**...*/)

ryanofsky · 2019-04-17T16:01:51Z

src/wallet/wallet.cpp

Note: Could call this GetAmount instead of GetCachable as suggested previously #15780 (comment) because this returns an amount and now handles caching internally instead of taking a cache reference.

The recalculate bool is for the cache, though. Maybe GetCachableAmount is better. Using that.

ryanofsky

utACK 46a7d7b1983ebf0a98b96a09d83d9f63ab81dbcc. Just method rename and new code comment since last review.

ryanofsky · 2019-04-18T16:18:06Z

src/wallet/wallet.cpp

I don't want to suggest a change for this PR, since it's good that this just rearranges variables and doesn't change any computation.

But in the future, it seems like IMMATURE_CREDIT could be replaced with CREDIT here without affecting anything, and IMMATURE_CREDIT and AVAILABLE_CREDIT could go away. Unless I'm missing something, it seems like a lot of the previous variables like nImmatureCreditCached were just redundant.

Nice! That would definitely improve things. I agree that's beyond this PR but will gladly dig into it if nobody else does after merge.

promag · 2019-04-18T19:01:13Z

utACK 46a7d7b.

maflcko

utACK 46a7d7b1983ebf0a98b96a09d83d9f63ab81dbcc

Show signature

maflcko · 2019-04-22T19:18:45Z

src/script/ismine.h

Could add a comment to say that NO and ALL are never (supposed to be) cached?

Thanks, done. I also fixed typo in wallet.cpp#1990 (said "Avoid caching ismine or NONE or all cases" before).

The two comments contradict each other:

// NO and ALL are never (supposed to be) cached

// Avoid caching ismine for NO or ALL cases (could remove this check and simplify in the future).

If it's harmful to cache these values, we should drop the second comment and explain the harm. If it's not harmful, we should drop the first comment because it's misleading and irrelevant.

In the future, it would be good to get rid of all unused variables and special cases by switching to a 2 part cache or just caching the 4 parts unconditionally.

promag · 2019-04-23T09:53:05Z

utACK c9e6e7e.

maflcko · 2019-04-23T11:38:35Z

re-utACK c9e6e7e (Only change is comments)

Show signature

ryanofsky

utACK c9e6e7e. Just comment changes since last review.

ryanofsky · 2019-04-23T14:38:56Z

src/script/ismine.h

The two comments contradict each other:

// NO and ALL are never (supposed to be) cached

// Avoid caching ismine for NO or ALL cases (could remove this check and simplify in the future).

If it's harmful to cache these values, we should drop the second comment and explain the harm. If it's not harmful, we should drop the first comment because it's misleading and irrelevant.

In the future, it would be good to get rid of all unused variables and special cases by switching to a 2 part cache or just caching the 4 parts unconditionally.

…alues c9e6e7e wallet: add cachable amounts for caching credit/debit values (Karl-Johan Alm) Pull request description: This is a refactoring that will make #13756 a lot cleaner and straight-forward, since it adds another combination to the pile (watch-only * spendable * reused). It's also a nice change in general. Tree-SHA512: 6c876d58bbffd5cb85ef632dea4fd6afed163904bbde5efdb307fa119af178ed3cb5df047255da7e9a9136fed876922f1116fce61a3710f308c72275f9b7d18b

5ebc6b0 bitcoind: update -avoidpartialspends description to account for auto-enable for avoid_reuse wallets (Karl-Johan Alm) ada258f doc: release notes for avoid_reuse (Karl-Johan Alm) 2766955 wallet: enable avoid_partial_spends by default if avoid_reuse is set (Karl-Johan Alm) 8f2e208 test: add test for avoidreuse feature (Karl-Johan Alm) 0bdfbd3 wallet/rpc: add 'avoid_reuse' option to RPC commands (Karl-Johan Alm) f904723 wallet/rpc: add setwalletflag RPC and MUTABLE_WALLET_FLAGS (Karl-Johan Alm) 8247a0d wallet: enable avoid_reuse feature (Karl-Johan Alm) eec1566 wallet: avoid reuse flags (Karl-Johan Alm) 5892809 wallet: make IsWalletFlagSet() const (Karl-Johan Alm) 129a5ba wallet: rename g_known_wallet_flags constant to KNOWN_WALLET_FLAGS (Karl-Johan Alm) Pull request description: Add a new wallet flag called `avoid_reuse` which, when enabled, will keep track of when a specific destination has been spent from, and will actively "blacklist" any new UTXOs which send to an already-spent-from destination. This improves privacy, as a payer could otherwise begin tracking a payee's wallet by regularly peppering a known UTXO with dust outputs, which would then be scooped up and used in payments by the payee, allowing the payer to map out (1) the inputs owned by the payee and (2) the destinations to which the payee is making payments. This replaces #10386 and together with the (now merged) #12257 it addresses #10065 in full. The concerns raised in #10386 (comment) are also addressed due to #12257. ~~Note: this builds on top of #15780.~~ (merged) ACKs for commit 5ebc6b: jnewbery: ACK 5ebc6b0 laanwj: Concept and code-review ACK 5ebc6b0 meshcollider: Code review ACK 5ebc6b0 achow101: ACK 5ebc6b0 modulo above nits Tree-SHA512: fdef45826af544cbbb45634ac367852cc467ec87081d86d08b53ca849e588617e9a0a255b7e7bb28692d15332de58d6c3d274ac003355220e4213d7d9070742e

…rivacy 5ebc6b0 bitcoind: update -avoidpartialspends description to account for auto-enable for avoid_reuse wallets (Karl-Johan Alm) ada258f doc: release notes for avoid_reuse (Karl-Johan Alm) 2766955 wallet: enable avoid_partial_spends by default if avoid_reuse is set (Karl-Johan Alm) 8f2e208 test: add test for avoidreuse feature (Karl-Johan Alm) 0bdfbd3 wallet/rpc: add 'avoid_reuse' option to RPC commands (Karl-Johan Alm) f904723 wallet/rpc: add setwalletflag RPC and MUTABLE_WALLET_FLAGS (Karl-Johan Alm) 8247a0d wallet: enable avoid_reuse feature (Karl-Johan Alm) eec1566 wallet: avoid reuse flags (Karl-Johan Alm) 5892809 wallet: make IsWalletFlagSet() const (Karl-Johan Alm) 129a5ba wallet: rename g_known_wallet_flags constant to KNOWN_WALLET_FLAGS (Karl-Johan Alm) Pull request description: Add a new wallet flag called `avoid_reuse` which, when enabled, will keep track of when a specific destination has been spent from, and will actively "blacklist" any new UTXOs which send to an already-spent-from destination. This improves privacy, as a payer could otherwise begin tracking a payee's wallet by regularly peppering a known UTXO with dust outputs, which would then be scooped up and used in payments by the payee, allowing the payer to map out (1) the inputs owned by the payee and (2) the destinations to which the payee is making payments. This replaces bitcoin#10386 and together with the (now merged) bitcoin#12257 it addresses bitcoin#10065 in full. The concerns raised in bitcoin#10386 (comment) are also addressed due to bitcoin#12257. ~~Note: this builds on top of bitcoin#15780.~~ (merged) ACKs for commit 5ebc6b: jnewbery: ACK 5ebc6b0 laanwj: Concept and code-review ACK 5ebc6b0 meshcollider: Code review ACK bitcoin@5ebc6b0 achow101: ACK 5ebc6b0 modulo above nits Tree-SHA512: fdef45826af544cbbb45634ac367852cc467ec87081d86d08b53ca849e588617e9a0a255b7e7bb28692d15332de58d6c3d274ac003355220e4213d7d9070742e

…t values Summary: bitcoin/bitcoin@c9e6e7e --- Backport of Core [[bitcoin/bitcoin#15780 | PR15780]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, nakihito Reviewed By: nakihito Differential Revision: https://reviews.bitcoinabc.org/D6347

…values e02edd3 Cleaning isminetype::ISMINE_SPENDABLE_STAKEABLE. (furszy) 1bf9dc4 wallet: unify GetUnspentCredit with GetCredit (furszy) 9f2ae47 wallet: remove currently unused UpdateAmount method. (furszy) bef69d3 wallet: add cachable amounts for caching credit/debit values (furszy) 8f2a156 Make IsMine stop distinguishing solvable/unsolvable (furszy) e541593 Make coincontrol use IsSolvable to determine solvability (furszy) e1a0cfb Back port of IsSolvable to check scripts solvability. (furszy) Pull request description: Built on top of #1755 and #1666 . The goal of this PR has been to improve the `CWalletTx` to be able to add shielded cached balances in a much cleaner way. Including the following changes: * Adapted IsSolvable function from 0c8ea63 (without the witness check. Only used to check regular outputs to be able to decouple the output solvability from `ismine.h` enum) * Solvability decoupled from `ismine.h`. Back ported 6d714c3 and 4e91820 from bitcoin#13142. * And lastly, adapted bitcoin#15780 beauty cleanup to our sources. ACKs for top commit: random-zebra: utACK e02edd3 Tree-SHA512: 3c54397585f2ce1a47a5a86292216b5beab6cbf1e4ad6c289a1daa958cf4c535855bac1e13a7de290bd8d374abef3da7a909b11ee0f5016af31e90f642b448d5

kallewoof force-pushed the cachable-accounts branch 2 times, most recently from 46f3146 to e831a26 Compare April 10, 2019 02:49

fanquake added the Wallet label Apr 10, 2019

kallewoof mentioned this pull request Apr 10, 2019

wallet: "avoid_reuse" wallet flag for improved privacy #13756

Merged

kallewoof force-pushed the cachable-accounts branch 2 times, most recently from 749787c to 17d2688 Compare April 10, 2019 04:38

gwillen reviewed Apr 10, 2019

View reviewed changes

kallewoof force-pushed the cachable-accounts branch from 17d2688 to 5b4ba10 Compare April 10, 2019 07:10

promag reviewed Apr 10, 2019

View reviewed changes

src/script/ismine.h Outdated Show resolved Hide resolved

promag reviewed Apr 10, 2019

View reviewed changes

src/script/ismine.h Outdated Show resolved Hide resolved

kallewoof force-pushed the cachable-accounts branch from 5b4ba10 to 1fbc051 Compare April 10, 2019 08:55

kallewoof force-pushed the cachable-accounts branch 2 times, most recently from 27e9469 to 3b9a9a4 Compare April 10, 2019 10:09

promag reviewed Apr 11, 2019

View reviewed changes

src/script/ismine.h Outdated Show resolved Hide resolved

src/script/ismine.h Outdated Show resolved Hide resolved

src/script/ismine.h Outdated Show resolved Hide resolved

src/script/ismine.h Outdated Show resolved Hide resolved

kallewoof force-pushed the cachable-accounts branch 2 times, most recently from 1eb132e to d9c7ec9 Compare April 11, 2019 14:30

kallewoof force-pushed the cachable-accounts branch from d9c7ec9 to fda0ade Compare April 11, 2019 15:10

promag reviewed Apr 14, 2019

View reviewed changes

kallewoof force-pushed the cachable-accounts branch from fda0ade to 1603501 Compare April 15, 2019 00:37

meshcollider reviewed Apr 15, 2019

View reviewed changes

kallewoof force-pushed the cachable-accounts branch from 1603501 to 18f23ae Compare April 15, 2019 07:09

ryanofsky approved these changes Apr 17, 2019

View reviewed changes

kallewoof force-pushed the cachable-accounts branch 2 times, most recently from 1de3ff8 to 46a7d7b Compare April 18, 2019 02:40

ryanofsky approved these changes Apr 18, 2019

View reviewed changes

ryanofsky reviewed Apr 18, 2019

View reviewed changes

maflcko approved these changes Apr 22, 2019

View reviewed changes

maflcko added the Refactoring label Apr 22, 2019

wallet: add cachable amounts for caching credit/debit values

c9e6e7e

kallewoof force-pushed the cachable-accounts branch from 46a7d7b to c9e6e7e Compare April 22, 2019 23:19

ryanofsky approved these changes Apr 23, 2019

View reviewed changes

laanwj merged commit c9e6e7e into bitcoin:master Apr 23, 2019

kallewoof deleted the cachable-accounts branch April 23, 2019 22:38

furszy mentioned this pull request Jul 21, 2020

[Wallet] add cacheable amounts for caching credit/debit values PIVX-Project/PIVX#1757

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

wallet: add cachable amounts for caching credit/debit values #15780

wallet: add cachable amounts for caching credit/debit values #15780

Uh oh!

Conversation

kallewoof commented Apr 10, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kallewoof commented Apr 10, 2019

Uh oh!

DrahtBot commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

laanwj commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Apr 10, 2019

Uh oh!

kallewoof commented Apr 10, 2019

Uh oh!

laanwj commented Apr 10, 2019

Uh oh!

gwillen commented Apr 10, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kallewoof commented Apr 11, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Apr 15, 2019

Uh oh!

maflcko commented Apr 17, 2019

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kallewoof Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Apr 10, 2019 •

edited

Loading

laanwj commented Apr 10, 2019 •

edited

Loading

ryanofsky left a comment •

edited

Loading

kallewoof Apr 18, 2019 •

edited

Loading