Reduce inefficiency of GetAccountAddress() #7262

pull dooglus wants to merge 1 commits into bitcoin:master from dooglus:faster-getaccountaddress changing 1 files +15 −16

dooglus commented at 12:58 AM on December 29, 2015: contributor
- Don't scan the wallet to see if the current key has been used if we're going to make a new key anyway.
- Stop scanning the wallet as soon as we see that the current key has been used.
- Don't call isValid() twice on the current key.

Reduce inefficiency of GetAccountAddress()

Don't scan the wallet to see if the current key has been used if we're going to make a new key anyway.
Stop scanning the wallet as soon as we see that the current key has been used.
Don't call isValid() twice on the current key.

2409865e14

dcousens commented at 1:35 AM on December 29, 2015: contributor

utACK
pstratem commented at 1:43 AM on December 29, 2015: contributor

NACK accounts are deprecated

performance improvements for code which will be removed isn't worth it
dcousens commented at 1:44 AM on December 29, 2015: contributor

@pstratem is there a concrete list of deprecations somewhere?
pstratem commented at 1:51 AM on December 29, 2015: contributor

@dcousens #5575
dcousens commented at 2:35 AM on December 29, 2015: contributor

Why do they still exist if they were deprecated prior to 0.11? Shouldn't they be removed for 0.12?
dooglus commented at 3:25 AM on December 29, 2015: contributor

Luke wrote "merchants and exchanges [...] shouldn't be using Bitcoin Core's wallet implementation at all right now" to which sipa responded "whether they should or not, I'm pretty sure that several do, and we don't want them to stop upgrading their software because we drop a feature".

I guess that's the crux of the matter. Dropping a feature that is in use without offering a replacement feature will cause people to either stop upgrading or worse.
dcousens commented at 3:33 AM on December 29, 2015: contributor

@laanwj this is obviously up to you, but IMHO we should NACK this if it is actually deprecated, then promptly remove all of the accounting features entirely (for 0.13, or even 0.12 if possible, may be too late now though).

If its staying around for a few more versions, then, IMHO, we should maintain it (aka, ACK).
laanwj commented at 8:39 AM on January 4, 2016: member

No problem with using bitcoin core's wallet implementation, but indeed, the accounts mechanism is going to be removed at some point.
laanwj commented at 8:41 AM on January 4, 2016: member

In any case that's not a reason for not merging this - this doesn't actually extend the API (which would be problematic) but purports make the current implementation more efficient. If this is code review correct and tested there's no reason not to merge it.
pstratem commented at 7:10 PM on January 5, 2016: contributor

@laanwj if anything we should be making these rpc calls slow in the hope that people notice they're deprecated
laanwj added the label Wallet on Jan 7, 2016
luke-jr commented at 8:55 PM on January 15, 2016: member

Note that one reason accounts are not removed yet, is because we have no alternative to getaccountaddress yet... and it's likely this code will get reused in implementing whatever replaces it. So concept ACK.

in src/wallet/rpcwallet.cpp:None in 2409865e14

 158 | +            // Check if the current key has been used
 159 | +            CScript scriptPubKey = GetScriptForDestination(account.vchPubKey.GetID());
 160 | +            for (map<uint256, CWalletTx>::iterator it = pwalletMain->mapWallet.begin();
 161 | +                 it != pwalletMain->mapWallet.end() && account.vchPubKey.IsValid();
 162 | +                 ++it)
 163 | +                BOOST_FOREACH(const CTxOut& txout, (*it).second.vout)

luke-jr commented at 8:59 PM on January 15, 2016:

Flattening the wtx variable doesn't actually optimise anything, and makes it slightly harder to follow. I'd prefer to not change this aspect.

luke-jr commented at 8:59 PM on January 15, 2016: member

Also utACK, with mentioned nit.
jonasschnelli commented at 1:07 PM on January 20, 2016: contributor

utACK. I agree that we should not extend the account API, but – as long as it's not removed – we should maintain it and should therefore accept optimizations.
laanwj merged this on Jan 22, 2016
laanwj closed this on Jan 22, 2016
laanwj referenced this in commit fc08994000 on Jan 22, 2016
luke-jr commented at 12:33 AM on January 30, 2016: member

Perhaps of interest: BFGMiner uses getaccountaddress, and it adds a 1-2 second delay after finding a block, before it switches to mining the next one... Not sure the optimisations here are sufficient, but we definitely should improve this.
pstratem commented at 12:38 AM on January 30, 2016: contributor

@luke-jr er... why?
luke-jr commented at 12:51 AM on January 30, 2016: member

Gets a non-reused address without filling the wallet with keys that invalidate backups.
luke-jr referenced this in commit fb485f57b5 on Feb 13, 2016
luke-jr referenced this in commit 4a050b910b on Jun 28, 2016
luke-jr referenced this in commit 6bfc2dc6d9 on Jun 28, 2016
codablock referenced this in commit 03a89b9472 on Sep 16, 2017
codablock referenced this in commit 9ef1bb5bb1 on Sep 19, 2017
codablock referenced this in commit 4ff561e629 on Dec 9, 2017
codablock referenced this in commit 643181fe15 on Dec 9, 2017
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#3029 Upgrade gitian win32 to Precise 12.04.