From 97ee6b63062b3f3db559b78956e371b59a6a3b59 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Wed, 19 Oct 2022 16:26:46 +0200 Subject: [PATCH] onchaind: Adjust witness weight estimate to be more conservative We were missing the OP_PUSH for the pubkeys, and the spec mentions we should be using 73 bytes to estimate the witness weight. Effectively this adds 4 bytes which really just matters in case fees hit the floor, and computing the weight becomes important. Changelog-Fixed: onchaind: Witness weight estimations could be slightly lower than the VLS signer --- ...-tx-bitcoin_tx_2of2_input_witness_weight.c | 8 ++++++++ bitcoin/tx.c | 11 ++++++---- tests/test_closing.py | 20 +++++++++---------- tests/utils.py | 2 +- 4 files changed, 26 insertions(+), 15 deletions(-) diff --git a/bitcoin/test/run-tx-bitcoin_tx_2of2_input_witness_weight.c b/bitcoin/test/run-tx-bitcoin_tx_2of2_input_witness_weight.c index d1c6a3c25d98..b387279c2ba0 100644 --- a/bitcoin/test/run-tx-bitcoin_tx_2of2_input_witness_weight.c +++ b/bitcoin/test/run-tx-bitcoin_tx_2of2_input_witness_weight.c @@ -145,6 +145,14 @@ int main(int argc, const char *argv[]) /* 1 byte for num witnesses, one per witness element */ weight = 1; + + /* Two signatures, slightly overestimated to be 73 bytes each, + * while the actual witness will often be smaller.*/ + /* BOLT #03: + * Signatures are 73 bytes long (the maximum length). + */ + weight += 2 + 2; + for (size_t i = 0; i < tal_count(wit); i++) weight += 1 + tal_bytelen(wit[i]); assert(bitcoin_tx_2of2_input_witness_weight() == weight); diff --git a/bitcoin/tx.c b/bitcoin/tx.c index 26fdbb9751f4..7d575196c729 100644 --- a/bitcoin/tx.c +++ b/bitcoin/tx.c @@ -886,13 +886,16 @@ size_t bitcoin_tx_simple_input_weight(bool p2sh) size_t bitcoin_tx_2of2_input_witness_weight(void) { + /* BOLT #03: + * Signatures are 73 bytes long (the maximum length). + */ return 1 + /* Prefix: 4 elements to push on stack */ (1 + 0) + /* [0]: witness-marker-and-flag */ - (1 + 72) + /* [1] Party A signature and length prefix */ - (1 + 72) + /* [2] Party B signature and length prefix */ + (1 + 73) + /* [1] Party A signature and length prefix */ + (1 + 73) + /* [2] Party B signature and length prefix */ (1 + 1 + /* [3] length prefix and numpushes (2) */ - 33 + /* pubkey A (missing prefix) */ - 33 + /* pubkey B (missing prefix) */ + 1 + 33 + /* pubkey A (with prefix) */ + 1 + 33 + /* pubkey B (with prefix) */ 1 + 1 /* num sigs required and checkmultisig */ ); } diff --git a/tests/test_closing.py b/tests/test_closing.py index cd6e5242b951..f8baa3a761d4 100644 --- a/tests/test_closing.py +++ b/tests/test_closing.py @@ -26,7 +26,7 @@ def test_closing_simple(node_factory, bitcoind, chainparams): l1, l2 = node_factory.line_graph(2, opts={'plugin': coin_mvt_plugin}) chan = l1.get_channel_scid(l2) channel_id = first_channel_id(l1, l2) - fee = closing_fee(3750, 2) if not chainparams['elements'] else 4263 + fee = closing_fee(3750, 2) if not chainparams['elements'] else 4278 l1.pay(l2, 200000000) @@ -3389,7 +3389,7 @@ def test_closing_higherfee(node_factory, bitcoind, executor): l1.rpc.connect(l2.info['id'], 'localhost', l2.port) # This causes us to *exceed* previous requirements! - l1.daemon.wait_for_log(r'deriving max fee from rate 30000 -> 16440sat \(not 1000000sat\)') + l1.daemon.wait_for_log(r'deriving max fee from rate 30000 -> 16560sat \(not 1000000sat\)') # This will fail because l1 restarted! with pytest.raises(RpcError, match=r'Connection to RPC server lost.'): @@ -3578,12 +3578,12 @@ def save_notifications(message, progress, request, **kwargs): l1.rpc.close(l2.info['id'], feerange=['253perkw', 'normal']) if not chainparams['elements']: - l1_range = [138, 4110] - l2_range = [1027, 1000000] + l1_range = [139, 4140] + l2_range = [1035, 1000000] else: # That fee output is a little chunky. - l1_range = [220, 6547] - l2_range = [1636, 1000000] + l1_range = [221, 6577] + l2_range = [1644, 1000000] l1.daemon.wait_for_log('Negotiating closing fee between {}sat and {}sat satoshi'.format(l1_range[0], l1_range[1])) l2.daemon.wait_for_log('Negotiating closing fee between {}sat and {}sat satoshi'.format(l2_range[0], l2_range[1])) @@ -3642,12 +3642,12 @@ def test_close_weight_estimate(node_factory, bitcoind): log = l1.daemon.wait_for_log('sendrawtransaction: ') tx = re.match('.*sendrawtransaction: ([0-9a-f]*).*', log).group(1) - # This could actually be a bit shorter: 1 in 256 chance we get - # lucky with a sig and it's shorter. We have 2 sigs, so that's - # 1 in 128. Unlikely to do better than 2 bytes off though! + # To match the signer's estimate we use the pessimistic estimate + # of 73bytes / signature, which often ends up overestimating the + # actual weight by up to 4 bytes. signed_weight = int(bitcoind.rpc.decoderawtransaction(tx)['weight']) assert signed_weight <= actual_weight - assert signed_weight >= actual_weight - 2 + assert signed_weight >= actual_weight - 4 @pytest.mark.developer("needs dev_disconnect") diff --git a/tests/utils.py b/tests/utils.py index c1380d02c14c..b6dc4729f993 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -427,7 +427,7 @@ def basic_fee(feerate): def closing_fee(feerate, num_outputs): assert num_outputs == 1 or num_outputs == 2 - weight = 424 + 124 * num_outputs + weight = 428 + 124 * num_outputs return (weight * feerate) // 1000