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

Create crypto submodules #2152

Closed
wants to merge 6 commits into from

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Oct 30, 2023

This PR is code move only.

Create two new submodules in the crypto module, ecdsa and sighash.

  • Move error code to a separate submodule
  • Move sighash types to a separate submude

All patches are code moves only (the last patch re-orders the code). The changes here were originally done in #1989, I redid the whole thing to make sure there were no rebase mistakes. Put up as a separate PR to ease review and qualify for the "Refactor carve-out".

@tcharding
Copy link
Member Author

This PR can be merged two weeks after getting a single ACK under the "Refactor carve-out".

@tcharding tcharding changed the title Crete crypto submodules Create crypto submodules Oct 30, 2023
@Kixunil
Copy link
Collaborator

Kixunil commented Nov 1, 2023

I'd prefer to get rid of crypto completely and put these things into the other modules. Especially after #2156 where the code organization looks weird.

@tcharding
Copy link
Member Author

I'm not exactly sure where we are up to with crate smashing but getting rid of crypto at this stage seems like going round in circles because we only just created it, supposedly as a step in crate smashing.

@apoelstra
Copy link
Member

@Kixunil where would you propose we put these things? It does seem like we're going in circles here.

The crypto module was introduced in #1260 for reference.

@tcharding
Copy link
Member Author

tcharding commented Nov 1, 2023

In case it helps, I've put all crate smashing tasks on hold except the next step because its hard to get anywhere, the next piece of the puzzle is bitcoin-io.

@Kixunil
Copy link
Collaborator

Kixunil commented Nov 2, 2023

At the very least move the contents of crypto::taproot into taproot. Consider flattening the rest. (Move the modules into src/)

@Kixunil Kixunil mentioned this pull request Nov 2, 2023
@apoelstra
Copy link
Member

I agree that we shouldn't have two modules named taproot. IIRC the crypto one has signature stuff only. We could rename it to bip340 which I think would be much clearer.

But maybe Kix is right that we should try flattening it. Or maybe we should just flatten the crypto module so it has no submodules. (We can keep the existing file structure, just make the submodules private and re-export all the types.)

@tcharding
Copy link
Member Author

With the whole "two taproot modules" thing, I tried to hide it from downstream by making the crypto one private, I didn't think it would be so confusing for us :(

@Kixunil
Copy link
Collaborator

Kixunil commented Nov 3, 2023

@apoelstra while bip340 would be technically clear it wouldn't be that easy to follow for people who don't remember all the BIP numbers by heart. Even I only remember couple important/(in)famous ones (21, 32, 39, 70...) Also now I'm thinking maybe we should rename existing bip* modules to use names rather than numbers and put the number in the doc comment.

Also the code in taproot and crypto::taproot is strongly linked from what I've seen (but maybe I just got confused).

@apoelstra
Copy link
Member

I wouldn't mind renaming the bip* modules where there's a common well-known name. I think all of them except 32 can be changed.

Having said that, I wonder if we need to back up and try to define the entire module structure of this crate before moving forward, since trying to make individual changes like this is starting to lead us in circles.

1 similar comment
@apoelstra
Copy link
Member

I wouldn't mind renaming the bip* modules where there's a common well-known name. I think all of them except 32 can be changed.

Having said that, I wonder if we need to back up and try to define the entire module structure of this crate before moving forward, since trying to make individual changes like this is starting to lead us in circles.

@tcharding
Copy link
Member Author

Having said that, I wonder if we need to back up and try to define the entire module structure of this crate before moving forward, since trying to make individual changes like this is starting to lead us in circles.

What if we come up with a prototypical set of applications/libraries that use rust-bitcoin. Allowing us to come up with concrete sets of things required by such users, then we have a clearer picture of what we are trying to achieve with the crate smashing. At some stage I rekon we need to get on a call (or plane) and nut this out.

@apoelstra
Copy link
Member

That sounds good to me. I think rust-lightning, rust-miniscript, bdk and esplora are the big ones. But we also want to support script hackery like whatever @stevenroose is currently working on as well as ICBOC. (Though I suspect that supporting the big things will imply that we support the small things.)

@Kixunil
Copy link
Collaborator

Kixunil commented Nov 5, 2023

I think hd_derivation would be an understandable name for BIP32. :) But yeah, that one is famous by number so I wouldn't be too worried about it.

@tcharding great idea! Among those mentioned by @apoelstra I would like to see a nice way of passing around primitive "interface" types (amount, address, txid...) without all complicated features so that different libraries can communicate them well without pulling in everything but a kitchen sink and blowing up the compilation time.

@tcharding tcharding marked this pull request as draft November 6, 2023 05:04
@tcharding
Copy link
Member Author

tcharding commented Nov 16, 2023

I think rust-lightning, rust-miniscript, bdk and esplora are the big ones.

You mean electrs, right? Any reason to use blockstream's version over Roman's version? Both would be leveraging rust-bitcoin in the same manner, right?

@Kixunil
Copy link
Collaborator

Kixunil commented Nov 17, 2023

Not sure how much they diverged but FWIW the Roman's version uses P2P as well, so that might cover more API surface.

@apoelstra
Copy link
Member

Yeah, I didn't realize there was a difference. I meant electrs then.

We recently introduce a bunch of formatting issues with the new `io`
crate, run the formatter.
Create a subdirectory `crytpo/ecdsa` and move `ecdsa.rs` to
`ecdsa/mod.rs`.

Done in preparation for further improvements to the `ecdsa` module.

No other changes.
Move the error code out of the `ecdsa/mod.rs` file and into a private
`error` submodule. Add public re-export so this not a breaking change.

Code move only plus import/export changes to build cleanly. No other
changes.
Create a subdirectory `crytpo/sighash` and move `sighash.rs` to
`sighash/mod.rs`.

Done in preparation for further improvements to the `sighash` module.

No other changes.
Move the error code out of the `sighash/mod.rs` file and into a private
`error` submodule. Add public re-export so this not a breaking change.

Code move only plus import/export changes to build cleanly. No other
changes.
Move the two sighash types (`TapSighashType` and `EcdsaSighashType`) to
a `sighash_type` submodule.

While we are at it lay the code out with impl blocks below the type
definitions as is customary.

Code move, re-order, and fix import/export paths only. No other changes.
@tcharding
Copy link
Member Author

tcharding commented Nov 20, 2023

I know you said you do not like the crypto module in general @Kixunil. If it is tolerable I'd like to merge this before then later going on to resolving what to really do with the crypto module. I have a tree of PRs that all build on top of this one.

For example: #2008

@tcharding tcharding marked this pull request as ready for review November 20, 2023 04:42
@tcharding
Copy link
Member Author

Fuck, review time is more valuable than dev time. Closing.

@tcharding tcharding closed this Nov 22, 2023
@tcharding tcharding deleted the 10-31-ecdsa-modules branch May 22, 2024 23:20
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.

None yet

3 participants