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

1. 
   jnewbery commented at 10:33 pm on January 31, 2019: member

   Adds salvage and zaptxs commands to bitcoin-wallet
2. [tool] Add salvage and zaptxs commands to bitcoin-wallet 2aa3111082
3. 
   jnewbery commented at 10:36 pm on January 31, 2019: member

   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.

4. 
   Sjors commented at 10:23 am on February 1, 2019: member

   Concept ACK. I suggest marking them deprecated in bitcoind at the same time.
5. 
   DrahtBot commented at 8:53 pm on February 6, 2019: member

   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.

6. 
   jonasschnelli commented at 6:27 am on February 7, 2019: contributor

   Concept ACK. Is there a reason why this duplicates the functionality rather than directly move it?
7. 
   ryanofsky commented at 3:50 pm on February 7, 2019: member

   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.

8. 
   MarcoFalke commented at 4:09 pm on February 7, 2019: member

   This is critical functionality and I think unless there is at least a basic test, this is not ready for review.
9. 
   jnewbery commented at 9:26 pm on February 7, 2019: member

   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).

10. jnewbery added the label Up for grabs on Feb 7, 2019
11. 
    DrahtBot commented at 3:12 pm on February 12, 2019: member
12. DrahtBot added the label Needs rebase on Feb 12, 2019
13. fanquake closed this on Jun 19, 2019

    ---

14. laanwj removed the label Needs rebase on Oct 24, 2019
15. fanquake removed the label Up for grabs on May 22, 2020
16. DrahtBot locked this on Feb 15, 2022

Contributors
jnewbery Sjors DrahtBot jonasschnelli ryanofsky MarcoFalke