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

doc: Add multisig tutorial #22822

Merged
merged 1 commit into from
Dec 14, 2021
Merged

Conversation

lsilva01
Copy link
Contributor

This PR adds a mutisig tutorial, as requested in #21278

Although there is already a brief explanation and a functional test about the multisig implemented in #22067, this tutorial proposes to use the signet (instead of regtest), bringing the reader closer to a real environment and explaining some functions in more detail.

I'm not sure if this format should be in this repository or on some wiki page. But as there is an open issue regarding this matter, that is my suggestion.

@fanquake fanquake added the Docs label Aug 28, 2021
@fanquake
Copy link
Member

Please add a link from the main README.

@lsilva01
Copy link
Contributor Author

@fanquake Done.

@fanquake fanquake linked an issue Aug 28, 2021 that may be closed by this pull request
@instagibbs
Copy link
Member

instagibbs commented Aug 28, 2021

@harding might be interested too

Unsure if here or some wiki page is best, but I can step through the tutorial in a bit and give feedback

doc/multisig-tutorial.md Outdated Show resolved Hide resolved
@Sjors
Copy link
Member

Sjors commented Aug 30, 2021

utACK 2693a97

This process is still quite tedious, but it's good to have it documented.

Copy link
Contributor

@mjdietzx mjdietzx left a comment

Choose a reason for hiding this comment

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

Concept ACK. Just commented on some minor type-os for now

doc/multisig-tutorial.md Outdated Show resolved Hide resolved
doc/multisig-tutorial.md Outdated Show resolved Hide resolved
doc/multisig-tutorial.md Outdated Show resolved Hide resolved
doc/multisig-tutorial.md Outdated Show resolved Hide resolved

Currently, it is possible to create a multig wallet using Bitcoin Core only.

Although there is already a brief explanation and a functional test about the multisig implemented in [PR 22067](https://github.com/bitcoin/bitcoin/pull/22067), this tutorial proposes to use the signet (instead of regtest), bringing the reader closer to a real environment and explaining some functions in more detail.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is regtest vs signet the main difference? I'd expect the guide to have the same steps regardless of the chain used.

What's the long-term plan for this guide and the guide from #22067? Do we plan to maintain both of them or keep only one?

Copy link
Member

Choose a reason for hiding this comment

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

The "Basic multisig example" in 22067 should probably link to this guide, but let's see what gets merged first.

@jaysonmald35
Copy link

Thank u

@Rspigler
Copy link
Contributor

Rspigler commented Sep 1, 2021

I don't quite understand the point of this, when we have #22067

22067 simply explains how to set up a multisig wallet using the GUI and RPC commands, why have another document with bash scripts? Other improvements can be added to the other PR

@lsilva01
Copy link
Contributor Author

lsilva01 commented Sep 2, 2021

The purpose of this PR is to add a tutorial (step by step) on multisig that explains some current functions and limitations in more detail.

Thus, the reader who doesn't understand the Test Framework or python will still be able to reproduce these steps on signet or even mainnet .

If this is a valid proposal, it can be a complement to PR #22067.

@jonatack
Copy link
Member

jonatack commented Sep 2, 2021

Concept ACK, provided people continue to maintain this and keep it up-to-date. Links between this and the docs + test in #22067 may be good to avoid information siloing.

@Sjors
Copy link
Member

Sjors commented Sep 2, 2021

Thus, the reader who doesn't understand the Test Framework or python will still be able to reproduce these steps on signet or even mainnet .

Conversely, the purpose of tests is to ensure we don't accidentally break functionality.

I agree it's useful if the toturial points to the test and vv, and we try to keep them somewhat consistent (although the test may continue to use earlier methods to ensure we don't break those as we change the recommended workflow for new users)

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

