Skip to content
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

waddrmgr: optimize using each style functions #216

Merged
merged 1 commit into from
May 22, 2015

Conversation

tuxcanfly
Copy link
Contributor

refs #215

WIP: Trying out the approach by replacing AllAccounts with a each style func first.

@davecgh
Copy link
Member

davecgh commented Mar 26, 2015

This looks like the discussed approach. However, unless we plan to add the ability to allow inactive accounts (which I don't really currently foresee), the name should probably be ForEachAccount instead of ForEachActiveAccount.

EDIT: Addresses, on the other hand, are definitely intended to be active/inactive, so naming that ForEachActiveAddress makes sense.

@tuxcanfly
Copy link
Contributor Author

Cool, let me correct the func names.

@davecgh
Copy link
Member

davecgh commented Mar 26, 2015

Looks good. I know there are more coming, so I won't merge it until everything is done.

@tuxcanfly
Copy link
Contributor Author

Thanks, will be working on the remaining funcs.

@tuxcanfly tuxcanfly force-pushed the waddrmgr-optimize branch 2 times, most recently from 40a3d6b to eed9785 Compare March 27, 2015 16:31
@tuxcanfly
Copy link
Contributor Author

Updated the remaining funcs. Working on optimizing storage for active/inactive addresses as discussed.

@tuxcanfly
Copy link
Contributor Author

Refactored to store all active addresses in a bucket and use it as an index while iterating over active addresses.

Added upgrade path for migrating from used bucket to active bucket. Running some checks, will be up for review in a bit.

@tuxcanfly tuxcanfly force-pushed the waddrmgr-optimize branch 2 times, most recently from 30a7ff1 to 02ba28c Compare April 7, 2015 15:27
@tuxcanfly
Copy link
Contributor Author

To avoid confusion, we should stick to differentiating active/inactive addresses. I've replaced all references to used with inactive.

@tuxcanfly tuxcanfly force-pushed the waddrmgr-optimize branch 2 times, most recently from d4b89c6 to e938d98 Compare April 7, 2015 18:49
@tuxcanfly
Copy link
Contributor Author

OK, ready for review. @davecgh

@tuxcanfly
Copy link
Contributor Author

Also renamed a few methods for consistency e.g AddrAccount to AddressAccount

@tuxcanfly
Copy link
Contributor Author

Depends on #238 in the way we will treat used addresses since this PR rewoks active/inactive (used) addrs in addition to each-style functions.

@tuxcanfly
Copy link
Contributor Author

As discussed on IRC, an address can expire after a duration and become inactive while still remaining unused. Therefore, instead of assuming that addr not present in active bucket is used, we should have an explicit db flag on address row using a flag field which determines whether the address is used or not. Similarly, the address should be marked expired after a certain configurable duration.

@tuxcanfly tuxcanfly force-pushed the waddrmgr-optimize branch 5 times, most recently from 4df6f33 to 8b1c588 Compare April 28, 2015 17:28
@tuxcanfly
Copy link
Contributor Author

Limiting this PR to ForEach implementations, created #254 for implementing active/inactive addresses, which I'll be working on.

@tuxcanfly tuxcanfly force-pushed the waddrmgr-optimize branch 2 times, most recently from 5915f39 to 0d7d4ce Compare May 14, 2015 08:17
_ = w.Manager.ForEachAccountAddress(account,
func(maddr waddrmgr.ManagedAddress) error {
used, err = maddr.Used()
return nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This throws away the error and re-assigns the possibly true used value instead of breaking out of the loop and returning true to the caller.

@jrick
Copy link
Member

jrick commented May 14, 2015

ActiveAccountAddresses wasn't switched over to the new style and I'd like to see some more details in each function comments about how it executes the passed function on each item, breaking early is an error is returned. Just saying "it iterates over X" isn't descriptive enough.

@tuxcanfly tuxcanfly force-pushed the waddrmgr-optimize branch from 0d7d4ce to 8c56747 Compare May 15, 2015 14:14
@tuxcanfly
Copy link
Contributor Author

Moved ActiveAccountAddresses to ForEachActiveAccountAddress and updated the docs to be more descriptive. Fixed AccountUsed to break out and return true on first used addr.

@jrick
Copy link
Member

jrick commented May 15, 2015

All of the calls to maybeConvertDbError will mangle the break error. I think that errors from this package should be converted, but errors from a caller's callback should not be.

@tuxcanfly tuxcanfly force-pushed the waddrmgr-optimize branch 5 times, most recently from 5e9875d to 54c5b8a Compare May 21, 2015 17:55
@tuxcanfly tuxcanfly force-pushed the waddrmgr-optimize branch from 54c5b8a to fbf744b Compare May 21, 2015 18:07
@tuxcanfly
Copy link
Contributor Author

Thanks, as discussed, I've added a global err Break to signal a early exit from the callback. Please review.

@jrick
Copy link
Member

jrick commented May 21, 2015

ok

@conformal-deploy conformal-deploy merged commit fbf744b into btcsuite:master May 22, 2015
@tuxcanfly
Copy link
Contributor Author

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants