-
Notifications
You must be signed in to change notification settings - Fork 37.7k
[WIP] [tool] Add salvage and zaptxs commands to bitcoin-wallet #15307
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
Conversation
As suggested here: #13926 (comment), the initial bitcoin-wallet PR stripped down the wallet tool to minimum functionality. This PR adds the 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 |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Curious why this is marked WIP. Would be good to clarify if you are still working on changes or are presently seeking review.
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 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. |
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). |
Needs rebase |
Adds
salvage
andzaptxs
commands to bitcoin-wallet