Improve the README #566

tcharding · 2022-12-16T00:14:26Z

Improve the README files

Improve the secp256k1 readme by:

~~Use a top level markdown header (level 1)~~ Use HTML for header and badges
Add a link to the SECG's website (www.secg.org)
Add a link for secp256k1 to bitcoin.it explaining the curve

Improve the secp256k1-sys readme by:

Use HTML for header and badges (a subset of the badges used in rust-secp256k1 readme)
Basic cleanup
- Use 100 column width
- Use backticks
- Use capitals

tcharding · 2022-12-16T00:52:07Z

Needs #565 to get green CI.

Kixunil · 2022-12-16T17:11:51Z

README.md

 `rust-secp256k1` is a wrapper around [libsecp256k1](https://github.com/bitcoin-core/secp256k1),
-a C library by Pieter Wuille for producing ECDSA signatures using the SECG curve
-`secp256k1`. This library
+a C library by Pieter Wuille for producing ECDSA signatures using the [SECG](https://www.secg.org/)


"for producing ECDSA signatures" sounds weird since it can also verify them and also produce/verify schnorr signatures and also perform ECDH and also tweak the keys and probably something I forgot.

"a C library implementing various cryptographic functions using the SECG curve secp256k1" might be good.

Used as suggested, note this removes Peter's name - this seems correct since it does not appear in the readme of libsecp256k1 (I thought I remembered seeing it there before but not sure).

cc @sipa in case he would like the attribution to remain.

We could say "created by Pieter Wuille". But I agree that we should follow the README, which presumably attributes it to "the bitcoin core developers" or something.

Oh yeah, I didn't mean to drop attribution but it's a good question which attribution is the correct one.

Kixunil · 2022-12-16T17:37:01Z

secp256k1-sys/README.md

-`--cfg=rust_secp_no_symbol_renaming'` to your `RUSTFLAGS` variable.
+If you want to compile this library without using the bundled symbols (which may be required for
+integration into other build systems), you can do so by adding `--cfg=rust_secp_no_symbol_renaming'`
+to your `RUSTFLAGS` variable.



Maybe we could add the information about the limitations of this @apoelstra mentioned while we're at it. I think this should be correct. I suggest putting it right after the heading.

Danger: doing this incorrectly may have catastrophic consequences!
This is mainly intended for applications consisting of various programming languages that intend to link the same library to save space or bundles of multiple binaries coming from the same source.
Do not use this to link to a random secp256k1 library you found in your OS!
Packaging the library into a separate distribution package could be done with care.
E.g. in Debian if you'd have libsecp256k1 with version 1.2.3 then all the binaries using it must use >= 1.2.3, << 1.2.4, assuming you will only ever release binary-compatible versions of the package.
This is because there are no binary compatibility guarantees and the library is security-critical.
Note also that unless you're packaging the library for an official repository you should prefix your package and the library with a string specific to you. E.g. if you have a set of packages called my-awesome-packages you should package libsecp256k1 as libmy-awesome-packages-secp256k1 and depend on that library/package name from your application.
If you do any of this you must make sure that either every component that depends on libsecp256k is written to depend on the same version (-sys crate not being duplicated in case of Rust) or that the differing components use static bundled version with the different prefixes.

E.g. in Debian if you'd have libsecp256k1 with version 1.2.3 then all the binaries using it must use >= 1.2.3, << 1.2.4, assuming you will only ever release binary-compatible versions of the package. This is because there are no binary compatibility guarantees and the library is security-critical.

I don't understand this sentence -- there is only one release of libsecp in existence and it is not in any distribution AFAIK. What distributions have actually packaged is random shit. I'm also not sure why, "if there are no binary compatibility guarantees", merely constraining a version number would provide any value.

I'm also not totally sure who "you" is here -- is in the author of the Rust code, or a distribution maintainer, or a libsecp developer, or what?

Other than this, this text looks good.

and it is not in any distribution AFAIK

Indeed, this is written in case someone wants to package it. But now I'm thinking it may overlooked anyway. I guess some doc like that should go to the upstream library instead.

I'm also not sure why, "if there are no binary compatibility guarantees", merely constraining a version number would provide any value.

If you control all the packages and make sure you won't release version 1.2.3-2 that is binary-incompatible with 1.2.3-1 then it's OK to update the library. (e.g. if you only made cosmetic changes like documentation or fixed something minor inside a function body)

Basically, if you control the library (package) and make sure it doesn't get confused/swapped with something else then it's OK to link to it from a binary you control.

I'm also not totally sure who "you" is here

The author of the binary using libsecp256k1 and potentially distribution maintainer. (I think they are sometimes the same person? I do make my own packages for some of my own projects but that doesn't make me distribution maintainer...)

How about If you are packaging software that depends on rust-secp256k1, using this flag to link to another package, make sure you stay within the binary compatibility guarantees of that package. For example, in Debian if you need libsecp256k1 1.2.3, make sure your package requires a version strictly >= 1.2.3 << 1,2.4.

Kixunil · 2022-12-16T17:41:31Z

secp256k1-sys/README.md

-This crate provides Rust definitions for the FFI structures and methods.
+[Full documentation](https://docs.rs/secp256k1_sys/)
+
+This is the Foreign Function Interface (FFI) to the [libsecp256k1](https://github.com/bitcoin-core/secp256k1) library.


This sounds a bit strange the library is not FFI. The library just provides low-level bindings to the C FFI exposed by libsecp256k1

Kixunil · 2022-12-16T17:42:32Z

secp256k1-sys/README.md

+[Full documentation](https://docs.rs/secp256k1_sys/)
+
+This is the Foreign Function Interface (FFI) to the [libsecp256k1](https://github.com/bitcoin-core/secp256k1) library.
+Provides bindings (structures and methods) in Rust for interfacing with `libsecp256k1` in a typesafe manner.


The -sys crate is not type safe.

We could drop the "in a typesafe manner" clause, but in fairness, the bindings are typesafe in that they enforce that you use the correct C types, correct number of arguments to functions, etc.

you use the correct C types

That's questionable at best. If you happen to be on a platform that defines c_int as i32 and you accidentally hard-code i32 in the code your code suddenly becomes non-portable to platforms that have c_int be a different type (presumably i16 or i64.

They are "sort of safe" yes.

tcharding · 2022-12-20T00:09:02Z

Force push attempts to implement suggestions, will require close review please.

apoelstra · 2022-12-20T00:18:46Z

secp256k1-sys/README.md

+software that depends on `rust-secp256k1`, using this flag to link to another package, make sure you
+stay within the binary compatibility guarantees of that package. For example, in Debian if you need
+`libsecp256k1 1.2.3`, make sure your package requires a version strictly`>= 1.2.3 << 1.2.4`. This is
+because there are no binary compatibility guarantees and the library is security-critical. Note also


I think the This is because line doesn't make sense, and should be removed.

apoelstra · 2022-12-20T00:19:35Z

secp256k1-sys/README.md

+that unless you're packaging the library for an official repository you should prefix your package
+and the library with a string specific to you. E.g. if you have a set of packages called
+`my-awesome-packages` you should package `libsecp256k1` as `libmy-awesome-packages-secp256k1` and
+depend on that library/package name from your application. If you do any of this you must make sure


I also think we should drop "If you do any of this..." since it's redundant with the existing warnings and isn't very clear.

tcharding · 2022-12-20T01:07:49Z

Force pushed suggested changes.

apoelstra

ACK a13297a

apoelstra · 2022-12-20T18:31:50Z

Will let @Kixunil review the current text before merging.

tcharding · 2023-03-20T07:41:26Z

Friendly bump when you get a chance please @Kixunil.

tcharding · 2023-05-15T06:26:03Z

Rebase only.

apoelstra · 2023-05-15T13:48:19Z

secp256k1-sys/README.md

-
+- Where `<version-code>` is the secp256k1-sys version number underscored: `0_1_2`.
+- Where `<rev>` is the git revision of `libsecp256k1` to checkout. If you do not specify a revision,
+  the script will simply clone the repo and use whatever revision the default branch is pointing to.


In 04f2c24:

The new text is now different (and incorrect) where before I think it was just a reflow of the old text.

tcharding · 2023-05-15T22:38:50Z

GitHub actions seem to be borked

Only "Cross testing" is running
WASM test is broken

[Invalid workflow file: .github/workflows/rust.yml#L117](https://github.com/rust-bitcoin/rust-secp256k1/actions/runs/4938183432/workflow)
The workflow is not valid. .github/workflows/rust.yml (Line: 117, Col: 15): Unrecognized named-value: 'matrix'. Located at position 1 within expression: matrix.toolchain

Improve the secp256k1 readme by: - Use a top level markdown header (level 1) - Add a link to the SECG's website (www.secg.org) - Add a link for `secp256k1` to bitcoin.it explaining the curve Improve the secp256k1-sys readme by: - Mirror secp256k1 readme badges, heading, docs link - Basic cleanup - Use 100 column width - Use backticks - Use capitals

tcharding · 2023-05-15T22:52:33Z

Changes in force push:

Fixed rebase issue flagged during review
Added HTML (incl. badges) as we do in rust-bitcoin (this uncovered the fact that CI is broken at the moment, WIN!)

tcharding · 2023-05-15T22:53:31Z

For HTML rendering of the two readme files see

apoelstra

ACK 47aa740

apoelstra · 2023-05-16T16:40:07Z

Oops, I forgot we were gating this one on @Kixunil. But I think the text is definitely an improvement over the existing text in any case.

Kixunil · 2023-05-16T20:40:13Z

No problem, looks fine.

tcharding force-pushed the 12-16-readme branch from b0a4f54 to 2d3c143 Compare December 16, 2022 00:30

Kixunil reviewed Dec 16, 2022

View reviewed changes

tcharding force-pushed the 12-16-readme branch 2 times, most recently from 0b3e9cb to 791def3 Compare December 20, 2022 00:08

apoelstra reviewed Dec 20, 2022

View reviewed changes

tcharding force-pushed the 12-16-readme branch from 791def3 to a13297a Compare December 20, 2022 01:07

apoelstra previously approved these changes Dec 20, 2022

View reviewed changes

Kixunil mentioned this pull request Feb 6, 2023

Support latest Debian libsecp256k1 package spesmilo/electrum#8185

Merged

tcharding dismissed apoelstra’s stale review via 04f2c24 May 15, 2023 06:24

tcharding force-pushed the 12-16-readme branch from a13297a to 04f2c24 Compare May 15, 2023 06:24

apoelstra reviewed May 15, 2023

View reviewed changes

tcharding force-pushed the 12-16-readme branch from 04f2c24 to f19b69f Compare May 15, 2023 22:31

tcharding force-pushed the 12-16-readme branch 3 times, most recently from c195600 to b7e667d Compare May 15, 2023 22:44

tcharding force-pushed the 12-16-readme branch from b7e667d to 47aa740 Compare May 15, 2023 22:49

apoelstra approved these changes May 16, 2023

View reviewed changes

apoelstra merged commit 7c8270a into rust-bitcoin:master May 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the README #566

Improve the README #566

tcharding commented Dec 16, 2022 •

edited

Loading

tcharding commented Dec 16, 2022

Kixunil Dec 16, 2022

tcharding Dec 19, 2022

apoelstra Dec 20, 2022

Kixunil Dec 20, 2022

Kixunil Dec 16, 2022

apoelstra Dec 17, 2022

Kixunil Dec 17, 2022

apoelstra Dec 17, 2022 •

edited

Loading

Kixunil Dec 17, 2022

Kixunil Dec 16, 2022

Kixunil Dec 16, 2022

apoelstra Dec 19, 2022

Kixunil Dec 19, 2022

tcharding commented Dec 20, 2022

apoelstra Dec 20, 2022

apoelstra Dec 20, 2022

tcharding commented Dec 20, 2022

apoelstra left a comment

apoelstra commented Dec 20, 2022

tcharding commented Mar 20, 2023

tcharding commented May 15, 2023

apoelstra May 15, 2023

tcharding commented May 15, 2023

tcharding commented May 15, 2023

tcharding commented May 15, 2023

apoelstra left a comment

apoelstra commented May 16, 2023

Kixunil commented May 16, 2023

Improve the README #566

Improve the README #566

Conversation

tcharding commented Dec 16, 2022 • edited Loading

tcharding commented Dec 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra Dec 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Dec 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Dec 20, 2022

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Dec 20, 2022

tcharding commented Mar 20, 2023

tcharding commented May 15, 2023

Choose a reason for hiding this comment

tcharding commented May 15, 2023

tcharding commented May 15, 2023

tcharding commented May 15, 2023

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented May 16, 2023

Kixunil commented May 16, 2023

tcharding commented Dec 16, 2022 •

edited

Loading

apoelstra Dec 17, 2022 •

edited

Loading