Stack len for legacy max_satisfaction_weight calculations

[evanlinjin]

As per the definition of max_satisfaction_weight, we should take into consideration the varint that records witness stack size. Even legacy spends may require this field if the tx has at least 1 segwit spend, so we should add it.

[apoelstra]

Agreed. I seem to remember considering this but don't remember why I landed on the wrong behavior.

[apoelstra]

ACK 054313c

[LLFourn]

Concept NACK.

I think the weight being accounted for here is not really "satisfaction weight". When considering the weight of adding an additional input there two kinds of costs to consider:

1. The weight of the input itself which including the weight of the witness header the input may add and the witness stack length that may need to be added.
2. The weight of the satisfaction material: the scriptSig and witness.

I think max_satisfaction_weight should only do (2). So the change here should be to remove the witness stack length from segwit descriptor weight calculations and provide clear documentation that to figure out the weight of this additional input you need to know a few things about the other inputs in the transaction. Perhaps even a function like max_weight_of_input_addition(is_there_already_a_segwit_input, n_existing_legacy_inputs) but I don't know if this belongs here.

If we wanted to carry the logic of this PR forward then it should add an additional 2WU to every segwit input because that input might cause a witness header to be added. But this is still wrong. What if you have 100 legacy inputs already, and then you are considering adding a witness input. Isn't the 100WU cost for the 100 0x00 witness stack lengths for all the existing inputs actually being created by the new input. It seems bad that you can add a new input and add wayyyy more weight than what max_satisfaction_weight tells you.

[evanlinjin]

@LLFourn I agree with you on the fact that the current idea of max_satisfaction_weight seems quite clunky.

However, if we go by the definition of max_satisfaction_weight (which is witness field + scriptSigLen + scriptSig), and also the definition of witness field (as specified in BIP-0141), we see that the definition of witness field is INCLUSIVE of the stack length varint.

A witness field starts with a var_int to indicate the number of stack items for the txin. It is followed by stack items, with each item starts with a var_int to indicate the length.

Also, the marker and flag fields are not part of the "satisfying" material within an TxIn.

So in terms of the scope of the PR, I think what we have here is appropriate.

[apoelstra]

Oops, I did not see this before merging #472. I can revert that if we decide not to go in this direction.

Personally I have no strong feeling either way -- but we should document what our decision is very clearly. I have a weak preference for @evanlinjin's view -- that we should return the "difference of size of the satisfied TxIn vs the size of a TxIn with no witness" -- and I disagree that the witness flag is part of that calculation. (In general I can't think of a clean way for this library to assist people in accounting for the witness flag.)

[evanlinjin]

Oops, I did not see this before merging #472. I can revert that if we decide not to go in this direction.

@apoelstra Did you mean #473 ?

Personally I have no strong feeling either way -- but we should document what our decision is very clearly. I have a weak preference for @evanlinjin's view -- that we should return the "difference of size of the satisfied TxIn vs the size of a TxIn with no witness" -- and I disagree that the witness flag is part of that calculation. (In general I can't think of a clean way for this library to assist people in accounting for the witness flag.)

Maybe a better definition would be "difference of size of the satisfied TxIn vs the TxIn with no satisfaction data"? So a non-satisfied TxIn contains the scriptSig len varint (which is 0, so 4 WU), and also the stack len for txs that have at least 1 segwit spend (1 WU).

So "satisfaction weight" will contain additional varint weight (if varints of scriptSig len and stack len increase over the 1 byte threshold), and also the scriptSig and the rest of the witness field for that input.

This would make a lot more sense to me.

---

I have attempted this in #476

[apoelstra]

Oops, I did mean #473. And yes, I believe we are saying the same thing -- I still consider the scriptSig to be witness data even if it's not in the witness field.

[evanlinjin]

And yes, I believe we are saying the same thing -- I still consider the scriptSig to be witness data even if it's not in the witness field.

Woops sorry, I'm getting confused with all this witness terminology!

[LLFourn]

I think #476 is good and max_satisfaction_weight could be deprecated in favor of that approach under a new method name (to avoid nasty surprises).

[evanlinjin]

Replaced by #476