From c0d68c5c2c287c82a5b276919ad1f810845898b1 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 6 May 2025 05:28:39 +0930 Subject: [PATCH] pytest: create warning if we grind signature shorter than 71 bytes, don't fail. One in 256 times, we will grind a signature to 70 bytes (or shorter). This breaks our feerate tests. Unfortunately the grinding is deterministic, so there doesn't seem to be a way to avoid it. So we add a log message, and then we skip the feerate test if it happens. Signed-off-by: Rusty Russell --- common/hsm_version.h | 2 +- hsmd/hsmd.c | 7 ++++-- hsmd/hsmd_wire.csv | 2 ++ hsmd/libhsmd.c | 14 ++++++++++++ hsmd/libhsmd.h | 2 ++ lightningd/hsm_control.c | 2 ++ lightningd/lightningd.c | 1 + lightningd/lightningd.h | 1 + lightningd/options.c | 4 ++++ tests/test_wallet.py | 47 +++++++++++++++++++++++++--------------- 10 files changed, 61 insertions(+), 21 deletions(-) diff --git a/common/hsm_version.h b/common/hsm_version.h index bcc7b06c5..b03fb6496 100644 --- a/common/hsm_version.h +++ b/common/hsm_version.h @@ -27,7 +27,7 @@ * v5 with preapprove_check: 0ed6dd4ea2c02b67c51b1420b3d07ab2227a4c06ce7e2942d946967687e9baf7 * v6 no secret from get_per_commitment_point: 0cad1790beb3473d64355f4cb4f64daa80c28c8a241998b7ef0223385d7ffff9 * v6 with sign_bolt12_2 (tweak using node id): 8fcb731279a10af3f95aeb8be1da6b2ced76a1984afa18c5f46a03515d70ea0e - * v6 (internal rework only): 22dc3adfb0d63dbd6a92ff1daacbb6218c5efa3080f8910933a18683ce75bf5f + * v6 with dev_warn_on_overgrind: a273b68e19336073e551c01a78bcd1e1f8cc510da7d0dde3afc45e249f9830cc */ #define HSM_MIN_VERSION 5 #define HSM_MAX_VERSION 6 diff --git a/hsmd/hsmd.c b/hsmd/hsmd.c index 16bf1e09a..6a5cc487b 100644 --- a/hsmd/hsmd.c +++ b/hsmd/hsmd.c @@ -442,8 +442,11 @@ static struct io_plan *preinit_hsm(struct io_conn *conn, if (tlv->no_preapprove_check) dev_no_preapprove_check = *tlv->no_preapprove_check; - status_debug("preinit: dev_fail_preapprove = %u, dev_no_preapprove_check = %u", - dev_fail_preapprove, dev_no_preapprove_check); + if (tlv->warn_on_overgrind) + dev_warn_on_overgrind = *tlv->warn_on_overgrind; + + status_debug("preinit: dev_fail_preapprove = %u, dev_no_preapprove_check = %u, dev_warn_on_overgrind = %u", + dev_fail_preapprove, dev_no_preapprove_check, dev_warn_on_overgrind); /* We don't send a reply, just read next */ return client_read_next(conn, c); } diff --git a/hsmd/hsmd_wire.csv b/hsmd/hsmd_wire.csv index ede88bff6..b758eea28 100644 --- a/hsmd/hsmd_wire.csv +++ b/hsmd/hsmd_wire.csv @@ -14,6 +14,8 @@ tlvtype,hsmd_dev_preinit_tlvs,fail_preapprove,1 tlvdata,hsmd_dev_preinit_tlvs,fail_preapprove,fail,bool, tlvtype,hsmd_dev_preinit_tlvs,no_preapprove_check,3 tlvdata,hsmd_dev_preinit_tlvs,no_preapprove_check,disable,bool, +tlvtype,hsmd_dev_preinit_tlvs,warn_on_overgrind,5 +tlvdata,hsmd_dev_preinit_tlvs,warn_on_overgrind,enable,bool, #include # Start the HSM. diff --git a/hsmd/libhsmd.c b/hsmd/libhsmd.c index cc655065c..eeaf9a6cf 100644 --- a/hsmd/libhsmd.c +++ b/hsmd/libhsmd.c @@ -40,6 +40,7 @@ bool initialized = false; /* Do we fail all preapprove requests? */ bool dev_fail_preapprove = false; bool dev_no_preapprove_check = false; +bool dev_warn_on_overgrind = false; struct hsmd_client *hsmd_client_new_main(const tal_t *ctx, u64 capabilities, void *extra) @@ -553,6 +554,7 @@ static void sign_our_inputs(struct hsm_utxo **utxos, struct wally_psbt *psbt) for (size_t j = 0; j < psbt->num_inputs; j++) { struct privkey privkey; struct pubkey pubkey; + bool needed_sig; if (!wally_psbt_input_spends(&psbt->inputs[j], &utxo->outpoint)) @@ -590,6 +592,10 @@ static void sign_our_inputs(struct hsm_utxo **utxos, struct wally_psbt *psbt) wally_psbt_signing_cache_enable(psbt, 0); is_cache_enabled = true; } + + /* We watch for pre-taproot variable-length sigs */ + needed_sig = (psbt->inputs[j].signatures.num_items == 0); + if (wally_psbt_sign(psbt, privkey.secret.data, sizeof(privkey.secret.data), EC_FLAG_GRIND_R) != WALLY_OK) { @@ -602,6 +608,14 @@ static void sign_our_inputs(struct hsm_utxo **utxos, struct wally_psbt *psbt) j, fmt_pubkey(tmpctx, &pubkey), fmt_wally_psbt(tmpctx, psbt)); } + if (dev_warn_on_overgrind + && needed_sig + && psbt->inputs[j].signatures.num_items == 1 + && psbt->inputs[j].signatures.items[0].value_len < 71) { + hsmd_status_fmt(LOG_BROKEN, NULL, + "overgrind: short signature length %zu", + psbt->inputs[j].signatures.items[0].value_len); + } tal_wally_end(psbt); } } diff --git a/hsmd/libhsmd.h b/hsmd/libhsmd.h index 33a6dfadc..db927284b 100644 --- a/hsmd/libhsmd.h +++ b/hsmd/libhsmd.h @@ -100,4 +100,6 @@ extern struct secret *dev_force_bip32_seed; extern bool dev_fail_preapprove; /* If they specify --dev-no-preapprove-check it ends up in here. */ extern bool dev_no_preapprove_check; +/* If they specify --dev-warn-on-overgrind it ends up in here. */ +extern bool dev_warn_on_overgrind; #endif /* LIGHTNING_HSMD_LIBHSMD_H */ diff --git a/lightningd/hsm_control.c b/lightningd/hsm_control.c index d4c0ccfb2..7c0c6587f 100644 --- a/lightningd/hsm_control.c +++ b/lightningd/hsm_control.c @@ -119,6 +119,8 @@ struct ext_key *hsm_init(struct lightningd *ld) &ld->dev_hsmd_fail_preapprove); tlv->no_preapprove_check = tal_dup(tlv, bool, &ld->dev_hsmd_no_preapprove_check); + tlv->warn_on_overgrind = tal_dup(tlv, bool, + &ld->dev_hsmd_warn_on_overgrind); msg = towire_hsmd_dev_preinit(tmpctx, tlv); if (!wire_sync_write(ld->hsm_fd, msg)) diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 49473c1c7..9820f7556 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -152,6 +152,7 @@ static struct lightningd *new_lightningd(const tal_t *ctx) ld->dev_allow_shutdown_destination_change = false; ld->dev_hsmd_no_preapprove_check = false; ld->dev_hsmd_fail_preapprove = false; + ld->dev_hsmd_warn_on_overgrind = false; ld->dev_handshake_no_reply = false; ld->dev_strict_forwarding = false; ld->dev_limit_connections_inflight = false; diff --git a/lightningd/lightningd.h b/lightningd/lightningd.h index c1ba87bbf..ce756f859 100644 --- a/lightningd/lightningd.h +++ b/lightningd/lightningd.h @@ -356,6 +356,7 @@ struct lightningd { /* hsmd characteristic tweaks */ bool dev_hsmd_no_preapprove_check; bool dev_hsmd_fail_preapprove; + bool dev_hsmd_warn_on_overgrind; /* Tell connectd not to talk after handshake */ bool dev_handshake_no_reply; diff --git a/lightningd/options.c b/lightningd/options.c index 767d03a5d..8efe28840 100644 --- a/lightningd/options.c +++ b/lightningd/options.c @@ -936,6 +936,10 @@ static void dev_register_opts(struct lightningd *ld) opt_set_crash_timeout, NULL, ld, "Crash if we are still going after this long."); + clnopt_noarg("--dev-warn-on-overgrind", OPT_DEV, + opt_set_bool, + &ld->dev_hsmd_warn_on_overgrind, + "Warn if we create signatures that are not exactly 71 bytes."); /* This is handled directly in daemon_developer_mode(), so we ignore it here */ clnopt_noarg("--dev-debug-self", OPT_DEV, opt_ignore, diff --git a/tests/test_wallet.py b/tests/test_wallet.py index 646d005ed..8c85ac68b 100644 --- a/tests/test_wallet.py +++ b/tests/test_wallet.py @@ -300,9 +300,25 @@ def feerate_from_psbt(bitcoind, node, psbt): return fee / weight * 1000 +# I wish we could force libwally to use different entropy and thus force it to +# create 71-byte sigs always! +def did_short_sig(node): + # This can take a moment to appear in the log! + time.sleep(1) + return node.daemon.is_in_log('overgrind: short signature length') + + +def check_feerate(node, actual_feerate, expected_feerate): + # Feerate can't be lower. + assert actual_feerate > expected_feerate - 2 + if not did_short_sig(node): + assert actual_feerate < expected_feerate + 2 + + def test_txprepare(node_factory, bitcoind, chainparams): amount = 1000000 - l1 = node_factory.get_node(random_hsm=True) + l1 = node_factory.get_node(random_hsm=True, options={'dev-warn-on-overgrind': None}, + broken_log='overgrind: short signature length') addr = chainparams['example_addr'] # Add some funds to withdraw later @@ -322,8 +338,7 @@ def test_txprepare(node_factory, bitcoind, chainparams): # 4 inputs, 2 outputs (3 if we have a fee output). assert len(decode['vin']) == 4 assert len(decode['vout']) == 2 if not chainparams['feeoutput'] else 3 - # Feerate should be ~ as we asked for - assert normal_feerate_perkw - 2 < feerate_from_psbt(bitcoind, l1, prep['psbt']) < normal_feerate_perkw + 2 + check_feerate(l1, feerate_from_psbt(bitcoind, l1, prep['psbt']), normal_feerate_perkw) # One output will be correct. outnum = [i for i, o in enumerate(decode['vout']) if o['value'] == Decimal(amount * 3) / 10**8][0] @@ -351,8 +366,7 @@ def test_txprepare(node_factory, bitcoind, chainparams): assert decode['vout'][0]['value'] > Decimal(amount * 6) / 10**8 - Decimal(0.0002) assert decode['vout'][0]['scriptPubKey']['type'] == 'witness_v0_keyhash' assert scriptpubkey_addr(decode['vout'][0]['scriptPubKey']) == addr - # Feerate should be ~ as we asked for - assert normal_feerate_perkw - 2 < feerate_from_psbt(bitcoind, l1, prep2['psbt']) < normal_feerate_perkw + 2 + check_feerate(l1, feerate_from_psbt(bitcoind, l1, prep2['psbt']), normal_feerate_perkw) # If I cancel the first one, I can get those first 4 outputs. discard = l1.rpc.txdiscard(prep['txid']) @@ -371,8 +385,7 @@ def test_txprepare(node_factory, bitcoind, chainparams): assert decode['vout'][0]['value'] > Decimal(amount * 4) / 10**8 - Decimal(0.0002) assert decode['vout'][0]['scriptPubKey']['type'] == 'witness_v0_keyhash' assert scriptpubkey_addr(decode['vout'][0]['scriptPubKey']) == addr - # Feerate should be ~ as we asked for - assert normal_feerate_perkw - 1 < feerate_from_psbt(bitcoind, l1, prep3['psbt']) < normal_feerate_perkw + 1 + check_feerate(l1, feerate_from_psbt(bitcoind, l1, prep3['psbt']), normal_feerate_perkw) # Cannot discard twice. with pytest.raises(RpcError, match=r'not an unreleased txid'): @@ -393,8 +406,7 @@ def test_txprepare(node_factory, bitcoind, chainparams): assert decode['vout'][0]['value'] > Decimal(amount * 10) / 10**8 - Decimal(0.0003) assert decode['vout'][0]['scriptPubKey']['type'] == 'witness_v0_keyhash' assert scriptpubkey_addr(decode['vout'][0]['scriptPubKey']) == addr - # Feerate should be ~ as we asked for - assert normal_feerate_perkw - 2 < feerate_from_psbt(bitcoind, l1, prep4['psbt']) < normal_feerate_perkw + 2 + check_feerate(l1, feerate_from_psbt(bitcoind, l1, prep4['psbt']), normal_feerate_perkw) l1.rpc.txdiscard(prep4['txid']) # Try passing in a utxo set @@ -402,8 +414,7 @@ def test_txprepare(node_factory, bitcoind, chainparams): for utxo in l1.rpc.listfunds()["outputs"]][:4] prep5 = l1.rpc.txprepare([{addr: Millisatoshi(amount * 3.5 * 1000)}], utxos=utxos) - # Feerate should be ~ as we asked for - assert normal_feerate_perkw - 2 < feerate_from_psbt(bitcoind, l1, prep3['psbt']) < normal_feerate_perkw + 2 + check_feerate(l1, feerate_from_psbt(bitcoind, l1, prep3['psbt']), normal_feerate_perkw) # Try passing unconfirmed utxos unconfirmed_utxo = l1.rpc.withdraw(l1.rpc.newaddr()["bech32"], 10**5) @@ -414,7 +425,7 @@ def test_txprepare(node_factory, bitcoind, chainparams): # Feerate should be ~ as we asked for unconfirmed_tx = bitcoind.rpc.getrawmempool(True)[unconfirmed_utxo["txid"]] feerate_perkw = int(unconfirmed_tx['fees']['base'] * 100_000_000) * 1000 / unconfirmed_tx['weight'] - assert normal_feerate_perkw - 1 < feerate_perkw < normal_feerate_perkw + 1 + check_feerate(l1, feerate_perkw, normal_feerate_perkw) decode = bitcoind.rpc.decoderawtransaction(prep5['unsigned_tx']) assert decode['txid'] == prep5['txid'] @@ -444,7 +455,7 @@ def test_txprepare(node_factory, bitcoind, chainparams): prep5 = l1.rpc.txprepare([{addr: Millisatoshi(amount * 3 * 1000)}, {addr: 'all'}]) # Feerate should be ~ as we asked for - assert normal_feerate_perkw - 2 < feerate_from_psbt(bitcoind, l1, prep5['psbt']) < normal_feerate_perkw + 2 + check_feerate(l1, feerate_from_psbt(bitcoind, l1, prep5['psbt']), normal_feerate_perkw) l1.rpc.txdiscard(prep5['txid']) with pytest.raises(RpcError, match=r"'all'"): prep5 = l1.rpc.txprepare([{addr: 'all'}, {addr: 'all'}]) @@ -452,7 +463,7 @@ def test_txprepare(node_factory, bitcoind, chainparams): prep5 = l1.rpc.txprepare([{addr: Millisatoshi(amount * 3 * 500 + 100000)}, {addr: Millisatoshi(amount * 3 * 500 - 100000)}]) # Feerate should be ~ as we asked for - assert normal_feerate_perkw - 2 < feerate_from_psbt(bitcoind, l1, prep5['psbt']) < normal_feerate_perkw + 2 + check_feerate(l1, feerate_from_psbt(bitcoind, l1, prep5['psbt']), normal_feerate_perkw) decode = bitcoind.rpc.decoderawtransaction(prep5['unsigned_tx']) assert decode['txid'] == prep5['txid'] # 4 inputs, 3 outputs(include change). @@ -484,7 +495,8 @@ def test_txprepare(node_factory, bitcoind, chainparams): def test_txprepare_feerate(node_factory, bitcoind, chainparams): # Make sure it works at different feerates! - l1, l2 = node_factory.get_nodes(2) + l1, l2 = node_factory.get_nodes(2, opts={'dev-warn-on-overgrind': None, + 'broken_log': 'overgrind: short signature length'}) # Add some funds to withdraw later for i in range(20): @@ -505,7 +517,7 @@ def test_txprepare_feerate(node_factory, bitcoind, chainparams): fee_output = 1 else: fee_output = 0 - if len(bitcoind.rpc.decoderawtransaction(prep['unsigned_tx'])['vout']) == 1 + 1 + fee_output: + if len(bitcoind.rpc.decoderawtransaction(prep['unsigned_tx'])['vout']) == 1 + 1 + fee_output and not did_short_sig(l1): assert actual_feerate < feerate + 2 l1.rpc.txdiscard(prep['txid']) @@ -561,8 +573,7 @@ def test_fundpsbt_feerates(node_factory, bitcoind, chainparams, addrtype): # We never actually added that `amount` output to PSBT, so that appears as "fee" fee = int(txinfo['fees']['base'] * 100_000_000) - amount actual_feerate = fee / (txinfo['weight'] / 1000) - # Out by one errors (due to rounding) change feerate by 2. - assert feerate - 2 < actual_feerate < feerate + 2 + check_feerate(l1, actual_feerate, feerate) def test_reserveinputs(node_factory, bitcoind, chainparams):