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 <[email protected]>

Author: rustyrussell

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

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);
 }

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 <bitcoin/chainparams.h>
 # Start the HSM.

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);
 		}
 	}

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 */

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))

lightningd/lightningd.c

@@ -151,6 +151,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;

lightningd/lightningd.h

@@ -353,6 +353,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;

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,

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,17 +406,15 @@ 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
     utxos = [utxo["txid"] + ":" + str(utxo["output"])
              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,15 +455,15 @@ 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'}])
 
     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):