Heads up (though CI will flag I'm sure) that this doesn't compile with 1.41.0:

   Compiling bitcoin v0.29.0 (/tmp/tmpiw57dv_s/bitcoin)
error[E0277]: `[(bool, &str); 6]` is not an iterator
    --> bitcoin/src/blockdata/transaction.rs:1555:32
     |
1555 |         for (is_segwit, tx) in txs {
     |                                ^^^ borrow the array with `&` or call `.iter()` on it to iterate over it
     |
     = help: the trait `std::iter::Iterator` is not implemented for `[(bool, &str); 6]`
     = note: arrays are not iterators, but slices like the following are: `&[1, 2, 3]`
     = note: required by `std::iter::IntoIterator::into_iter`

error[E0277]: the size for values of type `str` cannot be known at compilation time
    --> bitcoin/src/blockdata/transaction.rs:1561:61
     |
1561 |             let tx: Transaction = deserialize(Vec::from_hex(&tx).unwrap().as_slice()).unwrap();
     |                                                             ^^^ doesn't have a size known at compile-time
     |
     = help: the trait `std::marker::Sized` is not implemented for `str`
     = note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
     = note: all local variables must have a statically known size
     = help: unsized locals are gated as an unstable feature

otherwise d758074a8f583cd722f8b02deb0958789ea4a97f look good.

Pardon my ignorance, but what is the use of legacy_weight? Is segwit_weight not the only weight we should care about?

Is this about the one byte flag whether the tx contains witnesses or not?

@sanket1729 it's the weird reported to pre-segwit nodes ... which, okay, there's an argument that we don't really care about pre-segwit nodes 6 years after Segwit activation, but that's the value.

Do we know someone who actually uses it? If not, I'd just drop it for now and add when someone asks.

Heads up (though CI will flag I'm sure) that this doesn't compile with 1.41.0:

Updated, thanks!

Do we know someone who actually uses it? If not, I'd just drop it for now and add when someone asks.

This is needed for rust-bitcoin/rust-miniscript#476 :)

it's the weird reported to pre-segwit nodes ... which, okay, there's an argument that we don't really care about pre-segwit nodes 6 years after Segwit activation, but that's the value.

Yes, and also the correct weight of non-segwit inputs when included in non-segwit transactions, since the segwit_weight always includes (at least) 1WU for the witness length, even when the witness is empty

The new docs look great. Though I really wish there were a way to compute transaction weight without these footguns :/. Maybe we could have a method on Psbt that could more-reliably predict weight.

@apoelstra I was thinking about it too and I envision something like this:

/// Predicts the transaction weight without knowing the exact data of the transaction if the weights of inputs/outputs are known.
///
/// This is mainly useful when constructing the transaction before signing since signing data is not known but if one knows which
/// spending conditions are used their sizes may be known an can be used to compute the total size of the transaction.
pub fn predict_transaction_weight(inputs: impl IntoIterator<Item=InputWeightInfo>, outputs: implIterator<Weight>) -> Weight {
    // ...
}

pub struct InputWeightInfo {
    pub script_sig: Weight,
    pub witness: Weight,
}

+1 to adding _min to the names -- sorry, I forgot about this suggestion too.

I guess you missed my question about naming. I think naming it min_legacy_weight, min_segwit_weight and min_weight would make the potential foot gun more visible in the code. Is there some reason why the min_ prefix is undesirable?

Ops, sorry!
I do see your point about warning users, but I'm not sure if that's the best way of accomplishing it. I consider the segwit marker and the txin/txout varint length to be outside the txins/txouts themselves, so I think that the weight we give back is the exact weight of the object, not the minimum one.

I don't think the min_ makes a lot of sense since the varint and the markers are outside of the TxIn .

For the same reasoning, should we use Transaction::min_weight() because if serialized in a block could change the varint of the number of transactions in the block?

Yeah, these are good arguments. min_ does sound like the weight of the actual input could potentially be larger, or that it can't be computed exactly.

Yeah, good points.