Return Weight in the TxIn/TxOut weight() methods

[danielabrozzoni]

No description provided.

[tcharding]

ACK 40c37ad

Thanks!

[apoelstra]

ACK 40c37ad

[yancyribbens]

I had put in this same PR a few months ago:
#1831

Here is why it wasn't merged then:

The return type is intentionally usize because it's error-prone to blindly sum them. (Although usize doesn't signal that well...)

Has there been a change of opinion about this or was it only @Kixunil that had these objections? Seems so since it was merged.

[apoelstra]

I think it was a combination of us forgetting about Kix's opposition (I remembered there being some controversy about using Weight more broadly, but this case seemed harmless enough by itself) and us fast-tracking changes that come from the BDK developers because we're trying to get them up to date with the latest rust-bitcoin.

cc @tcharding I wonder if we should put a 1-week delay (or even 2 weeks) on API changes, even if they have multiple ACKs?

[danielabrozzoni]

I didn't see #1831 and when I noticed that those methods returned a usize I thought it was an oversight. I should have checked better, sorry. It's ok for me to revert this one if you prefer, legacy_weight and segwit_weight work just fine with usize :)

[yancyribbens]

I also thought it was an oversight :) Anyway, I still don't fully understand why it should be usize really.

[apoelstra]

Personally I don't think Weight is any worse than usize. Neither type really expresses the subtleties associated with computing transaction weight from the weight of their components. So I don't think we should revert this.

But it is a process failure that we had an open issue with a PR, and then merged another PR in parallel which did substantially the same thing, so I'd like to avoid that in future.

[tcharding]

cc @tcharding I wonder if we should put a 1-week delay (or even 2 weeks) on API changes, even if they have multiple ACKs?

Lets have two weeks.

I think it was a combination of us forgetting about Kix's opposition (I remembered there being some controversy about using Weight more broadly, but this case seemed harmless enough by itself) and us fast-tracking changes that come from the BDK developers because we're trying to get them up to date with the latest rust-bitcoin.

FWIW I just reviewed the code changes and they seemed sane, I have never been personally burned by the weight addition stuff @Kixunil put forward so I'm not surprised I did not remember it.

Going forward, neither the usize nor the Weight give any hint as to this subtlety - we need to improve the API. This is exactly what rust-bitcoin is supposed to do, protect devs from the sharp dangerous bits of Bitcoin.

[tcharding]

Oh and props to @yancyribbens for noticing this fly past.

[yancyribbens]

Personally I like when changes I make get merged and reviewed quick. That way that don't become stale. Part of the miss here is in my opinion the shift in who is reviewing.