Skip to content

Introduce primitives, psbt-v0, address, and bip32 - and depend on miniscript!#2883

Closed
tcharding wants to merge 2 commits intorust-bitcoin:masterfrom
tcharding:crate-smashing
Closed

Introduce primitives, psbt-v0, address, and bip32 - and depend on miniscript!#2883
tcharding wants to merge 2 commits intorust-bitcoin:masterfrom
tcharding:crate-smashing

Conversation

@tcharding
Copy link
Copy Markdown
Member

@tcharding tcharding commented Jun 20, 2024 •
edited
Loading

This is not a merge candidate, it is experimental work to try to prove the state we would like to get to with these three new crates.

By depending on this branch we hope to be able to invert the dependency relationship between rust-bitcoin and rust-miniscript.

This is the demo work for epic #2882.

All the crypto features in primitives are a bit of a mess, related to recent discussion of cargo feature limitations.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Jun 20, 2024 •
edited
Loading

Just so you know what we are getting ourselves into when we try to re-do this in Nashville @apoelstra this took me a good chunk of yesterday and a couple hours this morning.

This was referenced Jun 20, 2024
Merged
Merged
tcharding added a commit to tcharding/rust-bitcoin that referenced this pull request Jun 20, 2024
@tcharding
Remove InvalidInternalKey variant from TaprootBuilderError
e8250cd 
This variant is unused, remove it.

Done as part of rust-bitcoin#2883.
@tcharding tcharding mentioned this pull request Jun 20, 2024
Merged
@tcharding tcharding force-pushed the crate-smashing branch 7 times, most recently from ac0545c to 9bed8e3 Compare June 20, 2024 23:21
apoelstra added a commit that referenced this pull request Jun 20, 2024
@apoelstra
Merge #2885: Remove usage of blockdata from paths
817e54f 
a42bcdc Remove usage of blockdata from paths (Tobin C. Harding)

Pull request description:

  the `blockdata` directory is code organisation thing, all the types/modules are re-exported from other places. In preparation for, and to make easier, the `primitives` crate smashing work - remove all explicit usage of `blockdata`.

  Note that the few instances remain as they seem required e.g.,

    `pub(in crate::blockdata::script)`

  Refactor only, no logic changes.

  Done as part of #2883

ACKs for top commit:
  apoelstra:
    ACK a42bcdc lgtm! I consider this "trivial" and will one-ACK merge it

Tree-SHA512: 310605e5203cf04aaeb91fe5512677b8f1438b183916686ba2cdc41ffdc18af7a0676206724e8a14c50ce6ed8faa9d48c69a2d5149eb1f56ae9c5f276fc5200f
@tcharding tcharding force-pushed the crate-smashing branch 8 times, most recently from 395929c to cab599a Compare June 21, 2024 01:28
@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Jun 21, 2024 •
edited
Loading

  • What is the benefit/purpose of splitting address out into a separate crate?

address has dependencies on base58, bech32 and possibly crypto (for Taproot tweaking and then definitely for silent payments) while the rest of the primitives only need hashes (I think).

Thanks, I thought this might be the assumption. We also have:

  • crypto::key depending on base58 because of WIF stuff.
  • witness_version depending on bech32 to do fe32 conversion.

Addresses are also conceptually outside of primitives because they don't exist on-chain and are basically a social convention for communicating scriptpubkeys.

Excellent, I did not think of this.

Having said all that I don't feel too strongly about it.

On that note, I'm going to leave it for now. As you suggested before.

@tcharding tcharding mentioned this pull request Jun 21, 2024
Closed
@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Jun 22, 2024

  • What is the benefit/purpose of splitting address out into a separate crate?

What @apoelstra said plus I've recently realized that the more we split the crate the easier code review becomes for applications that don't need certain parts of the library. For instance Firefish signers don't need addresses at all - they only operate on transactions/scripts/keys. Having more easily reviewable dependencies is a huge win for security-critical stuff.

As another non-security-critical example, electrs doesn't use addresses (it uses script hashes), so we could make it faster to compile by removing the whole address code.

Thus I strongly believe that address should be separated.

  • crypto::key depending on base58 because of WIF stuff.

  • witness_version depending on bech32 to do fe32 conversion.

I believe it's reasonable to make both optional dependencies. It's easy and niche.

apoelstra added a commit that referenced this pull request Jun 23, 2024
@apoelstra
Merge #2887: Remove InvalidInternalKey variant from `TaprootBuilder…
0974085 
…Error`

7265560 api: Run just check-api (Tobin C. Harding)
e8250cd Remove InvalidInternalKey variant from TaprootBuilderError (Tobin C. Harding)

Pull request description:

  This variant is unused, remove it.

  Done as part of #2883.

ACKs for top commit:
  Kixunil:
    ACK 7265560
  apoelstra:
    ACK 7265560

Tree-SHA512: 5cd27cacebcf078afdf1ceba4e3d51e78f20ee4883000b766efb7a246fd7a166240038a2e7d27249a22049c3258673a393ff2fd62cb4b27a2cade04b28ef2ac9
@tcharding tcharding force-pushed the crate-smashing branch 2 times, most recently from 12424b3 to 77afe64 Compare June 23, 2024 02:41
@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Jun 23, 2024 •
edited
Loading

Now includes:

  • Optional dependencies in primitives on bech32 and base58.
  • A separate address crate.

FTR the bitcoin-address name is taken on crates.io by another, to me until today, unknown developer: https://crates.io/crates/bitcoin-address/0.1.5

I'd say we need to come up with another name, he has a bunch of crates using the bitcoin-foo naming scheme.

@tcharding tcharding changed the title Introduce primitives, psbt-v0, and bip32 - and depend on miniscript! Introduce primitives, psbt-v0, address, and bip32 - and depend on miniscript`! Jun 23, 2024
@tcharding tcharding changed the title Introduce primitives, psbt-v0, address, and bip32 - and depend on miniscript`! Introduce primitives, psbt-v0, address, and bip32 - and depend on miniscript! Jun 23, 2024
@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Jun 23, 2024

FTR the bitcoin-address name is taken on crates.io by another, to me until today, unknown developer: https://crates.io/crates/bitcoin-address/0.1.5

Lol, he also has his own hex crate. Gotta love Rust.

And ok, hmm. bitcoin-address is hard to rename. The existing crate is super small, hasn't been maintained in a few years, and has lots of "learning Rust"isms like functions taking &String. We could try asking for it but it looks like Kix did this a year ago joegesualdo/bitcoin-address#1 and got no reply.

At the very least the existing bitcoin-address crate is totally safe but has a nearly-useless API. I worry that one day the name will be stolen by somebody malicious and replaced with a bad copy of our crate, whatever it's called. So we shouldn't pick a name too close to bitcoin-address.

Maybe we could take bitcoin-scriptpubkey or bitcoin-receive or bitcoin-address-encoding (maybe dangerous).

@yancyribbens
Copy link
Copy Markdown
Contributor

yancyribbens commented Jun 23, 2024

We could try asking for it but it looks like Kix did this a year ago joegesualdo/bitcoin-address#1 and got no reply.

Crates.io policies page mentions that if the current owner is not reachable that the crates.io team can help mediate.

I worry that one day the name will be stolen by somebody malicious and replaced with a bad copy of our crate, whatever it's called.

Sounds like that would be in violation of the usage policy and could probably be contested.

For better or worse crates.io is centralized. I guess it's also an option to just host crates outside of crates.io and then use any name.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Jun 23, 2024 •
edited
Loading

The name bitcoin-scriptpubkey is available. If we use that name I'm concerned that the abstraction might become muddied if/when we introduced script tagging. Presumably there will be some sort of "scriptPubkey" abstraction then in rust-bitcoin. Perhaps bitcoin-receive is a good name because its a bit more fuzzy - "stuff that allows you to receive bitcoin" seems like a good description for the crate that includes Address, AddressType, NetworkChecked, NetworkUnchecked, KnownHrp, AddressData.

Please see: #2898

@tcharding tcharding mentioned this pull request Jun 24, 2024
Closed
@yancyribbens yancyribbens mentioned this pull request Jun 24, 2024
Closed
@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Jun 25, 2024

  • Optional dependencies in primitives on bech32 and base58.

That's not what I meant.

@tcharding
Copy link
Copy Markdown
Member Author

tcharding commented Jun 26, 2024

  • Optional dependencies in primitives on bech32 and base58.

That's not what I meant.

I'm lost, by this comment and what you mean by the original comment then?

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Jun 26, 2024

I'm lost, by this comment and what you mean by the original comment then?

Sorry, I had a huge brain fart, I was tired. You were correct. Though I suspect the fe32 conversion will cause stabilization issues so maybe we'll have to remove it temporarily.

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Jun 26, 2024

We should remove the Fe32 conversion if it's causing any problems. Users can convert to a u8 and then to a Fe32 if they need to do this ... but I'm struggling to think when, outside of implementing the rust-bech32 crate (which owns Fe32 and can do whatever conversions it wants), they would ever need to do so.

@tcharding tcharding mentioned this pull request Jun 27, 2024
Merged
apoelstra added a commit that referenced this pull request Jun 28, 2024
@apoelstra
Merge #2886: taproot: Split errors up
6e1fe5e 
6a7f780 api: Run just check-api (Tobin C. Harding)
0c9223a Manually format write_err statement (Tobin C. Harding)
43d7c75 taproot: Add error types (Tobin C. Harding)
afe41c8 taproot: Split errors up (Tobin C. Harding)

Pull request description:

  Currently there are a couple of errors in the `taproot` module that are too general, resulting in functions that return a general error type when a specific one would do.

  Split two errors out and use them for for enum variants and function returns as possible.

  Done as part of #2883

ACKs for top commit:
  Kixunil:
    ACK 6a7f780
  apoelstra:
    ACK 6a7f780

Tree-SHA512: bf5ed50dd8f913280d007f03124c7918c4b6cd642e67726bc3ffff23d9897f764a4391e167115668c3a1e951197485eba280fdbb8a0ce1bb7efb051388d13997
@github-actions github-actions bot added the C-units PRs modifying the units crate label Jul 30, 2024
tcharding added 2 commits July 30, 2024 15:40
@tcharding
Introduce primitives, psbt-v0, bip32, and address
98f303e 
This is not a merge candidate, it is experimental work to try to prove
the state we would like to get to with these three new crates.

By depending on this branch we hope to be able to invert the dependency
relationship between `rust-bitcoin` and `rust-miniscript`.

This is the demo work for epic rust-bitcoin#2882.
@tcharding
Depend on miniscript
74516c7 
Now that we have a branch on `rust-miniscript` that depends on
`primitives`, `psbt-v0`, and `bip32` we can depend on `miniscript` here
in `rust-bitcoin` - BOOM!
This was referenced Jan 9, 2025
Open
Merged
@tcharding tcharding closed this Nov 18, 2025
@tcharding tcharding deleted the crate-smashing branch November 18, 2025 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

C-address C-bip32 C-bitcoin PRs modifying the bitcoin crate C-primitives C-psbt C-units PRs modifying the units crate doc test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tcharding @apoelstra @Kixunil @yancyribbens