New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce inefficiency of GetAccountAddress() #7262

Merged
merged 1 commit into from Jan 22, 2016

Conversation

Projects
None yet
6 participants
@dooglus
Contributor

dooglus commented Dec 29, 2015

  • 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.
@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Dec 29, 2015

Contributor

utACK

Contributor

dcousens commented Dec 29, 2015

utACK

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Dec 29, 2015

Contributor

NACK accounts are deprecated

performance improvements for code which will be removed isn't worth it

Contributor

pstratem commented Dec 29, 2015

NACK accounts are deprecated

performance improvements for code which will be removed isn't worth it

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Dec 29, 2015

Contributor

@pstratem is there a concrete list of deprecations somewhere?

Contributor

dcousens commented Dec 29, 2015

@pstratem is there a concrete list of deprecations somewhere?

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem
Contributor

pstratem commented Dec 29, 2015

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Dec 29, 2015

Contributor

Why do they still exist if they were deprecated prior to 0.11?
Shouldn't they be removed for 0.12?

Contributor

dcousens commented Dec 29, 2015

Why do they still exist if they were deprecated prior to 0.11?
Shouldn't they be removed for 0.12?

@dooglus

This comment has been minimized.

Show comment
Hide comment
@dooglus

dooglus Dec 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.

Contributor

dooglus commented Dec 29, 2015

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

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Dec 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).

Contributor

dcousens commented Dec 29, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 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.

Member

laanwj commented Jan 4, 2016

No problem with using bitcoin core's wallet implementation, but indeed, the accounts mechanism is going to be removed at some point.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 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.

Member

laanwj commented Jan 4, 2016

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

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Jan 5, 2016

Contributor

@laanwj if anything we should be making these rpc calls slow in the hope that people notice they're deprecated

Contributor

pstratem commented Jan 5, 2016

@laanwj if anything we should be making these rpc calls slow in the hope that people notice they're deprecated

@laanwj laanwj added the Wallet label Jan 7, 2016

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jan 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.

Member

luke-jr commented Jan 15, 2016

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.

for (map<uint256, CWalletTx>::iterator it = pwalletMain->mapWallet.begin();
it != pwalletMain->mapWallet.end() && account.vchPubKey.IsValid();
++it)
BOOST_FOREACH(const CTxOut& txout, (*it).second.vout)

This comment has been minimized.

@luke-jr

luke-jr Jan 15, 2016

Member

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

luke-jr Jan 15, 2016

Member

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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jan 15, 2016

Member

Also utACK, with mentioned nit.

Member

luke-jr commented Jan 15, 2016

Also utACK, with mentioned nit.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jan 20, 2016

Member

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.

Member

jonasschnelli commented Jan 20, 2016

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 laanwj merged commit 2409865 into bitcoin:master Jan 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 22, 2016

Merge #7262: Reduce inefficiency of GetAccountAddress()
2409865 Reduce inefficiency of GetAccountAddress() (Chris Moore)
@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jan 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.

Member

luke-jr commented Jan 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Jan 30, 2016

Contributor

@luke-jr er... why?

Contributor

pstratem commented Jan 30, 2016

@luke-jr er... why?

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jan 30, 2016

Member

Gets a non-reused address without filling the wallet with keys that invalidate backups.

Member

luke-jr commented Jan 30, 2016

Gets a non-reused address without filling the wallet with keys that invalidate backups.

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016

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.

Github-Pull: #7262
Rebased-From: 2409865

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 28, 2016

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #7262: Reduce inefficiency of GetAccountAddress()
2409865 Reduce inefficiency of GetAccountAddress() (Chris Moore)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #7262: Reduce inefficiency of GetAccountAddress()
2409865 Reduce inefficiency of GetAccountAddress() (Chris Moore)

codablock added a commit to codablock/dash that referenced this pull request Dec 9, 2017

Merge #7262: Reduce inefficiency of GetAccountAddress()
2409865 Reduce inefficiency of GetAccountAddress() (Chris Moore)

codablock added a commit to codablock/dash that referenced this pull request Dec 9, 2017

Merge #7262: Reduce inefficiency of GetAccountAddress()
2409865 Reduce inefficiency of GetAccountAddress() (Chris Moore)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment