Conversation
I'm not an expert on abstractions but I think the question we ask when choosing between a trait and inherent methods is "do we ever need to pass around a generic object that implements XYZ" (in this case do we expect to operate on objects that implement
Putting |
|
I'm leaning toward this being too much abstraction, yeah. There are only a few objects for which weight makes sense -- transactions and their parts, mainly. But the weight of a transaction isn't the sum of the weights of its parts. For example, the first input witness of a transaction causes an extra "flag" byte (weight 4, IIRC) to be added to the transaction while later witnesses don't. The 253th input adds an extra byte to the number-of-inputs count, etc. If we had a |
|
The reason this abstraction came to be was in negotiating who owes what fee in interactive transactions. In the lightning payjoin repo, Kixunil/loptos#14, there's a bunch of manual arithmetic which is hard to verify. I think the idea of exposing only You can see this in action in the payjoin crate. Here is one of many spots it handles weights to validate a psbt
|
|
It might make sense to have standalone I'd really like to see some sort of abstraction that can handle stuff like "when you add the nth input, for specific values of n, the total transaction weight changes" but I really don't see how. |
|
"when you add the nth input, for specific values of n, the total transaction weight changes" sounds like PSBT I/O arithmetic vs the existing TxIn / TxOut variety. |
|
I actually have another WIP project that needs all this weight calculation before the transaction is known. Actually all wallets should need it since they don't know the signature before constructing the I have a code for weight prediction in other project, I will cut it out and upstream it. Meanwhile I suggest we return |
Kixunil
left a comment
There was a problem hiding this comment.
I agree that extra trait is too much hassle for little to no benefit. I can't imagine where one would like to be able to treat different components of the transaction as the same.
Also maybe this should have FeeRate type included (or do it soon after). I initially wanted to do this PR but will only be able to do it later.
| #[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd)] | ||
| #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | ||
| #[cfg_attr(feature = "serde", serde(crate = "actual_serde"))] | ||
| pub struct Weight(usize); |
There was a problem hiding this comment.
I suspect u64 is better. Weight can be larger than actual size in memory and u64 will better interact with Amount and FeeRate.
| fn from(value: Weight) -> Self { | ||
| value.0 | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure about this one but don't have a particular argument against it.
70cf451 Add `Weight` and `FeeRate` newtypes (Martin Habovstiak) Pull request description: Use of general-purpose integers is often error-prone and annoying. We're working towards improving it by introducing newtypes. This adds newtypes for weight and fee rate to make fee computation easier and more readable. Note however that this dosn't change the type for individual parts of the transaction since computing the total weight is not as simple as summing them up and we want to avoid such confusion. Part of #630 Replaces #1607 (I want to get this in quickly and don't want to be blocked on DanGould's availability.) ACKs for top commit: apoelstra: ACK 70cf451 tcharding: ACK 70cf451 Tree-SHA512: ab9cc9f554a52ab0109ff23565b3e2cb2d3f609b557457b4afd8763e3e1b418aecbb3d22733e33304e858ecf900904a1af6e6fdc16dc21483b9ef84f56f103b2
|
replaced by #1627 😎 |
This work intends to addresses part of @Kixunil's #630 by introducing a Weight type by adding:
ComputeWeightfor weight calculation so that one may for instance predict how much transaction fee will change (would use inherent methods here). Implemented onComputeSizefor size calculation on Scriptu64usize for transaction weight, returned by weight-computing functions. usize was already being used for weight so it seemed more appropriate than u64This implementation differs from that in the payjoin crate because it exposes the type as public. That other version forces the use of the constructor on an inner module, but I felt we should expose weight since that was the purpose of #1411, & #1467. This is also still missing the FeeRate type.
I'm seeking feedback as to whether the design with extension traits makes sense and as to the location of the additions on this early draft, please.