Remove TxIn::BASE_WEIGHT and fix docs on effective_value()
#2345
Conversation
The term "base weight" is already defined in the codebase, in the `TxIn`, by the `base_weight` function. Recently we added a const `BASE_WEIGHT` that contradicts this meaning because it does not include the script sig. Remove `BASE_WEIGHT` and just use the already available consts instead.
bitcoin/src/blockdata/transaction.rs
Outdated
| /// | ||
| /// Note: the effective value of a [`Transaction`] may increase less than the effective value of | ||
| /// Note: the effective value of a [`Transaction`] may increase more than the effective value of |
There was a problem hiding this comment.
Nope, this was correct. (It originally wasn't until I pointed it out in my review BTW; it's subtle.)
The logic is inverted - the more bytes transaction has the more you spend on fees thus the value is lower.
There was a problem hiding this comment.
Oh, that is subtle, I'll improve the docs - thanks.
There was a problem hiding this comment.
The docs refer to TxOut but we are calculating the effective value of in input, I didn't notice that before.
There was a problem hiding this comment.
Well, each input is an output of previous transaction (except for coinbase) so it's often hard to communicate when we kinda mean both.
| @@ -1170,10 +1170,10 @@ impl From<&Transaction> for Wtxid { | |||
|
|
|||
| /// Computes the value of an output accounting for the cost of spending it. | |||
| /// | |||
| /// The effective value is the value of an output value minus the amount to spend it. That is, the | |||
| /// effective_value can be calculated as: value - (fee_rate * weight). | |||
| /// The effective value is the value of an output minus the cost to spend it. That is, the | |||
There was a problem hiding this comment.
Shouldn't it say "cost of spending it"?
There was a problem hiding this comment.
Both are grammatically correct. "of" goes with "spending" and "to" goes with "spend" - English, I know.
There was a problem hiding this comment.
"spending" somehow feels more natural here but I don't know why. But I don't really care unless there's an actual difference in meaning.
The change to effective value of transaction when adding an input is While we are at it improve the first paragraph, stylistic only.
tcharding
force-pushed
the
01-17-effective-value
branch
from
45d79b5
to
ee11ba3
Compare
|
Changes in force push:
|
|
I could go either way on the first commit since |
| let weight = satisfaction_weight.checked_add(TxIn::BASE_WEIGHT)?; | ||
| let previous_output_weight = Weight::from_vb_unwrap(OutPoint::SIZE as u64); | ||
| let sequence_weight = Weight::from_vb_unwrap(Sequence::SIZE as u64); | ||
| let weight = satisfaction_weight.checked_add(previous_output_weight)?.checked_add(sequence_weight)?; |
There was a problem hiding this comment.
Just realized, the original was more efficient because it had one-less overflow check. Maybe the compiler can remove it but I wouldn't bet on it here.
I suggest you add the constants together without check.
| /// Note: the effective value of a [`Transaction`] may increase less than the effective value of | ||
| /// a [`TxOut`] when adding another [`TxOut`] to the transaction. This happens when the new | ||
| /// [`TxOut`] added causes the output length `VarInt` to increase its encoding length. | ||
| /// Note: adding an input to a [`Transaction`] the effective value of the transaction may increase |
There was a problem hiding this comment.
Shouldn't this say "when adding" ?
|
Closing, I'm going to PR to revert #2230. |
|
I'm not investing any more time into this. Let it be on record that IMO #2230 is not of a standard that can be released in the 1.0 release and that |
BASE_WEIGHTis misnamed and the docs are wrong. Please see commit logs for full explanations.