Concept ACK
I successfully followed the tutorial on Ubuntu 20.04 to create a multisig transaction over the signet network. Thank you very much for creating such a detailed guide.
I have some suggestions that might help to improve this tutorial further. This suggestion includes the occasional situation where I had some confusion following the tutorial or needed additional help from the internet.
Suggestions:

  • OP should mention .src/bitcoind -signet -daemon instead of .src/bitcoind -signet; otherwise, the terminal starts a non-finishing process which might off-put a new user. -daemon does the same job, just starts the process in the background.
  • Dependencies like jq that are required to run the code should be mentioned at starting of the file.
  • In the earlier parts of the tutorial, the instructions and reasoning precede the code. In contrast, in the later sections, after 1.4 Create the MultiSig Wallet, code precedes the instruction. This should be standardized, and the same style should be followed throughout the tutorial document.
  • Following line 126, OP should provide instruction to copy the receive_address. Something like: “To copy the receiving address onto the clipboard, use the following command.”
echo -n "$receiving_address" | xclip -sel clip
  • When a wallet is once created, and this process needs to be repeated, one has to load it instead of recreating wallets. This should be explicitly mentioned and explained in the tutorial file.
for ((n=1;n<=3;n++)); do  ./src/bitcoin-cli -signet loadwallet "participant_${n}"; done
./src/bitcoin-cli -signet loadwallet "multisig_wallet_01"

I hope these suggestions will be of help to the OP to improve upon the tutorial.

@michaelfolkson
Copy link
Contributor

michaelfolkson commented Sep 2, 2021

I don't quite understand the point of this, when we have #22067

22067 simply explains how to set up a multisig wallet using the GUI and RPC commands, why have another document with bash scripts? Other improvements can be added to the other PR

I agree with @Rspigler. I'd prefer this was a StackExchange post given that the process will likely repeatedly change in future and introduces maintenance burden. It looks good though, just quibbling on the best location for it given we want to avoid bloated, outdated docs in the repo in future.

edit: Feel free to put it here if you agree with above. Once the (optimal) process has crystallized I'd be more comfortable transferring it back as a stable doc to the repo.

I put up a StackExchange post by @ben-kaufman there on multisig backups which seems to me to be in a similar vein.

@lsilva01
Copy link
Contributor Author

lsilva01 commented Sep 8, 2021

Thanks for suggestions @shaavan. I added them.

@michaelfolkson I will add a short version of this tutorial to the StackExchange question you created.

I don't have a strong opinion whether this type of documentation is better to be in the repo or on another site.

The advantage of listing the procedures here is that it is extensively reviewed, so it is much more reliable and is part of the reference implementation.

The downside, as already mentioned, is the maintenance burden.

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

The document looks a lot better now. Great Work, @lsilva01!
Just a tiny thing to add. Since xclip is now being used to copy the address, it should also be mentioned as a dependency along with jq as it is not usually preinstalled in a Debian-based system.

edit: Feel free to put it here if you agree with the above. Once the (optimal) process has crystallized, I'd be more comfortable transferring it back as a stable doc to the repo.

I am no expert when it comes to maintenance, but I understand that it would be a burden to keep updating the doc as the process keeps changing. But posting this as a StackExchange answer doesn’t feel right to me. I think so because most people searching for the same question do not end up on the same StackExchange article, resulting in not many people being aware of this thorough documentation.

It would be great if we could figure out some middle ground where the documentation is visible to the audience who needs a multisig tutorial. But at the same time, not being a maintenance burden to the bitcoin in general.

@S3RK
Copy link
Contributor

S3RK commented Sep 16, 2021

I have a slight preference towards more concise guide from #22067.

But in case this will be merged sooner, I'll repeat my concern for both this PR and #22067

Why not have just one wallet? Having two wallets is less intuitive, complicates backups and could lead to user confusion and even loss of funds. Especially taking into account that you need to backup both wallets to restore your funds. This doesn't come through the documentation.

See my comment for suggestions

@ghost
Copy link

ghost commented Sep 16, 2021

I have a slight preference towards more concise guide from #22067.

whynotboth.gif

Although I like this tutorial. Other PR adds lot of things which can be useful for devs but docs from repository are also used by power users.

