[WIP] [tool] Add salvage and zaptxs commands to bitcoin-wallet #15307

Adds salvage and zaptxs commands to bitcoin-wallet

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.

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

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

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.

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

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.

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

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:

• needs updating to address feedback here #13926 (comment)
• needs tests

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

<!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase