bitcoindevkit / bdk

A modern, lightweight, descriptor-based wallet library written in Rust!
Other
863 stars 308 forks source link

How to calculate TXIN_BASE_WEIGHT? #160

Closed danielabrozzoni closed 2 years ago

danielabrozzoni commented 3 years ago

PR #159 adds a TXIN_BASE_WEIGHT constant, which represents the base weight of a Txin, not counting the weight needed for satisfaying it. At the moment this constant is calculated as:

// prev_txid (32 bytes) + prev_vout (4 bytes) + sequence (4 bytes) + script_len (1 bytes)
pub const TXIN_BASE_WEIGHT: usize = (32 + 4 + 4 + 1) * 4;

My doubt is: should the script_len be included in the calculation? Miniscript docs say that the satisfaction weight Includes the weight of the VarInts encoding the scriptSig and witness stack length. (see https://docs.rs/miniscript/3.0.0/miniscript/descriptor/enum.Descriptor.html#method.max_satisfaction_weight)

Removing the script_len (i.e. defining TXIN_BASE_WEIGHT as (32 + 4 + 4) * 4) causes electrs tests to fail (min relay fee not met, see .https://github.com/bitcoindevkit/bdk/runs/1336260689?check_suite_focus=true).

I'm trying to understand if there's something I'm not seeing, or if there's some bug in bdk, or if I should ask directly in miniscript repo :)

murchandamus commented 3 years ago

That's a bit odd. As far as I know, each (p2pkh) input only has the outpoint (32+4 bytes), script length (1-9 B), script sig (variable), sequence (4 B). A segwit input should have an empty or smaller script sig, but adds the witness instead.

It is possible that this missing byte is produced by another part of the transaction length estimation. Segwit transactions have for example a header of 42 WU instead of 10 bytes. Maybe the added byte in the input helps satisfy the increased header size.

Have you looked at the complete transaction composition?

murchandamus commented 3 years ago

Okay, a similar question just came up for another project, so I invested a few hours to actually figure it out for myself: https://bitcoin.stackexchange.com/q/100159/5406

danielabrozzoni commented 3 years ago

Hi, sorry for the late reply. Forgot to mention initially: before using TXIN_BASE_WEIGHT there was a serialize(txin).len(), which gave 41 bytes as a result (because it counted one byte in the script_len). I haven't looked into the tx creation much (and I honestly don't have time to do so), so I don't know if the problem is there. I still think TXIN_BASE_WEIGHT shouldn't include the script_len byte though, I'm leaving this open so we don't forget :)

murchandamus commented 3 years ago

The non-witness part does need to have a script_len, although it will give a 0 there, because the scriptsig is empty. So, 41 bytes is correct for the non-witness part of a P2WPKH input.

afilini commented 3 years ago

Yeah I guess the problem is that we use rust-miniscript to estimate the size of the script_sig and witness, and in the docs it explicitly says that it accounts for all the necessary varints, so it should also account for the 0 varints that goes in the script_sig and we should define our BASE_WEIGHT as just 40 bytes.

I think the problem is what you described, we don't account for the larger header for witness transactions. It should be an easy fix if that's the issue.