Reduce inefficiency of GetAccountAddress() #7262
Conversation
dooglus
commented
Dec 29, 2015
dooglus
commented
- 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.
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.
|
utACK |
|
NACK accounts are deprecated performance improvements for code which will be removed isn't worth it |
|
@pstratem is there a concrete list of deprecations somewhere? |
|
Why do they still exist if they were deprecated prior to |
|
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. |
|
@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 If its staying around for a few more versions, then, IMHO, we should maintain it (aka, ACK). |
|
No problem with using bitcoin core's wallet implementation, but indeed, the accounts mechanism is going to be removed at some point. |
|
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. |
|
@laanwj if anything we should be making these rpc calls slow in the hope that people notice they're deprecated |
|
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. |
There was a problem hiding this comment.
Flattening the wtx variable doesn't actually optimise anything, and makes it slightly harder to follow. I'd prefer to not change this aspect.
|
Also utACK, with mentioned nit. |
|
utACK. |
2409865 Reduce inefficiency of GetAccountAddress() (Chris Moore)
|
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. |
|
@luke-jr er... why? |
|
Gets a non-reused address without filling the wallet with keys that invalidate backups. |
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. Github-Pull: bitcoin#7262 Rebased-From: 2409865
2409865 Reduce inefficiency of GetAccountAddress() (Chris Moore)
6 participants
