Skip to content

Conversation

jnewbery
Copy link
Contributor

Adds salvage and zaptxs commands to bitcoin-wallet

@jnewbery
Copy link
Contributor Author

As suggested here: #13926 (comment), the initial bitcoin-wallet PR stripped down the wallet tool to minimum functionality. This PR adds the salvage and zaptxs commands back in.

I haven't made any changes from the original PR, so @Sjors comment here: #13926 (comment) still isn't addressed. I plan to do that in future, but wanted to have a PR open in case others wanted to provide early concept feedback.

@Sjors
Copy link
Member

Sjors commented Feb 1, 2019

Concept ACK. I suggest marking them deprecated in bitcoind at the same time.

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 6, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15358 (util: Add SetupHelpOptions() by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@jonasschnelli
Copy link
Contributor

Concept ACK.
Is there a reason why this duplicates the functionality rather than directly move it?

@ryanofsky
Copy link
Contributor

ryanofsky commented Feb 7, 2019

Curious why this is marked WIP. Would be good to clarify if you are still working on changes or are presently seeking review.

Is there a reason why this duplicates the functionality rather than directly move it?

I don't think it should hold up this PR, but I do have the same question, and personally would love to see a PR getting rid of the current duplication between CreateWallet and createwallet, LoadWallet and loadwallet.

I think it would be good to have a plan or at least a basic idea going forward of how to avoid duplicating functionality between the wallet and wallet tool. Making the wallet tool into a limited frontend for a subset RPC methods that work offline (suggested in #13926 (comment)) would be a good approach that would enable code sharing, and also decrease the likelihood of a garbagey command line parsing code getting added to wallet tool in the future. But other approaches are certainly possible.

@maflcko
Copy link
Member

maflcko commented Feb 7, 2019

This is critical functionality and I think unless there is at least a basic test, this is not ready for review.

@jnewbery
Copy link
Contributor Author

jnewbery commented Feb 7, 2019

Curious why this is marked WIP.

Sorry, a better tag would be work-not-in-progress. I'm not actively working on this and plan to pick it up later. I wanted to open the PR so people could give concept feedback. I'll also mark as 'Up For Grabs' if someone else wants to grab it with higher priority.

This isn't ready for merge for a couple of reasons:

I definitely agree with your concerns about code duplication and like your idea #13926 (comment).

@DrahtBot
Copy link
Contributor

Needs rebase

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.

8 participants