Concept ACK and thanks for your contribution @lsilva01

I want to use this tutorial and make some GUI or script to make it even easier for users before reviewing this PR in detail. Because multisig still feels like something only devs and few users can do.

https://unchained-capital.github.io/caravan/ is a nice web app although I don't have the skills to create such things, I will try doing a console application/desktop application/scripts. Also it would be better if we can avoid reference to open PRs in docs.

@Rspigler
Copy link
Contributor

@S3RK I think it is better to have the setup with 2 wallets, because that is how offline multisig should be run (1 wallet for the offline privkey/signing, another wallet for the online coordinator/node).

@ghost
Copy link

ghost commented Sep 21, 2021

I have also started working on a desktop application based on this tutorial. Hopefully will be completed by end of this week.

image

@Rspigler
Copy link
Contributor

@prayank23 Awesome!

Please see #21071 (comment) if you are working on a multisig GUI

done
```

### 1.2 Create the Descriptor Wallets
Copy link

Choose a reason for hiding this comment

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

Line 23 is ### 1.1 Create the Descriptor Wallets and this line ### 1.2 Create the Descriptor Wallets

Should combine them in one or change 1.2 to something else? I think we list descriptors in 1.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 1b58755

@laanwj
Copy link
Member

laanwj commented Oct 18, 2021

It would be great if we could figure out some middle ground where the documentation is visible to the audience who needs a multisig tutorial. But at the same time, not being a maintenance burden to the bitcoin in general.

I think documenting a basic multisig flow is important, as it is something a lot of people want to do nowadays. IMO it's best to regard it as basic functionality of the wallet not something exotic.

but I understand that it would be a burden to keep updating the doc as the process keeps changing

Now that PSBT's and descriptor wallets have been hammered out. The process shouldn't change too much anymore, right?

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 9, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #23711 (docs: RBF policy and mempool limit exemptions by glozow)

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.

@laanwj
Copy link
Member

laanwj commented Dec 8, 2021

Anything left to be done here?

@Sjors
Copy link
Member

Sjors commented Dec 9, 2021

utACK d92210e

Looking forward to making this easier over time. I think it's fine to update this document whenever that happens; it could even make review easier.

doc/multisig-tutorial.md Outdated Show resolved Hide resolved
@lsilva01 lsilva01 force-pushed the multisig_tutorial_doc branch 2 times, most recently from aee6bfa to 1b58755 Compare December 10, 2021 16:06
@Sjors
Copy link
Member

Sjors commented Dec 11, 2021

re-utACK 1b58755

doc/multisig-tutorial.md Outdated Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

ACK 1ef2c03

@Sjors
Copy link
Member

Sjors commented Dec 13, 2021

re-utACK 1ef2c03

@maflcko maflcko merged commit 87ce2d6 into bitcoin:master Dec 14, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 14, 2021
1ef2c03 Add multisig tutorial (lsilva01)

Pull request description:

  This PR adds a mutisig tutorial, as requested in bitcoin#21278

  Although there is already a brief explanation and a functional test about the multisig implemented in bitcoin#22067, this tutorial proposes to use the signet (instead of regtest), bringing the reader closer to a real environment and explaining some functions in more detail.

  I'm not sure if this format should be in this repository or on some wiki page. But as there is an open issue regarding this matter, that is my suggestion.

ACKs for top commit:
  Sjors:
    re-utACK 1ef2c03
  prayank23:
    ACK bitcoin@1ef2c03

Tree-SHA512: 2c3f17a8c50e554f802029dceb28ab90a77021f135b8cbd77dca3879ba1f1a0eac6bda0afb90d1ff6b8116fb0628471687d3fb77bb255ef5d8b9590b775cbce9
@lsilva01 lsilva01 deleted the multisig_tutorial_doc branch December 20, 2021 14:51
@bitcoin bitcoin locked and limited conversation to collaborators Jan 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wallet: Multi-sig flow with descriptor wallets