plan: fix witness size for wsh and sh(wsh), fix scriptSig size of sh(wpkh) and sh(wsh)

[guggero]

This fixes two size estimation bugs I found while doing LLM-assisted review on btcsuite/btcd#2568.

Since I'm no Rust developer (though I'm learning a lot recently), the code was written by Claude Fable. But every line is carefully reviewed by myself and the commits have already gone through several revisions locally to make them as clean and easy to review as possible.

Fix 1: count the witness script in witness_size for wsh and sh(wsh)

This seems to be a bug introduced by #897: the witness script was added to the witness stack, but the count in witness_size was not updated.
This leads to serious underestimation of witness size, which in case of large scripts can cause you to run into the minimum relay fee rate limit if you're targeting a very low fee.

Fix 2: fix the scriptSig size in scriptsig_size for sh(wpkh) and sh(wsh)

This fixes an off-by-one in the script_size for the nested segwit p2sh cases. This is a tricky one, since it's two layers of data. The outer one (the payload that is the preimage of the hash) and the inner one (which is the output script as it would go into a native witness output).

Here's an attempt to visualize it a bit better:

  sh(wsh(pk(K)))                                  sh(wpkh(K))
  23        var-int: scriptSig is 35 bytes        17        var-int: 23 bytes
    22      OP_PUSHBYTES_34                         16      OP_PUSHBYTES_22
      00    OP_0            (witness version 0)       00    OP_0
      20    OP_PUSHBYTES_32                           14    OP_PUSHBYTES_20
      <32>  SHA256(witnessScript)                     <20>  HASH160(pubkey)
            ──────────────                                  ──────────────
            redeemScript = 34 bytes                         redeemScript = 22 bytes
            content 35, on-wire 36                          content 23, on-wire 24

Again, this only affects size (fee) estimation.

[apoelstra]

Can you pull just the tests out into separate commits? I think that me or @portlandhodl can do the fixes.

I appreciate the LLM disclosure. This code doesn't meet the quality bar for this project (in general I've been really disappointed with Claude stuff lately ... you'd think Fable 5.1 could avoid obfuscated code with unnecessary panic paths..), and I'd rather just reimplement the fixes than play LLM-by-proxy.

[apoelstra]

Thanks!!

[guggero]

Thank you for taking a look!

Sure, no problem. I've pushed a new commit that only has the test changes, then I'm going to close the PR. But don't worry about the commit or attribution, I don't care about that.

I definitely don't want to waste your time or cause frustration! And I'm sorry if that's already been the case.
Let me know if you prefer to implement #1037 yourself as well.
It's probably better for me to switch to issues rather than PRs anyway (I have one more small discrepancy I found, so I'll post that to the appropriate issue).

[apoelstra]

I definitely don't want to waste your time or cause frustration! And I'm sorry if that's already been the case.

Nope, definitely not. I appreciate the top-level comment explaining the provenance of the code (and also strongly signalling that you are a human who is not going to go on some crazy tirade, or a LLM agent which is going to go post on Hacker News about what an evil maintainer I am).

So then I just read the first function in the diff, which I didn't like, and then skipped ahead to find regression tests I could extract. And you did that part for me :P.

#1037 I think is in good shape. It has just the one misplaced deprecation note and then it should be good to go.

[portlandhodl]

I am going to open a new PR addressing this.