While looking into #5054, found that these constructors were causing the daily fuzz cron job to fail, so removing them from the fuzz target

In cacf1b3

The commit log says 'deprecate ...' but then two public functions have their return types changed. This is a breaking change and needs to be done in a separate patch please. I"d actually prefer to see it in a different PR. In this PR we could just throw and allow-deprecated attribute on the call sites in bitcoin.

@tcharding the two functions that had their return types changed aren't public:

fn legacy_weight(&self) -> Option<Weight> // previously not wrapped in `Option`

and

fn segwit_weight(&self) -> Option<Weight> // previously not wrapped in `Option`

I just added the deprecated tag to the public ones, and for from_non_witness_data_size, updated the doc to state that it panics (due to the overflow found from fuzzing)

I believe these are public because they are defined as part of the TxInExt trait.

ah you're totally right. Is it ok if i use:

#![allow(deprecated_in_future)] // For `from_non_witness_data_size` and `from_witness_data_size`

at the top of transaction.rs? There's something weird going on with the internal_macros::define_extension_trait! marco that isn't adding the attributes correctly. For example,

internal_macros::define_extension_trait! {
    /// Extension functionality for the [`OutPoint`] type.
    pub trait OutPointExt impl for OutPoint {
        /// Constructs a new [`OutPoint`].
        #[inline]
        #[deprecated(since = "TBD", note = "use struct initialization syntax instead")]
        #[allow(clippy::new-ret-no-self)]
        fn new(txid: Txid, vout: u32) -> Self { OutPoint { txid, vout } }

        /// Checks if an `OutPoint` is "null".
        #[inline]
        #[deprecated(since = "TBD", note = "use `outpoint == OutPoint::COINBASE_PREVOUT` instead")]
        fn is_null(&self) -> bool { *self == OutPoint::COINBASE_PREVOUT }
    }
}

gets expanded to (note the missing deprecated and allow attributes):

#[cfg_attr(docsrs, doc(notable_trait))]
#[doc = " Extension functionality for the [`OutPoint`] type."]
pub trait OutPointExt: sealed::Sealed {
    #[doc = " Constructs a new [`OutPoint`]."]
    fn new(txid: Txid, vout: u32) -> Self;
    
    #[doc = " Checks if an `OutPoint` is \"null\"."]
    fn is_null(&self) -> bool;
}

impl OutPointExt for OutPoint {
    #[doc = " Constructs a new [`OutPoint`]."]
    fn new(txid: Txid, vout: u32) -> Self { 
        OutPoint { txid, vout } 
    }
    
    #[doc = " Checks if an `OutPoint` is \"null\"."]
    fn is_null(&self) -> bool { 
        *self == OutPoint::COINBASE_PREVOUT 
    }
}

I put the macro into an LLM, which got me to a point to where it was adding the missing attributes to the functions, but then it broke a bunch of stuff because

error: `#[deprecated]` attribute cannot be used on trait methods in impl blocks

I'm starting to understand why you guys hate macros now, and I'm quickly approaching that point too lol

ah I'm sorry mate, yeah deprecating doesn't work in macros. Let me think again tomorrow with fresh eyes.

I believe these are public because they are defined as part of the TxInExt trait.

Oof, now you catch something important and obvious that I totally missed.

Its good to have a crew huh. Seems the only constant is that us humans make mistakes constantly.

My suggestion would be to use from_vb_unchecked, that way we maintain the current behaviour and API. Then either raise an issue about the overflow bug and/or raise a PR that changes the API.

Sorry to burden you with such thoroughness.

@apoelstra may want to override me and just update this PR title/description himself to something sane.

My suggestion would be to backport the deprecation to 0.32.x and then just delete it in 0.33.x. This is a bit mean to users and is a faster deprecation than we'd prefer but these are fairly obscure methods and our extension-trait thing is causing us problems with doing a full deprecation cycle.

Let's go!

Did you mean delete them or just change the return value like originally attempted here?

Done in #5082, the question above needs addressing please to ensure the deprecation message is sensible.

Hmmm. So, we don't have good replacements for these methods (you can sorta use base_size and total_size but it's hard to do especially if you don't want arithmetic panics).

So I think "changing the signature" is the right way to go.

Now that I look at total_size and base_size I think these shoud also be changed to return an Option since it's possible to create transactions whose serialized size would overflow (at least, on 32-bit or lower machines).

I think the correct thing to do here, annoyingly, is to deprecate and rename, and change the signature of the renamed function. Then we can backport both the deprecation and the rename.

We may want to move to an issue to discuss the new names.

Is there anything wrong with the current names? Just to be clear we are talking about TxIn::segwit_weight and TxIn::legacy_weight.

If its all this hard why don't we just break the API, change the return value (exactly like what is done here) and call it a day. My main grievance, and why I originally brought it up, is that all this is hidden inside a PR titled 'units: Deprecate ...'.

Hmmm, yeah, maybe we should just break it.

No, the existing names are fine. The only reason to rename would be to change the API in a non-breaking way.

My main grievance, and why I originally brought it up, is that all this is hidden inside a PR titled 'units: Deprecate ...

Yeah, ok, agreed. @shinghim can you remove this change (you'll have to stick an explicit .unwrap into these methods I guess, but the result will be the same as the old code) and then open a separate PR which changes the return type?

While we're at it lets do total_size and base_size the same way.

sorry was out traveling, but I'm following now. Updated to use non-deprecated functions and just .unwrap() with panic documentation and will follow to change the return type in a new change

I just remembered that there used to be a bot that commented on the PR if it changed the public api. Did we get rid of it? I'm not at my computer so I can't check right now, but I don't want to forget and thought maybe one of you guys knew off the top of your head

There is a CI job that fails if a PR changes the API without updating the API text files (units only). We got rid of the bot.