@tcharding can you add a commit to this which moves the whole amount module back to bitcoin before releasing? (We may need to add extension traits to other types which use Amount ... or you can just outright delete the methods and open another post-release PR to revert the commit ... and we will lose the core::ops impls on those types that use Amount but this is fine. Our next priority is to get Amount stabilized, which we will do before releasing any new version of bitcoin, and we can revert the commit then.)

@Kixunil our original plan here was to have a separate 1.x branch. You said in #3935 that it would make for a cleaner history if we were to add these extra commits which move/delete code and then unmove/undelete it. I don't care either way but I don't want to go in circles on this.

we will lose the core::ops impls on those types that use Amount but this is fine.

I think we won't since they can be implemented in bitcoin.

which we will do before releasing any new version of bitcoin

Hold on, I if bitcoin is not going to be released then I think the feature flag is cleaner. Brb, I'll write proper explanation of my confusion.

I think we won't since they can be implemented in bitcoin.

They can't be because the Weight and FeeRate types live in units while the ops traits live in std.

Hold on, I if bitcoin is not going to be released then I think the feature flag is cleaner. Brb, I'll write proper explanation of my confusion.

If we put a feature flag in then we've got stuff in our release candidate that we don't intend to release.

So my confusion is that I wasn't sure which of the following options you want to do:

1. release units 1.0 without Amount, release bitcoin that depends on new units, move Amount later
2. release units 1.0 without Amounts, don't release bitcoin until units has Amount
3. release units 1.0-alpha without Amount, release bitcoin that depends on it, move Amount later
4. release units 1.0-alphawithoutAmount, don't release bitcoin` yet
5. release units 1.0-apha with broken Amount and release bitcoin that depends on it, fix Amount later and bump towards 1.0 (possibly through rc)
6. release units 1.0-alpha with broken Amount, don't release bitcoin, fix Amount later and bump towards 1.0 (possibly through rc)

These options have different sensible approaches:

1. We definitely have to move Amount from units to bitcoin, since it's unstable
2. We can afford to keep amount in units if we make it impossible to use by accident - e.g. by cfg flag spelled unstable-internal-bitcoin-feature-do-not-use - if someone is stupid enough to rely on a feature with such a clear name we can freely ignore him and I'll be first one to say "let's break him". Same philosophy as with the hidden enum variant. This has the advantage compared to above that it's smaller diff, quicker to write, quicker to review, less git history pollution, etc
3. I think this approach is not that good and given the versioning/release scheme 5 is better - see below
4. Again, I think this is not that good and 6 is better
5. We can afford to just ignore everything and break stuff, but it might be nicer of us if we at least #[doc(hidden)] the amount module&reexports. Feature/cfg doesn't help much here since bitcoin will turn it on anyway.
6. As opposed to this we can also use a flag to further improve protection against accidental use, similarly to 2. Except the difference is in 2 we pretty much have to while here's it's only nice to have.

It's also possible that you want to do something I didn't cover, in which case please describe precisely what you want. I hope that regardless, this explanation sheds light on why we're going in circles and why I'm seemingly pushing against everything. It''s just a huge confusion.

2. release units 1.0 without Amounts, don't release bitcoin until units has Amount

Yes -- this is my current goal. If we manage to merge the amounts PRs in the next 48 hours (which depends a bit on my own review and server time) (and also on the fact that they'll need to be rebased if we intend to use master as the "units 1.0 branch" since we'll be moving code out from under them) (although I intend to ack and merge #4164 before this, to avoid that situation) then we might as well slip the module in.

But I don't see Amount as being essential to units 1.0, and by treating it as part of 1.0, it greatly increases the amount of API surface we need to review when moving from a rc to an actual release, as well as delaying the rc itself. And conversely Amount is complicated if we're merging it into a codebase full of other types which are technically unstable.

It's also possible that you want to do something I didn't cover, in which case please describe precisely what you want. I hope that regardless, this explanation sheds light on why we're going in circles and why I'm seemingly pushing against everything. It''s just a huge confusion.

Yes, I appreciate you writing out this big list of alternatives.

Alright then, so what do you think about keeping it behind the cfg? That way bitcoin in master can still compile and units can compile as well and be released/developed from master. No weird history confusion, branches or huge diffs. It should be much faster then to both write the PR (~3 lines change?) and review it. Or if you want to be extra sure we could add the amount.rs file to ignore list so it'd be published without it and whoever turns on that switch will get just broken build unless it's bitcoin with [patch] from this very repository.

And yes, you're right that Amount is technically not required. Even though units without it seemed crazy weird to me, it's not wrong.

release units 1.0 without Amounts, don't release bitcoin until units has Amount

Yes -- this is my current goal.

FWIW I had in mind (4) - same as (2) but only release the alpha. And the reason is that we have discussed previously that we want a final release with all the deprecated stuff in it which I only see happening if we keep all crates (units, primitives, and 0.33.0-alpha) live at the same time then do 1.0 all together.

@apoelstra you thinking (2) explains your comment the other day that you want to upgrade your stuff and not touch it again - at the time I did not understand how you thought that was possible.

At the risk of getting emotional again this whole cfg flag discussion, while valid is extremely annoying. Why did we just spend a year pulling stuff out of primitives and units into horrible extension traits if we could just cfg behind a feature the whole god damn lot?? Why did you just ask me to remove Opcodes from primitives if we could just cfg it. Why do we keep coming up with tiny new ideas that are slightly different every other week and not seeming to actually get anywhere? There are literally 8 basic ADTs in units (excluding parse which is borked still) and it has taken us a fucking year to get to this stage. That is unacceptably slow. If this is what open source development has become the sign me up for a corporate job because we are next to useless.

FWIW I had in mind (4) - same as (2) but only release the alpha. And the reason is that we have discussed previously that we want a final release with all the deprecated stuff in it which I only see happening if we keep all crates

Lemme chew on this. My hope had been that we could have the 0.x tree contain re-exports of the 1.0 stuff and somehow bolt on deprecated and unstable things. We can do this using extension traits, but this does mean that users' code will still break until they import the extension traits. This sucks, but I believe we have already committed to going this direction.

So my hope now is:

• To a large extent, users of 0.32 will be able to cargo upgrade to 0.32.10 or whatever and get most of the deprecations that way. Then they can update their code.
• Then they can update to 0.33, or 1.0-pre, or whatever we wanna call it. The compiler will tell them to import a bunch of extension traits, which they can do, and then the compiler will tell them that a bunch of stuff is deprecated, which they can fix.
• Then they can update to 1.0 with only a small set of necessary changes.

Between the 2nd and 3rd step maybe we have a series of "prerelease" versions that do incremental changes that cargo fix can handle, as Kix has suggested elsewhere. I think that, no matter what, we'll have to somewhat-manually construct this series of prereleases. I don't think it's realistic for the set of prereleases to match our actual code history.

IOW we shouldn't be afraid to break users in ways that we could prevent or ameliorate -- we can double-back and do the amelioration after the fact.

Why did we just spend a year pulling stuff out of primitives and units into horrible extension traits if we could just cfg behind a feature the whole god damn lot?? Why did you just ask me to remove Opcodes from primitives if we could just cfg it.

Those are very different things. Notice my comment that we cannot do that with option 1. We are currently not doing option 1, but later will do what is effectively option 1 - we will release bitcoin and have units 1.0 out too at some point. Extension traits was preparation for that.

Technically we could've done it the other way around (though frankly it didn't occur me at the time). We could've feature flagged everything and start with empty rates and slowly removing flags on individual items that we're 100% sure are done. However this approach involves releasing a bunch of super-tiny crates that are useless for most people. The idea that miniscript doesn't use Amount at all didn't occur to me and I had to think for like 10 seconds how it's even possible. (The bitcoin script itself cannot manipulate amounts at all yet, therefore miniscript cannot either.) But even if I knew this upfront I would've proposed to move everything except amount and release it, then do amount, then primitives without blocks (in none of my applications I touch blocks, just Transaction and its parts), then addresses, then add blocks to primitives (well, that's my priority list but it might be that addresses are less important than blocks to someone else and I wouldn't push too hard at that specific ordering)

Which makes me wonder, could we do the equivalent of option 1 above in case off primitives without blocks? Though blocks are probably not as messed up as Amount so maybe it's not worth bothering. And in my case I'll also need Address anyway (and by extension crypto).

Yes, I think we can release primitives without Block, and yes, I think it would be worth doing -- especially since Block is much more decoupled from Transaction than Amount is from the other unit types. So it should be totally possible for a 0.x Block type to coexist with a 1.0 Transaction type during the transition period.

It's true that Block is less messed up than Amount but there are still weirdnesses there -- we are still nailing down our timestamp newtypes, we haven't committed to a solution for how to keep the merkle root in the blockheader consistent with the transaction list in the block, there are edge cases in the difficulty recalculation that we aren't 100% certain of, etc.

Also, withholding Block also means withholding Target and Difficult and BlockHeader so it's a pretty large chunk of code, even if it turns out to all be individually simple.

Alright then, I'm in!

Block also means withholding Target and Difficult and BlockHeader

Just noting that Target and Work (which is likely what you meant instead of non-existing Difficulty) are in units. If we have any reason to doubt their API we need to pull them sooner. (But I personally am pretty confident in it, though that's from memory and I'd rather double check again.)

