Details

Reviewers

deadalnix
markblundeberg
jasonbcox

Group Reviewers

Restricted Project

Commits

rSTAGING1f824752ca59: Update secp256k1 README
rABC1f824752ca59: Update secp256k1 README

Summary

Now that secp256k1 is being bundled as it's own repo on Github,
it would be nice to have a more accurate and complete README.

Test Plan

Read it, make sure it's complete and accurate.

Diff Detail

Repository

rABC Bitcoin ABC

Branch

libsecp256k1

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 8720
Build 15425: Default Diff Build & Tests
Build 15424: arc lint + arc unit

Event Timeline

Mengerian created this revision.Jan 2 2020, 21:07

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 2 2020, 21:07

Build Bitcoin-ABC / Diffs / Diff Testing started.

@markblundeberg It may also be good to add some Schnorr information to the "Implementation Details" portion, and maybe a link to the BCH Schnorr spec on bitcoincash.org. Please let me know if you have any suggestions.

src/secp256k1/README.md
4 ↗	(On Diff #15113)	@jasonbcox Is there some link we could use for the equivalent information for Bitcoin ABC's automated builds?

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8710: Diff 15113.Jan 2 2020, 21:16

jasonbcox added inline comments.Jan 2 2020, 21:25

src/secp256k1/README.md
4 ↗	(On Diff #15113)	It's not a perfect comparison, but our default master CI config builds and tests libsecp256k1 in addition to bitcoind: https://build.bitcoinabc.org/viewType.html?buildTypeId=BitcoinABCMasterLinux If this build is green, so is libsecp256k1. That said, it would be pretty easy to move it to a stand-alone build (see `contrib/teamcity/build-configurations.sh`)

@jasonbcox, @Mengerian FYI I plan to update the current build-default by splitting it into 3: build-diff, build-master and build-secp256k1, the later will cover the standalone build and all the unit tests.

Fabien added inline comments.Jan 2 2020, 22:09

src/secp256k1/README.md
8 ↗	(On Diff #15113)	Is there any possible legal consequence with this removal ?

deadalnix added inline comments.Jan 3 2020, 03:37

src/secp256k1/README.md
4 ↗	(On Diff #15113)	Or we can just use travis, the adaptation from core's shouldn't be very difficult.

Add a bit more explanation of how the repository is maintained, so that people can understand what's going on.
Re-add "Use at your own risk" disclaimer as recommended by @Fabien
Add link and mention that this is the Bitcoin Cash variant of the Schnorr algorithm.

src/secp256k1/README.md
8 ↗	(On Diff #15113)	I doubt it's an issue, but I can re-add the "Use at your own risk" statement. I will also add a bit more explanation of how the repository is maintained, so that people can understand what's going on.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8720: Diff 15131.Jan 3 2020, 04:46

deadalnix added inline comments.Jan 3 2020, 05:45

src/secp256k1/README.md
4	Is travis actually broken for us? If not, then why not use it?

Mengerian added inline comments.Jan 3 2020, 06:01

src/secp256k1/README.md
4	IDK, I don't have sufficient privileges in the Bitcoin ABC Github to enable Travis. Just by clicking around, it looks doable, so that's probably a good idea to enable it. If someone wants to give me the needed Github privileges I can try to figure it out, or alternatively someone familiar with Travis can do it.

deadalnix added inline comments.Jan 4 2020, 06:53

src/secp256k1/README.md
4	I granted you the right to the github repo so you can do it. Getting the cmake build to also be tested would be a plus. I think @Fabien can help here.