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

Integrate wtxmgr #234

Merged
merged 5 commits into from
Apr 28, 2015
Merged

Integrate wtxmgr #234

merged 5 commits into from
Apr 28, 2015

Conversation

jrick
Copy link
Member

@jrick jrick commented Apr 6, 2015

Depends on #217. merged

This contains the changes to integrate wtxmgr with the rest of the wallet code and removes the legacy txstore package.

@jrick jrick mentioned this pull request Apr 6, 2015
4 tasks
@jrick jrick force-pushed the jrick_intwtxmgr branch 2 times, most recently from ab4a4c3 to 9f2a560 Compare April 8, 2015 14:46
@jrick jrick force-pushed the jrick_intwtxmgr branch 4 times, most recently from 0565215 to 6fe92dc Compare April 13, 2015 13:26
@jrick
Copy link
Member Author

jrick commented Apr 13, 2015

I'm going to add an extra utility that can be used to drop the wtxmgr namespace. This will allow us to force rescans. I'm hesitant to make this a feature of the rpc server, since this step really should never be necessary (but good idea to add it just in case).

@jrick jrick force-pushed the jrick_intwtxmgr branch 7 times, most recently from 21592f7 to 48b93b6 Compare April 13, 2015 18:21
// height comes before the begin height, blocks are iterated in reverse order
// and unmined transactions (if any) are processed first.
//
// The function f may return an error which, if non-nil, is propigated to the
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: propigated

@jrick jrick force-pushed the jrick_intwtxmgr branch 2 times, most recently from 1dde322 to 7904b21 Compare April 17, 2015 13:12
@jrick
Copy link
Member Author

jrick commented Apr 17, 2015

Rebased over hashing API changes.

This was referenced Apr 18, 2015
@jrick jrick force-pushed the jrick_intwtxmgr branch 3 times, most recently from 48d20d6 to 329c16d Compare April 20, 2015 22:20
@davecgh
Copy link
Member

davecgh commented Apr 25, 2015

Since the new utility isn't using the same flags package as wallet and everything else, it's inconsistent. I think it's important from a usability aspect for everything in the suite to be consistent. Can you change the util over to use go-flags?

@jrick jrick force-pushed the jrick_intwtxmgr branch from 329c16d to bcdc3c9 Compare April 25, 2015 04:48
two situations when this holds true:

1. The wallet is newly created or was recreated from the seed

Copy link
Member

Choose a reason for hiding this comment

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

I believe markdown formats lists better if there is no newline between the list items.

Copy link
Member Author

Choose a reason for hiding this comment

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

@davecgh
Copy link
Member

davecgh commented Apr 25, 2015

Guide about dropping transactions is a very nice addition.

@jrick
Copy link
Member Author

jrick commented Apr 25, 2015

I can convert the flags over. I just used the stdlib because this is a dev tool that won't see much use, and the entire flags package API fits in my head.

@@ -52,7 +52,7 @@ var (
var subsystemLoggers = map[string]btclog.Logger{
"BTCW": log,
"WLLT": walletLog,
"TXST": txstLog,
"TXST": txmgrLog,
Copy link
Member

Choose a reason for hiding this comment

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

Not a big deal, but probably should make the prefix match the name. If it's going to be called tx manager now, ideally it would be referred to that throughout. How about "TMGR" or "TXMR"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I like TMGR.

@davecgh
Copy link
Member

davecgh commented Apr 28, 2015

Everything else reads correctly. I'm going to spend some time testing it, but I'm done with the code review.

@jrick jrick force-pushed the jrick_intwtxmgr branch from 18e53cd to dd5ce1f Compare April 28, 2015 02:41
@davecgh
Copy link
Member

davecgh commented Apr 28, 2015

Doing some testing and noticed this doesn't remove the old tx.bin on update.

@jrick
Copy link
Member Author

jrick commented Apr 28, 2015

No harm in leaving it around, and it can always be manually deleted. I'd rather avoid checking for an ancient file each and every startup for the rest of forever to clean up something that maybe a handful of people might have.

@davecgh
Copy link
Member

davecgh commented Apr 28, 2015

Alright, I've tested this pretty well. I've also confirmed the latest commit fixed the issue reported in IRC. I also tested the dropwtxmgr tool and it works. As mentioned on IRC, the tool isn't really consistent with the flags and such, and that could be improved, but I don't see that as a reason to hold up this PR.

OK.

@jrick jrick force-pushed the jrick_intwtxmgr branch from 09ce1be to 09c391c Compare April 28, 2015 21:28
@conformal-deploy conformal-deploy merged commit 09c391c into btcsuite:master Apr 28, 2015
@jrick jrick deleted the jrick_intwtxmgr branch April 28, 2015 21:56
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.

5 participants