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

[CI test] Integrate secp256k1lab as subtree #83

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

theStack
Copy link
Contributor

@theStack theStack commented Feb 19, 2025

This PR moves the secp256k1lab (formerly called secp256k1proto) folder to an external package (living at https://github.com/theStack/secp256k1lab currently, created with the uv package manager), integrating it as git-subtree here [1]:

$ git subtree add --prefix=python/secp256k1lab --squash https://github.com/theStack/secp256k1lab secp256k1lab-draft02

Supersedes #81, using the suggested syspath extension approach [1] [2] [3] to avoid the ugly import hack that was used before.

[1] #81 (comment)
[2] #81 (comment)
[3] https://github.com/BlockstreamResearch/bip-frost-dkg/tree/syspath

Copy link
Collaborator

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

This is looking good. Added some comments.

@theStack theStack force-pushed the separate_secp256k1lab branch from 4d38d98 to 3c03e67 Compare February 20, 2025 15:18
@theStack
Copy link
Contributor Author

Rebased on master and addressed the review comments above (#83 (comment), #83 (comment), #83 (comment)). I btw thought it might be nice to squash the first three commits into one, but somehow always the rename of the vendored package (src/secp256k1lab => python/secp256k1lab/src/secp256k1lab, apparently happening in the merge commit) gets lost 🤔

Comment on lines 7 to 8
# Prefer the vendored copy of secp256k1lab.
sys.path.insert(0, os.path.join(os.path.dirname(__file__), "../secp256k1lab/src"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit more modern Python and also a bit more readable IMO:

Suggested change
# Prefer the vendored copy of secp256k1lab.
sys.path.insert(0, os.path.join(os.path.dirname(__file__), "../secp256k1lab/src"))
# Prefer the vendored copy of secp256k1lab.
sys.path.insert(0, Path(__file__).parent / "../secp256k1lab/src")

(needs from pathlib import Path)

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. Note that I had to convert the resulting Path object (PosixPath on my machine) to a string though (str(...)), as otherwise it wouldn't work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, indeed... Well, fine, I guess it's still a tiny bit more readable than the os.path variants.

Comment on lines 6 to 16
authors = [
{ name = "Tim Ruffing", email = "[email protected]" },
{ name = "Jonas Nick", email = "[email protected]" },
{ name = "Sebastian Falbesoner", email = "[email protected]" }
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it will be fair to list @sipa as an author:

Suggested change
authors = [
{ name = "Tim Ruffing", email = "[email protected]" },
{ name = "Jonas Nick", email = "[email protected]" },
{ name = "Sebastian Falbesoner", email = "[email protected]" }
]
authors = [
{ name = "Pieter Wuille", email = "[email protected]" },
{ name = "Tim Ruffing", email = "[email protected]" },
{ name = "Jonas Nick", email = "[email protected]" },
{ name = "Sebastian Falbesoner", email = "[email protected]" }
]
maintainers = [
{ name = "Tim Ruffing", email = "[email protected]" },
{ name = "Jonas Nick", email = "[email protected]" },
{ name = "Sebastian Falbesoner", email = "[email protected]" }
]

based on:
https://github.com/bitcoin/bitcoin/commits/e486597f9a57903600656fb5106858941885852f/test/functional/test_framework/crypto/secp256k1.py

Copy link
Collaborator

Choose a reason for hiding this comment

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

You've added authors but not maintainers. Was this intentional? (I also don't like the duplication too much, but well, it's correct...)

@@ -0,0 +1,16 @@
[project]
name = "secp256k1lab"
version = "0.1.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can safely make this "production", otherwise it'll be in 0.x.x land forever.

Perhaps a date-based versioning scheme like YYYY.M.D is better? I'd be hesitant to use MAJOR.MINOR.REVISON as people then think "semantic versioning" and I don't think we want to make any guarantees. This a primarily a playground. If someone wants to use it, they can vendor/pin the commit?

@jonasnick @theStack What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think semantic versioning is a good choice in our case. I don't really see the problem with trying to make guarantees. We can just be very liberal about increasing the major version. Even if users vendor or pin the commit, it's very helpful to see very quickly what has changed in the library.

I agree that starting with 1.0.0 makes sense here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fine with me. Related question: Do we want a changelog?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that'd be helpful (at least once we have actual changes).

Are there any changes we already know we want to do before 1.0.0?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd keep it minimal:

anything else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any opinion about the "[build-system]" section? "hatchling" is what "uv init" created by default, but I don't have any knowledge about what makes sense to use (or if it even matters that much, considering we have no dependencies).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I won't really matter for us, but hatchling is totally fine and the uv devs choose it for a reason. astral-sh/uv#7132. Fwiw, uv will soon have its own build backend. I'd just keep this as is.

@real-or-random
Copy link
Collaborator

I btw thought it might be nice to squash the first three commits into one, but somehow always the rename of the vendored package (src/secp256k1lab => python/secp256k1lab/src/secp256k1lab, apparently happening in the merge commit) gets lost 🤔

I think I've seen this before. Just a shot in the dark but perhaps this helps: https://stackoverflow.com/questions/30136558/how-to-squash-commits-which-have-merge-commit-in-between

@theStack theStack force-pushed the separate_secp256k1lab branch from 3c03e67 to def1dfc Compare February 25, 2025 19:26
@theStack
Copy link
Contributor Author

Added the secp256k1lab suggestions in branch https://github.com/theStack/secp256k1lab/tree/secp256k1lab-draft02 and updated the subtree integration in this PR accordingly. Still a bunch of TODOs (#83 (comment), plus determining a reasonable required Python version), I hope to tackle them this week. I guess with the ruff and mypy integration I can pretty much reuse the CI config here with minor adaptions, or is there anything special to be considered if it's a (uv) package?

@real-or-random
Copy link
Collaborator

I guess with the ruff and mypy integration I can pretty much reuse the CI config here with minor adaptions, or is there anything special to be considered if it's a (uv) package?

This is not strictly necessary, but since we use uv anyway: we won't need pip install foo, and we can just use uvx foo (see uv help tool).

plus determining a reasonable required Python version)

Something like uv run --python 3.9 should do it. But you'll need to execute the BIP code: uv run --python 3.9 python/tests.sh (or something like this, I haven't tested.) The thing is that we can't really run the library, and there are no tests, so the best test we have is running the BIP code...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants