Skip to content
This repository has been archived by the owner on May 13, 2022. It is now read-only.

Fix #588 Wallet list index out of range error #589

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fix #588 Wallet list index out of range error #589

wants to merge 1 commit into from

Conversation

AlexCato
Copy link
Contributor

@AlexCato AlexCato commented Jul 4, 2016

Uses the number of mixdepths from the wallet file

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 79.945% when pulling 141be19 on AlexCato:fix_wallet_index_error into 984626b on JoinMarket-Org:develop.

@AlexCato
Copy link
Contributor Author

AlexCato commented Jul 4, 2016

I think I found a problem with this solution:

Given there's a standard wallet with 5 mixdepths, but you start a yieldgenerator with a max_mixdepth-configuration of fewer than 5: then there will be wallet.json-file will be changed to contain the fewer index caches.
This PR then will make it impossible to ever be able to use the original number of mixdepths again.

Proposed solution:
Changing the default value of max_mixdepth in the configuration to -1 (from 5) and only read the number of mixdepths from the wallet file if the user does not specify the max_mixdepth.
Will code that later if noone sees a problem with that.

        auto-detect number of mixdepths if not overridden by user
@AlexCato
Copy link
Contributor Author

AlexCato commented Jul 4, 2016

Changed default in wallet-tool.py to -1 now. This will auto-detect the number of mixdepths only if the user does not explicitly configure it.

Therefor the default behavior is improved without any change in behavior if the user intentionally sets the parameter.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 79.958% when pulling b7db6d1 on AlexCato:fix_wallet_index_error into 984626b on JoinMarket-Org:develop.

@chris-belcher
Copy link
Collaborator

utACK on this. Anyone else have any thoughts?

@AdamISZ
Copy link
Member

AdamISZ commented Jan 4, 2017

Agreed. No conceivable negative I can see. Merging. Edit: on second thoughts, need to think a bit :)

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

Successfully merging this pull request may close these issues.

4 participants