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

Add package wtxmgr to use walletdb interface #154

Closed
wants to merge 9 commits into from

Conversation

tuxcanfly
Copy link
Contributor

NOTE: This pull request requires #147

Moved txstore to legacy and added package wtxmgr which provides walletdb support.

Legacy txstore will be imported as a part of the wallet setup as implemented in #147.

Working on docs, cleanup and rebase.

@davecgh
Copy link
Member

davecgh commented Dec 17, 2014

Thanks @tuxcanfly. I'll go through and review this as time permits. It's obviously a huge change.

@tuxcanfly
Copy link
Contributor Author

Thanks, please start with #155. Will be working on docs, cleanup and refactor of this PR meanwhile.

// outputs spendable with wallet keys and transactions that are signed by
// wallet keys in memory, handle spend tracking for newly-inserted
// transactions, report the spendable balance from each unspent
// transaction output, and finally to provide a means to serialize the
Copy link
Member

Choose a reason for hiding this comment

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

This section is no longer true.

@davecgh
Copy link
Member

davecgh commented Jan 12, 2015

There is a major lack of comments. Tons of unexported functions without comments, details not explained, etc.

@tuxcanfly
Copy link
Contributor Author

@davecgh Sorry, let me get those and also incorporate suggestions from the previous PR and get back to you before reviewing this.

@davecgh
Copy link
Member

davecgh commented Jan 12, 2015

@tuxcanfly No rush on this. It's going to take a while to get through it all and I still have more to review on the other PR too. Just was taking a quick perusal through this.

@tuxcanfly
Copy link
Contributor Author

OK, thanks. I have already addressed most of the items in the other PR, so shouldn't be long before getting back to this.

@tuxcanfly tuxcanfly force-pushed the wtxmgr branch 7 times, most recently from 434d15f to 90ab004 Compare January 21, 2015 13:04
@tuxcanfly
Copy link
Contributor Author

Rebased on top of btcsuite migration. Added docs for all newly introduced changes. Will be cross checking for addressable items from previous reviews, but should generally be good for review now.

@tuxcanfly tuxcanfly force-pushed the wtxmgr branch 4 times, most recently from 63004fd to 33d1b32 Compare January 22, 2015 12:20
@davecgh
Copy link
Member

davecgh commented Jan 25, 2015

One of the things we'll need before this can go in is a new RPC to force the txstore to be recreated. Perhaps something like "recalculatebalances". Previously, if anything got out of sync, we could simply delete the separate file to force this to happen, but that will no longer be possible once it's part of the overall wallet database.

This becomes particularly important when BIP0032 keys are being used since a user could easily generate the next keys in sequence from something external and receive funds to them. The wallet would obviously not yet know about these. Subsequent calls to getnewaddress would, of course, make them known to wallet, but at that point it won't be rescanning those earlier blocks where the funds were received.

@jrick
Copy link
Member

jrick commented Feb 5, 2015

We will want this in before merging the walletdb integration. Can you rebase on top of the intwaddrmgr branch?

Also if possible, can you switch the PR from master to intwaddrmgr so it only includes new commits not already on intwaddrmgr? (I don't know if github allows this)

davecgh and others added 9 commits February 5, 2015 18:10
This commit converts the wallet to use the new secure hierarchical
deterministic wallet address manager package as well as the walletdb
package.

The following is an overview of modified functionality:

- The wallet must now be created before starting the executable
- A new flag --create has been added to create the new wallet using wizard
  style question and answer prompts
- Starting the process without an existing wallet will instruct now
  display a message to run it with --create
- Providing the --create flag with an existing wallet will simply show an
  error and return
Previously a runtime.GC was being invoked which forced it to release the
memory as far as the garbage collector is concerned, but the memory was
not released back to the OS immediatley.  This modification allows the
memory to be released immedately since it won't be needed again until the
next wallet unlock.
@tuxcanfly
Copy link
Contributor Author

Rebased on top of intwaddrmgr. Looks like we'll need to create a new PR with the base branch as intwaddrmgr instead of master.

@jrick
Copy link
Member

jrick commented Feb 6, 2015

Okay, it's a pain but I think that's the best solution. You can close this one and reference it from the new one.

@tuxcanfly
Copy link
Contributor Author

Done, closing in favor of #190

@tuxcanfly tuxcanfly closed this Feb 6, 2015
jrick pushed a commit to jrick/btcwallet that referenced this pull request Nov 15, 2016
Prevent pasting in backup seed textbox
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.

3 participants