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

sighash module name is confusing #1463

Closed
Kixunil opened this issue Dec 12, 2022 · 16 comments · Fixed by #1624
Closed

sighash module name is confusing #1463

Kixunil opened this issue Dec 12, 2022 · 16 comments · Fixed by #1624
Labels
1.0 Issues and PRs required or helping to stabilize the API API break This PR requires a version bump for the next release brainstorm
Milestone

Comments

@Kixunil
Copy link
Collaborator

Kixunil commented Dec 12, 2022

AFAIK the term sighash usually refers to the SIGHASH_x flags. However the sighash module provides tools for actually signing the transactions. (I vaguely remember getting confused about this.)

Alternative names:

  • sign
  • tx_sign/sign_tx
  • signing
  • horribly long variants such as transaction_signing

I think I like sign_tx the most, feel free to suggest others.

Also *SighashType and Annex belong to something like primitives.

@Kixunil Kixunil added API break This PR requires a version bump for the next release brainstorm 1.0 Issues and PRs required or helping to stabilize the API labels Dec 12, 2022
@Kixunil Kixunil added this to the 0.30.0 milestone Dec 12, 2022
@apoelstra
Copy link
Member

IME sighash refers to the actual signature hash while "sighash flag" refers to the flags (which are closely related to the sighash).

Is there anything in this module that is not sighash-related? I did a quick skim and it seems not.

I also don't see any actual signing code so I don't think it makes sense to call it something like signing.

@sanket1729
Copy link
Member

Perhaps, I am also used to sighash name here. I would be okay with sighash_msg, sighash_message or sig_msg (BIP 341 term)

@Kixunil
Copy link
Collaborator Author

Kixunil commented Dec 13, 2022

Hmm, maybe I'm biased by having less coding experience than doc-reading in this area. sig_msg sounds quite good. (or even without underscore).

@apoelstra
Copy link
Member

That sounds like it's related to signmessage, or something ... it's definitely not clear that it's referring to a sighash.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Dec 13, 2022

Oh, true. Is there anything particularly wrong with sign_tx?

@apoelstra
Copy link
Member

This module is only tangentially related to signing. It's about computing and caching sighashes.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Dec 13, 2022

That's like 90% of all work one has to do when signing Bitcoin transactions. The only other parts are constructing tap tree (if any), calling one function from secp256k1 and shoving the result into a Witness

What's confusing about it is it's not obvious that "you need this module to sign transactions". Maybe just change the doc summary to something like "Infrastructure for signing Bitcoin transactions"?

@apoelstra
Copy link
Member

If it's 90% of the work then we should definitely label it something discoverable, which "sighash" is. "Infrastructure for signing Bitcoin transactions" does not make it obvious at all what's inside, and kinda sounds like it might be a collection of ad-hoc accoutrements that a normal users might not need, since normally you just compute a sighash and sign it.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Dec 13, 2022

I guess that depends on the background. Someone coming from Bitcoin Core would probably look for sighash and seeing that module name would make it obvious. Someone completely new would search for "transaction signing" or "sign transaction" and would see that. Having both "signature hash" an "transaction signing" in the doc doesn't sound bad if we can come up with reasonable wording.

@apoelstra
Copy link
Member

For sure -- I'm happy to extend the documentation to describe what a sighash is (and maybe disambiguate between the sighash and a sighash flag), and to give the high-level "compute a sighash, sign it, stick the signature into the transaction/PSBT" workflow. I think it'll take more than a sentence or two.

We could also put an example of signing a transaction in the top-level lib.rs, which will use this module, hopefully hinting new users where they need to look.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Dec 13, 2022

put an example of signing a transaction in the top-level lib.rs

I'd love to have something like a cookbook:

  • This is how you receive data over P2P
  • This is how you parse blocks and transactions
  • This is how you construct and sign transactions
  • ...

This should be well-visible at front page (README, doc) and link to separated doc. Otherwise the front page could become cluttered with things.

@tcharding
Copy link
Member

If #1597 merges the sighash module will have been moved to crypto::sighash and will only contain the sighash cache. Does that satisfy this issue for now? As part of #1606 I plan on writing a bunch of documentation on what all the various sighashes are.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 3, 2023

It's basically a documentation issue now, I will try to come up with a simple PR.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 6, 2023

Bad news: I just found out the generated docs don't have full text search - they can only search in names. I think the cookbook is the only way.

Kixunil added a commit to Kixunil/rust-bitcoin that referenced this issue Feb 6, 2023
"Sighash" is a technical term that newbies in Bitcoin may not know and
could get lost when trying to find how to sign a transaction. This
change attempts to make it more obvious that this module is needed for
signing.

Closes rust-bitcoin#1463
@tcharding
Copy link
Member

I think the cookbook is the only way.

If we get this done by BTC Prague we could have two tracks in the workshop, one for full enthusiasts (develop a wallet) and one for casually interested Rust devs (work through / look through the cookbook)?

@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 6, 2023

That could be interesting. But I wouldn't try too hard. I suggest:

  • One of us figures out a nice way of writing and publishing (I like what other Rust stuff, such as Rustnomicon, uses, not sure what it is.)
  • One of us sets up a repository with an intro page and some example page
  • Whenever anyone of us feels like adding a chapter, he makes a PR

apoelstra added a commit that referenced this issue Feb 7, 2023
75b266a Improve `sighash` module documentation (Martin Habovstiak)

Pull request description:

  "Sighash" is a technical term that newbies in Bitcoin may not know and could get lost when trying to find how to sign a transaction. This change attempts to make it more obvious that this module is needed for signing.

  Closes #1463

ACKs for top commit:
  tcharding:
    ACK 75b266a
  apoelstra:
    ACK 75b266a

Tree-SHA512: 7157566c1639c63ce0fba2832e8e5e846e689d89e24077ed7769b721c5db4613cd7fd8d91464992eb78de74b42912ca877e7182a9c3c9c8848bf94d89767b8cc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 Issues and PRs required or helping to stabilize the API API break This PR requires a version bump for the next release brainstorm
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants