bug: incorrectly calculated tx vsize

[theborakompanioni]

    Ahh, yes, there are multiple situations where JM sometimes incorrectly calculates tx vsize.

Originally posted by @kristapsk in https://github.com/JoinMarket-Org/joinmarket-clientserver/issues/1360#issuecomment-1262295463

Picking up from a bug report in #1360. The original comment contains an example that created an tx with too little fees (error pushing = -26 min relay fee not met).

Want to revisit this again, as Jam still enforces a minimum of 3 sats/vbyte. It'd be great to drop this limitation.

In the example linked above, the txs have 4 inputs and 1 output, all of type p2wpkh. The fee estimate_tx_size function returns: (witness_estimate, non_witness_estimate) := (432, 207) for both TXs - which means 639 bytes are taken into account when calculating the tx fee in estimate_tx_fee. However, they are 645 and 646 bytes respectively. This seems to stem from a few bytes variation in signature sizes which vary from 107 to 117 bytes (in the example above), but are always estimated at having 108 bytes (hardcoded).

It would be great for users to be able to specify any valid value for the fee settings..

The witness with 117 bytes is coming from the expired fidelity bond.. So this can be simplified and demonstrated by just doing a direct send of a single expired fidelity bond utxo with tx_fees := 1001 and tx_fees_factor := 0, which will fail all the time.

2023-01-04 19:19:33,037 [INFO]  Using this manually set tx feerate: 1001 sat/vkB (1.0 sat/vB).
2023-01-04 19:19:33,038 [INFO]  Using a fee of: 0.00000111 BTC (111 sat).
2023-01-04 19:19:33,041 [INFO]  Got signed transaction:

2023-01-04 19:19:33,042 [INFO]  {
    "hex": "020000000001019d96a41938382ea9a77675f8902fc1904479539c06467c5eae7b641a4b5698d30000000000feffffff01b54b194001000000160014651de42f6d85363293f129917bad6bb828c54aa002483045022100fec5706875d39c633f8304739fdae92181badfb6008807ffb6e804af669a5c660220384642434e1a013aea2a5252f1538879d5c1b6fbd26b4e386b9e8fa34ca7a612012a0400621d62b17521032b6eabc2d30812593d5f2ba51188f439523e9e6a3a5f07a3f494607876d8c0a6ac01621d62",
    "inputs": [
        {
            "outpoint": "d398564b1a647bae5e7c46069c53794490c12f90f87576a7a92e383819a4969d:0",
            "scriptSig": "",
            "nSequence": 4294967294,
            "witness": "02483045022100fec5706875d39c633f8304739fdae92181badfb6008807ffb6e804af669a5c660220384642434e1a013aea2a5252f1538879d5c1b6fbd26b4e386b9e8fa34ca7a612012a0400621d62b17521032b6eabc2d30812593d5f2ba51188f439523e9e6a3a5f07a3f494607876d8c0a6ac"
        }
    ],
    "outputs": [
        {
            "value_sats": 5370366901,
            "scriptPubKey": "0014651de42f6d85363293f129917bad6bb828c54aa0",
            "address": "bcrt1qv5w7gtmds5mr9yl39xghhttthq5v2j4qfck7m3"
        }
    ],
    "txid": "b80adc36c759d8cf9f5e1be2e20a8dd46a0788e07968132f426f771fc2b4a3ba",
    "nLockTime": 1646092801,
    "nVersion": 2
}
2023-01-04 19:19:33,042 [INFO]  Sends: 53.70366901 BTC (5370366901 sat) to destination: bcrt1qv5w7gtmds5mr9yl39xghhttthq5v2j4qfck7m3
2023-01-04 19:19:33,042 [DEBUG]  rpc: sendrawtransaction ['020000000001019d96a41938382ea9a77675f8902fc1904479539c06467c5eae7b641a4b5698d30000000000feffffff01b54b194001000000160014651de42f6d85363293f129917bad6bb828c54aa002483045022100fec5706875d39c633f8304739fdae92181badfb6008807ffb6e804af669a5c660220384642434e1a013aea2a5252f1538879d5c1b6fbd26b4e386b9e8fa34ca7a612012a0400621d62b17521032b6eabc2d30812593d5f2ba51188f439523e9e6a3a5f07a3f494607876d8c0a6ac01621d62']
2023-01-04 19:19:33,043 [WARNING]  error pushing = -26 min relay fee not met, 111 < 112
2023-01-04 19:19:33,043 [ERROR]  Transaction broadcast failed!

So although rounding up in estimate_tx_fee() would make it work in this case, the actual problem lies in estimate_tx_size() if I have understood correctly. @kristapsk Do you have any idea how to best fix or mitigate this problem?

[AdamISZ]

This problem is indeed a bug in the sense that something has been omitted/forgotten. Our transaction size estimation code simply never had code added to handle the fidelity bond type utxo input/output (which? more on that below!).

So, the estimation algo has two input arguments txtype and outtype to help it be as accurate as possible, where txtype specifies the type of scriptPubKey being spent in all the inputs (already we can see here a limitation), but moreover, currently the only implemented txtypes are p2pkh, p2sh-p2wpkh and p2wpkh, with the latter of course being the default nowadays. The outtype defaults to 'whatever is the same as the current wallet'; we call jmclient.wallet.BaseWallet.get_outtype(), but it can return (correctly) p2wsh, using the auto-detection code in our bitcoin library python-bitcointx. See the code here:

https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/a3e1ba3c2572da5545c3a87d4d9b38919d7b828a/jmclient/jmclient/wallet.py#L462-L480

When a person does the recommended action: spend an expired fidelity bond into another, they are going to call taker_utils.direct_send which will check the input and output types here:

https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/a3e1ba3c2572da5545c3a87d4d9b38919d7b828a/jmclient/jmclient/taker_utils.py#L82-L90

For the input type, I believe there is not a problem, though probably that's just luck: it just checks the type of the wallet and ignores the fact that this transaction's input is actually of a different type, but the witness is just an ECDSA signature (around 72 bytes, variable in theory, in practice never different by more than about 2 bytes), i.e. it's exactly the same format of witness as for plain vanilla p2wpkh. The reason it only needs a signature can be seen from the scriptPubKey:

https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/a3e1ba3c2572da5545c3a87d4d9b38919d7b828a/jmbitcoin/jmbitcoin/secp256k1_transaction.py#L219-L220

For the output type however, it's not the same: the output scriptPubKey, if spending to a fidelity bond address, is a p2wsh of the above custom script, and so its length in bytes is 33 bytes, not 21 bytes (due to sha2 instead of hash160, plus one version byte). As can be seen in the estimate_tx_size function, there is an attempt to account for a different script types in outputs, with a field OUTPUT_EXTRA_BYTES (ping @takinbo ), but it does not account for p2wsh.

Summarizing: 1. txfee estimates can always be incorrect by a few bytes with our choice not to low-R grind ECDSA signatures (that can be changed). 2. Our code is messy, but there is a reason: from the start we were focused on having to estimate the fee of transaction before they are fully formed, meaning we don't simply calculate the exact size of the fully signed transaction. 3. The low hanging fruit: account for p2wsh outputs in estimate_tx_size to prevent this specific failure vector.

(So in that list, 3. is the obvious action item, but 1. and 2. should probably be addressed, and for 2. it might mean: make a separate code section for calculating transaction sizes exactly, in those cases where we do have the fully signed transaction already available).

(Also don't forget our randomization choice, it is there for a reason and it is why we do not let users just choose 1.01 sats/vbyte).

[theborakompanioni]

Thank you for the detailed response, @AdamISZ .

For the input type, I believe there is not a problem, though probably that's just luck: it just checks the type of the wallet and ignores the fact that this transaction's input is actually of a different type, but the witness is just an ECDSA signature (around 72 bytes, variable in theory, in practice never different by more than about 2 bytes), i.e. it's exactly the same format of witness as for plain vanilla p2wpkh.

Have to say I am unfortunately not really deep into it yet, but the above example spends an expired fidelity bond (p2wsh) to a p2wpkh output (1 in, 1 out), right?. The length of the witness in the above example is 117 bytes, right? Not ~108 as used in the estimate as the txtype is p2wpkh, not p2wsh (when I understood the first section of your comment). So txtype := p2wpkh and outtype := p2wpkh, not txtype := p2wsh and outtype := p2wpkh as it is not supported atm. Sorry for my uninformed take, but that's what I don't get when you say "For the input type, I believe there is not a problem".

What I understand (at least I think I do) is that it will always assume all inputs to have the same type. So it is a matter of using the most expensive input type as assumption for all, right?

The docs of estimate_tx_size really explains the limitations quite well:

https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/5740c3bec8e3497891ba50bb9155fec5eabe67de/jmbitcoin/jmbitcoin/secp256k1_transaction.py#L110-L143

---

As can be seen in the estimate_tx_size function, there is an attempt to account for a different script types in outputs, with a field OUTPUT_EXTRA_BYTES (ping @takinbo ), but it does not account for p2wsh.

I think the 12 extra bytes (33 - 21 = 12) is taken care of when creating a fidelity bond (n p2wpkh in, 1 p2wsh out). https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/5740c3bec8e3497891ba50bb9155fec5eabe67de/jmbitcoin/jmbitcoin/secp256k1_transaction.py#L37

However, when a bond is renewed (1p2wsh in, 1 p2wsh out), then it is a matter of the "wrong" input size estimate, right? Again referencing my misunderstanding of "For the input type, I believe there is not a problem".

---

(Also don't forget our randomization choice, it is there for a reason and it is why we do not let users just choose 1.01 sats/vbyte).

I am not exactly sure what you mean.. currently, there is no code in place that stops users from using tx_fees := 1001 in combination with tx_fees_factor := 0, correct? estimate_fee_per_kb() in BitcoinCoreInterface will happily return (non randomized) 1001: https://github.com/JoinMarket-Org/joinmarket-clientserver/blob/5740c3bec8e3497891ba50bb9155fec5eabe67de/jmclient/jmclient/blockchaininterface.py#L472-L484

[AdamISZ]

Oops sorry! Bad analysis based on me rushing out an answer without actually reading your example (well, also, and forgetting something).

I forgot that p2wsh inputs have to reveal the redeem script. Here it's 2a0400621d62b17521032b6eabc2d30812593d5f2ba51188f439523e9e6a3a5f07a3f494607876d8c0a6ac which is length(42) then should be [locktime, OP_CHECKLOCKTIMEVERIFY, OP_DROP, pub, OP_CHECKSIG]

So that adds up: you get ~ 43 bytes extra in the witness, which corresponds to close to 11 vbytes with discount.

I believe the other thing I talked about above (that the p2wsh output type is not accounted for) is also true, but doesn't apply to your case, as you say. (wrong there, see below). Thanks for checking it @theborakompanioni

I am not exactly sure what you mean.. currently, there is no code in place that stops users from using tx_fees := 1001 in combination with tx_fees_factor := 0, correct?

Good point I forgot about tx_fees_factor, I was thinking only of default.

[AdamISZ]

I think the 12 extra bytes (33 - 21 = 12) is taken care of when creating a fidelity bond (n p2wpkh in, 1 p2wsh out).

Correct. Huh, how did I miss that? I wonder if I was looking at the wrong commit or something.

Anyway the conclusion seems clear:

• the output type is OK for p2wsh
• the input calculation is wrong for fidelity bond inputs; we need to add accounting for the redeem script

I'll try to knock together a short PR to fix just that.

[AdamISZ]

Although uh 'short PR' - it isn't that easy really. Our model of 'one type of input' is really borked by this situation. I might have to go further and write a separate txsize calculation function specifically for the case where all input types are known. Can't really see it working otherwise.

[kristapsk]

Although uh 'short PR' - it isn't that easy really. Our model of 'one type of input' is really borked by this situation. I might have to go further and write a separate txsize calculation function specifically for the case where all input types are known. Can't really see it working otherwise.

Alternative would be to rewrite function to specify number of various type inputs and outputs. Kinda ugly, lots of parameters, but should work in all situations. Like I do with my bitcoin-scripts. https://github.com/kristapsk/bitcoin-scripts/blob/a9b7986abb79bf5403579a9e60a2bcfc4bd0ee0d/inc.common.sh#L78-L85

[AdamISZ]

Yeah true. I was considering that.

The first thing I have to remember is, there is a circular dependency even outside coinjoin, i.e. the problem that you don't know the exact witness size in bytes until you have signed, but you can only set the output amounts, and therefore the fee, before you sign. So until we go to taproot in which signature sizes are always exactly 64 bytes, we can never be perfect, to the byte (though low-R grinding maybe fixes it?). But that imperfection should, I guess be wrong by less than 1 vbyte since there's a 1/4 discount.

Then to consider the different scenarios we support: either we direct send and know all the input types and output types, or we are doing the pre-tx-creation estimation (as happens in 2 or 3 places), for coinjoins. Those cases are basically hopeless in trying to be exact, though we can get pretty close in the 'we have the tx template, but not yet the signatures' case.

On balance I think you're right. Stick with one function, estimate_tx_size, but where the first two arguments are lists of input types, not numbers. Then, in direct_send, we can correctly pass in the list of types according to what we're actually spending.

[kristapsk]

but where the first two arguments are lists of input types, not numbers.

Right! Like ins=[p2wpkh,p2wpkh,p2wpkh] in case of three p2wpkh inputs. Maybe I should rewrite my bash function to do something similar (bash v4+ has pretty good improved support for arrays).

[theborakompanioni]

With the fix in #1421 and the release of v0.9.9, I am closing this issue now. Seems to work great. Thank you :pray: