From 7aa8ffa2a07aeaa7e043498d9a983bd19da989bf Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 6 Jul 2020 14:56:14 +0930 Subject: [PATCH] bitcoin: add weight calculation helpers. These are pulled from wallet/wallet.c, with the fix now that we grind sigs. This reduces the fees we pay slightly, as you can see in the coinmoves changes. I now print out all the coin moves in suitable format before we match: you only see this if the test fails, but it's really helpful. Signed-off-by: Rusty Russell --- bitcoin/tx.c | 72 ++++++++++++++++++++++++++++++++++++ bitcoin/tx.h | 11 ++++++ common/utxo.c | 5 +++ common/utxo.h | 2 + lightningd/closing_control.c | 2 +- tests/test_misc.py | 34 ++++++++--------- tests/test_plugin.py | 6 +-- tests/test_wallet.py | 2 +- tests/utils.py | 8 ++++ wallet/wallet.c | 49 +++--------------------- 10 files changed, 125 insertions(+), 66 deletions(-) diff --git a/bitcoin/tx.c b/bitcoin/tx.c index 4027c0d9b..6469fa285 100644 --- a/bitcoin/tx.c +++ b/bitcoin/tx.c @@ -731,3 +731,75 @@ wally_tx_output_get_amount(const struct wally_tx_output *output) return amount; } + +/* Various weights of transaction parts. */ +size_t bitcoin_tx_core_weight(size_t num_inputs, size_t num_outputs) +{ + size_t weight; + + /* version, input count, output count, locktime */ + weight = (4 + varint_size(num_inputs) + varint_size(num_outputs) + 4) + * 4; + + /* Add segwit fields: marker + flag */ + weight += 1 + 1; + + /* A couple of things need to change for elements: */ + if (chainparams->is_elements) { + /* Each transaction has surjection and rangeproof (both empty + * for us as long as we use unblinded L-BTC transactions). */ + weight += 2 * 4; + + /* An elements transaction has 1 additional output for fees */ + weight += bitcoin_tx_output_weight(0); + } + return weight; +} + +size_t bitcoin_tx_output_weight(size_t outscript_len) +{ + size_t weight; + + /* amount, len, scriptpubkey */ + weight = (8 + varint_size(outscript_len) + outscript_len) * 4; + + if (chainparams->is_elements) { + /* Each output additionally has an asset_tag (1 + 32), value + * is prefixed by a version (1 byte), an empty nonce (1 + * byte), two empty proofs (2 bytes). */ + weight += (32 + 1 + 1 + 1) * 4; + } + + return weight; +} + +/* We grind signatures to get them down to 71 bytes (+1 for sighash flags) */ +size_t bitcoin_tx_input_sig_weight(void) +{ + return 1 + 71 + 1; +} + +/* We only do segwit inputs, and we assume witness is sig + key */ +size_t bitcoin_tx_simple_input_weight(bool p2sh) +{ + size_t weight; + + /* Input weight: txid + index + sequence */ + weight = (32 + 4 + 4) * 4; + + /* We always encode the length of the script, even if empty */ + weight += 1 * 4; + + /* P2SH variants include push of <0 <20-byte-key-hash>> */ + if (p2sh) + weight += 23 * 4; + + /* Account for witness (1 byte count + sig + key) */ + weight += 1 + (bitcoin_tx_input_sig_weight() + 1 + 33); + + /* Elements inputs have 6 bytes of blank proofs attached. */ + if (chainparams->is_elements) + weight += 6; + + return weight; +} diff --git a/bitcoin/tx.h b/bitcoin/tx.h index bdc2928e0..e4f726469 100644 --- a/bitcoin/tx.h +++ b/bitcoin/tx.h @@ -230,4 +230,15 @@ void towire_bitcoin_tx(u8 **pptr, const struct bitcoin_tx *tx); void towire_bitcoin_tx_output(u8 **pptr, const struct bitcoin_tx_output *output); int wally_tx_clone(struct wally_tx *tx, struct wally_tx **output); + +/* Various weights of transaction parts. */ +size_t bitcoin_tx_core_weight(size_t num_inputs, size_t num_outputs); +size_t bitcoin_tx_output_weight(size_t outscript_len); + +/* Weight to push sig on stack. */ +size_t bitcoin_tx_input_sig_weight(void); + +/* We only do segwit inputs, and we assume witness is sig + key */ +size_t bitcoin_tx_simple_input_weight(bool p2sh); + #endif /* LIGHTNING_BITCOIN_TX_H */ diff --git a/common/utxo.c b/common/utxo.c index f5a932e77..7ff9bcfa6 100644 --- a/common/utxo.c +++ b/common/utxo.c @@ -113,3 +113,8 @@ struct bitcoin_tx *tx_spending_utxos(const tal_t *ctx, return tx; } + +size_t utxo_spend_weight(const struct utxo *utxo) +{ + return bitcoin_tx_simple_input_weight(utxo->is_p2sh); +} diff --git a/common/utxo.h b/common/utxo.h index 1652cc2c6..93a16bbf5 100644 --- a/common/utxo.h +++ b/common/utxo.h @@ -59,4 +59,6 @@ struct bitcoin_tx *tx_spending_utxos(const tal_t *ctx, u32 nlocktime, u32 nsequence); +/* Estimate of (signed) UTXO weight in transaction */ +size_t utxo_spend_weight(const struct utxo *utxo); #endif /* LIGHTNING_COMMON_UTXO_H */ diff --git a/lightningd/closing_control.c b/lightningd/closing_control.c index 353d95b0a..98cd6ef5c 100644 --- a/lightningd/closing_control.c +++ b/lightningd/closing_control.c @@ -69,7 +69,7 @@ static bool closing_fee_is_acceptable(struct lightningd *ld, type_to_string(tmpctx, struct amount_sat, &last_fee)); /* Weight once we add in sigs. */ - weight = bitcoin_tx_weight(tx) + 74 * 2; + weight = bitcoin_tx_weight(tx) + bitcoin_tx_input_sig_weight() * 2; /* If we don't have a feerate estimate, this gives feerate_floor */ min_feerate = feerate_min(ld, &feerate_unknown); diff --git a/tests/test_misc.py b/tests/test_misc.py index 0f03bffdb..48a5b9d1f 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -623,40 +623,40 @@ def test_withdraw_misc(node_factory, bitcoind, chainparams): {'type': 'chain_mvt', 'credit': 2000000000, 'debit': 0, 'tag': 'deposit'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'}, - {'type': 'chain_mvt', 'credit': 0, 'debit': 1993730000, 'tag': 'withdrawal'}, + {'type': 'chain_mvt', 'credit': 0, 'debit': 1993745000, 'tag': 'withdrawal'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 2000000000, 'tag': 'withdrawal'}, - {'type': 'chain_mvt', 'credit': 0, 'debit': 6270000, 'tag': 'chain_fees'}, + {'type': 'chain_mvt', 'credit': 0, 'debit': 6255000, 'tag': 'chain_fees'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'}, - {'type': 'chain_mvt', 'credit': 0, 'debit': 1993730000, 'tag': 'withdrawal'}, + {'type': 'chain_mvt', 'credit': 0, 'debit': 1993745000, 'tag': 'withdrawal'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 2000000000, 'tag': 'withdrawal'}, - {'type': 'chain_mvt', 'credit': 0, 'debit': 6270000, 'tag': 'chain_fees'}, - {'type': 'chain_mvt', 'credit': 1993730000, 'debit': 0, 'tag': 'deposit'}, - {'type': 'chain_mvt', 'credit': 1993730000, 'debit': 0, 'tag': 'deposit'}, + {'type': 'chain_mvt', 'credit': 0, 'debit': 6255000, 'tag': 'chain_fees'}, + {'type': 'chain_mvt', 'credit': 1993745000, 'debit': 0, 'tag': 'deposit'}, + {'type': 'chain_mvt', 'credit': 1993745000, 'debit': 0, 'tag': 'deposit'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'}, - {'type': 'chain_mvt', 'credit': 0, 'debit': 1993730000, 'tag': 'withdrawal'}, + {'type': 'chain_mvt', 'credit': 0, 'debit': 1993745000, 'tag': 'withdrawal'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 2000000000, 'tag': 'withdrawal'}, - {'type': 'chain_mvt', 'credit': 0, 'debit': 6270000, 'tag': 'chain_fees'}, - {'type': 'chain_mvt', 'credit': 1993730000, 'debit': 0, 'tag': 'deposit'}, + {'type': 'chain_mvt', 'credit': 0, 'debit': 6255000, 'tag': 'chain_fees'}, + {'type': 'chain_mvt', 'credit': 1993745000, 'debit': 0, 'tag': 'deposit'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'}, - {'type': 'chain_mvt', 'credit': 0, 'debit': 1993370000, 'tag': 'withdrawal'}, + {'type': 'chain_mvt', 'credit': 0, 'debit': 1993385000, 'tag': 'withdrawal'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 2000000000, 'tag': 'withdrawal'}, - {'type': 'chain_mvt', 'credit': 0, 'debit': 6630000, 'tag': 'chain_fees'}, - {'type': 'chain_mvt', 'credit': 1993370000, 'debit': 0, 'tag': 'deposit'}, + {'type': 'chain_mvt', 'credit': 0, 'debit': 6615000, 'tag': 'chain_fees'}, + {'type': 'chain_mvt', 'credit': 1993385000, 'debit': 0, 'tag': 'deposit'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'}, - {'type': 'chain_mvt', 'credit': 0, 'debit': 11961030000, 'tag': 'withdrawal'}, - {'type': 'chain_mvt', 'credit': 0, 'debit': 13530000, 'tag': 'chain_fees'}, - {'type': 'chain_mvt', 'credit': 11961030000, 'debit': 0, 'tag': 'deposit'}, + {'type': 'chain_mvt', 'credit': 0, 'debit': 11961135000, 'tag': 'withdrawal'}, + {'type': 'chain_mvt', 'credit': 0, 'debit': 13485000, 'tag': 'chain_fees'}, + {'type': 'chain_mvt', 'credit': 11961135000, 'debit': 0, 'tag': 'deposit'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'}, - {'type': 'chain_mvt', 'credit': 0, 'debit': 11957378000, 'tag': 'withdrawal'}, - {'type': 'chain_mvt', 'credit': 0, 'debit': 3652000, 'tag': 'chain_fees'}, + {'type': 'chain_mvt', 'credit': 0, 'debit': 11957490000, 'tag': 'withdrawal'}, + {'type': 'chain_mvt', 'credit': 0, 'debit': 3645000, 'tag': 'chain_fees'}, ] check_coin_moves(l1, 'wallet', wallet_moves, chainparams) diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 707acc0d6..c83abe119 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -1408,10 +1408,10 @@ def test_coin_movement_notices(node_factory, bitcoind, chainparams): l2_wallet_mvts = [ {'type': 'chain_mvt', 'credit': 2000000000, 'debit': 0, 'tag': 'deposit'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'}, - {'type': 'chain_mvt', 'credit': 0, 'debit': 995418000, 'tag': 'withdrawal'}, + {'type': 'chain_mvt', 'credit': 0, 'debit': 995425000, 'tag': 'withdrawal'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 1000000000, 'tag': 'withdrawal'}, - {'type': 'chain_mvt', 'credit': 0, 'debit': 4582000, 'tag': 'chain_fees'}, - {'type': 'chain_mvt', 'credit': 995418000, 'debit': 0, 'tag': 'deposit'}, + {'type': 'chain_mvt', 'credit': 0, 'debit': 4575000, 'tag': 'chain_fees'}, + {'type': 'chain_mvt', 'credit': 995425000, 'debit': 0, 'tag': 'deposit'}, {'type': 'chain_mvt', 'credit': 100001000, 'debit': 0, 'tag': 'deposit'}, {'type': 'chain_mvt', 'credit': 944570000, 'debit': 0, 'tag': 'deposit'}, ] diff --git a/tests/test_wallet.py b/tests/test_wallet.py index d02c5834f..bc1451ddc 100644 --- a/tests/test_wallet.py +++ b/tests/test_wallet.py @@ -710,7 +710,7 @@ def test_sign_and_send_psbt(node_factory, bitcoind, chainparams): # do proper acccounting for it. {'type': 'chain_mvt', 'credit': 0, 'debit': 4000000000, 'tag': 'withdrawal'}, {'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'chain_fees'}, - {'type': 'chain_mvt', 'credit': 988255000, 'debit': 0, 'tag': 'deposit'}, + {'type': 'chain_mvt', 'credit': 988285000, 'debit': 0, 'tag': 'deposit'}, ] check_coin_moves(l1, 'wallet', wallet_coin_mvts, chainparams) diff --git a/tests/utils.py b/tests/utils.py index 8908a291f..843664393 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -1,6 +1,7 @@ from pyln.testing.utils import TEST_NETWORK, SLOW_MACHINE, TIMEOUT, VALGRIND, DEVELOPER, DEPRECATED_APIS # noqa: F401 from pyln.testing.utils import env, only_one, wait_for, write_config, TailableProc, sync_blockheight, wait_channel_quiescent, get_tx_p2wsh_outnum # noqa: F401 import bitstring +from pyln.client import Millisatoshi EXPERIMENTAL_FEATURES = env("EXPERIMENTAL_FEATURES", "0") == "1" COMPAT = env("COMPAT", "1") == "1" @@ -53,6 +54,13 @@ def check_coin_moves(n, account_id, expected_moves, chainparams): moves = n.rpc.call('listcoinmoves_plugin')['coin_moves'] node_id = n.info['id'] acct_moves = [m for m in moves if m['account_id'] == account_id] + for mv in acct_moves: + print("{{'type': '{}', 'credit': {}, 'debit': {}, 'tag': '{}'}}," + .format(mv['type'], + Millisatoshi(mv['credit']).millisatoshis, + Millisatoshi(mv['debit']).millisatoshis, + mv['tag'])) + assert len(acct_moves) == len(expected_moves) for mv, exp in list(zip(acct_moves, expected_moves)): assert mv['version'] == 1 diff --git a/wallet/wallet.c b/wallet/wallet.c index 865531af3..1b25c5f59 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -418,34 +418,13 @@ static const struct utxo **wallet_select(const tal_t *ctx, struct wallet *w, const struct utxo **utxos = tal_arr(ctx, const struct utxo *, 0); tal_add_destructor2(utxos, destroy_utxos, w); - /* version, input count, output count, locktime */ - weight = (4 + 1 + 1 + 4) * 4; - - /* Add segwit fields: marker + flag */ - weight += 1 + 1; - - /* The main output: amount, len, scriptpubkey */ - weight += (8 + 1 + outscriptlen) * 4; + /* We assume < 253 inputs, and margin is tiny if we're wrong */ + weight = bitcoin_tx_core_weight(1, num_outputs) + + bitcoin_tx_output_weight(outscriptlen); /* Change output will be P2WPKH */ if (may_have_change) - weight += (8 + 1 + BITCOIN_SCRIPTPUBKEY_P2WPKH_LEN) * 4; - - /* A couple of things need to change for elements: */ - if (chainparams->is_elements) { - /* Each transaction has surjection and rangeproof (both empty - * for us as long as we use unblinded L-BTC transactions). */ - weight += 2 * 4; - - /* Each output additionally has an asset_tag (1 + 32), value - * is prefixed by a version (1 byte), an empty nonce (1 - * byte), two empty proofs (2 bytes). */ - weight += (32 + 1 + 1 + 1) * 4 * num_outputs; - - /* An elements transaction has 1 additional output for fees */ - weight += (8 + 1) * 4; /* Bitcoin style output */ - weight += (32 + 1 + 1 + 1) * 4; /* Elements added fields */ - } + weight += bitcoin_tx_output_weight(BITCOIN_SCRIPTPUBKEY_P2WPKH_LEN); *fee_estimate = AMOUNT_SAT(0); *satoshi_in = AMOUNT_SAT(0); @@ -453,7 +432,6 @@ static const struct utxo **wallet_select(const tal_t *ctx, struct wallet *w, available = wallet_get_utxos(ctx, w, output_state_available); for (i = 0; i < tal_count(available); i++) { - size_t input_weight; struct amount_sat needed; struct utxo *u = tal_steal(utxos, available[i]); @@ -473,24 +451,7 @@ static const struct utxo **wallet_select(const tal_t *ctx, struct wallet *w, output_state_available, output_state_reserved)) fatal("Unable to reserve output"); - /* Input weight: txid + index + sequence */ - input_weight = (32 + 4 + 4) * 4; - - /* We always encode the length of the script, even if empty */ - input_weight += 1 * 4; - - /* P2SH variants include push of <0 <20-byte-key-hash>> */ - if (u->is_p2sh) - input_weight += 23 * 4; - - /* Account for witness (1 byte count + sig + key) */ - input_weight += 1 + (1 + 73 + 1 + 33); - - /* Elements inputs have 6 bytes of blank proofs attached. */ - if (chainparams->is_elements) - input_weight += 6; - - weight += input_weight; + weight += bitcoin_tx_simple_input_weight(u->is_p2sh); if (!amount_sat_add(satoshi_in, *satoshi_in, u->amount)) fatal("Overflow in available satoshis %zu/%zu %s + %s",