I think this is dependent on how we view miniscript. I view miniscript as a bare script and its usage in a descriptor would determine how the script is used. In other words, the wsh descriptor would add the +1 to the stack element count(required for fee estimation).

In the case of wsh(ms), the stack element count would be stack_elem_count(ms) + 1. In the case of tr(pk, ....), it might be something else. Similarly, the satisfaction of miniscript should not add the witness script on the script stack top. That is something that would be decided by the descriptor using the miniscript. In case, of sh(wsh(ms)), the descriptor satisfy API also need to the ScriptSig, and in the case wsh(), it only needs to touch the witness stack. And in tr case, there would be the internal key, control block etc.

I know that we are only dealing with p2wsh style things in the current codebase, but the above approach makes more sense to me. The reason why I argue for stack size to not include the +1 is that it also has applications estimating fees (contrast to MAX_STANDARD_P2WSH_SCRIPT_SIZE which is just a validity check). What do you think? cc @sipa

@sanket1729 I agree; miniscript should just deal with the script side of things.

Though, that does make the max stack size check a bit ugly - but I guess it's ok as that's restricted to sanity.

I too view it this way, probably because i'm biased from diving into the Rust implem first :)

However this implementation is really a wsh() extension and it seems awkward to me that all the sanity checks would be done in src/script/miniscript.h but one in src/script/descriptor.cpp.
Plus, following this logic we should externalize all the P2WSH-specific checks?

The reason why I argue for stack size to not include the +1 is that it also has applications estimating fees

I don't get this, if you want to estimate fees you want the entire witness not just part of it? Also you want the size not the count?

Plus, following this logic we should externalize all the P2WSH-specific checks?

Ideally, we want to do this. However, doing this in the current codebase would require some re-architecture.

I don't get this, if you want to estimate fees you want the entire witness not just part of it? Also you want the size, not the count?

One of the other benefits of miniscript is that it allows reasoning for worst-case tx weight cost at script creation time. This can be helpful in setting a conservative fee to avoid second-party witness malleability to increase the overall tx weight. For this purpose, we need a 1) max satisfaction cost of script(not yet done in the c++ impl, but is fairly easy to do), 2) the worst-case stack size (to compute the size of varint that stores the number of elements in the stack) and 3) information about the descriptor to compute the final witness.

Points 1) and 2) should be the responsibility of the miniscript module. and 3) should be done by descriptor code. All of this is to say that stack size calculation(GetStackSize()), IMO should not include the +1. The checks(CheckStackSize) should be adjusted accordingly.

Anyways i think it's a nit (an ugly one but still), i don't want to fight on this and be a blocker to our steady progress. Just that the arguments don't seem right.

Agreed, let's move on to this. Let's keep rolling :) , once we have multiple descriptors supporting miniscript, we can clean up everything

ACK c4b9c10