Yeah, I did mean Work -- and as I was writing that, I couldn't remember whether they lived in units or primitives.

I also think that they're in fine shape. But we will go over them carefully again before final release, of course.

Target and Work are in bitcoin. Only CompactTarget is in primitives.

units only has:

• amount::Denomination
• amount::NumOpResult<T>
• amount::Amount
• amount::Display
• amount::SignedAmount
• block::BlockHeight
• block::BlockInterval
• fee_rate::FeeRate
• locktime::absolute::Height
• locktime::absolute::Time
• locktime::relative::Height
• locktime::relative::Time
• weight::Weight

We have a tool that spits these out contrib/api.sh but it requires one to have the API text files locally in api/. See also contrib/api.sh primitives types_no_err. Output includes re-exports only (I removed them in the list above).

The list of types sounds good. Not sure why those two didn't end up in units but I guess it's for the better now. If we put amount behind some unstable flag we should be able to generate the API files. Once we've reviewed and stabilzied the API I'm in favor of putting in the API files to intentionally create friction adding new APIs (while not enforcing it for items behind the flag!). Any API change after stabilizing is potentially a risk since we may later find it's not a good API, thus the friction is warranted.

IIRC its just because we tried to move the least code possible to units to support primitives and Target and Work were not required.

I've made all the PRs in the todo list P-high - lets get this crate released crew.

diff --git a/units/Cargo.toml b/units/Cargo.toml
index 1bad34a7c..5cc16b6f4 100644
--- a/units/Cargo.toml
+++ b/units/Cargo.toml
@@ -18,13 +18,13 @@ std = ["alloc", "internals/std"]
 alloc = ["internals/alloc","serde?/alloc"]

 [dependencies]
-internals = { package = "bitcoin-internals", path = "../internals" }
+internals = { package = "bitcoin-internals", version = "0.4.0", path = "../internals" }

 serde = { version = "1.0.103", default-features = false, optional = true }
 arbitrary = { version = "1.4", optional = true }

 [dev-dependencies]
-internals = { package = "bitcoin-internals", path = "../internals", features = ["test-serde"] }
+internals = { package = "bitcoin-internals", version = "0.4.0", path = "../internals", features = ["test-serde"] }
 bincode = "1.3.1"
 serde = { version = "1.0.103", default-features = false, features = ["derive"] }
 serde_test = "1.0"

seems to fix it. Which is dumb and not DRY but ok.

@tcharding your patch removes the path and uses the version of internals from crates.io. This is not what we want, because it means that changes to internals in master will not be reflected in units.

Correct. I would have thought that for the stable units we don't want any further changes to the internals. And if we do we have to do another release of unit anyway. When we need local changes we have to put the patch section back in as it was before. I'm still confused as to why it was removed.

I actually ran into this surprising (to me) cargo behavior last week, so have a link handy: https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#multiple-locations

Kinda nice for local development in workspaces, but wasn't obvious to me at first what cargo would do with it.

Oh so we can add path back in as well. Shall we do that? Thanks @nyonson. Although I still think there may be some merit in not using local internals just be be extra careful?

Note please I only added the version number to units. Can do the others separately. Lets just get units out.

Yeah, this is kinda frustrating. If we have the path in there, then possibly we could publish a version of units which compiles locally but does not compile when published. But conversely, if we take the path out, then maybe we break internals locally but don't realize it because our local units isn't using it.

I'll test your previous version of this PR as well before merging this (which will have to be tomorrow, sorry, since it's late here).

No stress, for what its worth I think we should have just the version number and no path until we need it. I'm going to push that version again because then you can ack, merge, and release if you like it.

The previous patch fails for me with

error: found crates (`bitcoin_internals` and `bitcoin_internals`) with colliding StableCrateId values` when building `primitives

I'm not sure if this is rustc or my own tooling at fault. But I think we should just go ahead and publish the rc as-is.

If it later turns out that compilation is broken due to internals somehow, it is easy (though annoying and tedious) for us to publish a new major rev of internals and patch-release 1.0 to use that new major rev.

Yeah lets get it out. I'll throw a bit more time into it and we can do another RC if we want another internals release for some reason.

@tcharding if we want to "get it out" then you need to revert this to the one that keeps path in place.

Done, you'll have to tell me at some stage why you prefer that but I'm not fussed ATM.

@tcharding the other way doesn't build for me. See my previous comment.

Tagged and published.

Pop the bubbly, drinks on me!

Nice! Good job all.

bitcoin-units only has one owner on crates.io. bitcoin-primitives has @apoelstra and myself. bitcoin has Andrew and Matt.

You can add me if you think its prudent, if not we should find someone else to take it on.

I added you @tcharding. Also happy to add @Kixunil if you want.