Skip to content

Commit 992771b

Browse files
rustyrussellrustyrussell
committed
pytest: fix flakes in feerates.
Sometimes we get a shorter signature than expected, and we're supposed to disable the feerate tests in that case. But we didn't catch all the cases where we make signatures. ``` 2025-10-13T02:36:24.1901527Z ____________________________ test_peer_anchor_push _____________________________ 2025-10-13T02:36:24.1902251Z [gw5] linux -- Python 3.10.18 /home/runner/work/lightning/lightning/.venv/bin/python 2025-10-13T02:36:24.1902731Z 2025-10-13T02:36:24.1903045Z node_factory = <pyln.testing.utils.NodeFactory object at 0x7f17fdffd6f0> 2025-10-13T02:36:24.1903740Z bitcoind = <pyln.testing.utils.BitcoinD object at 0x7f17fdffe830> 2025-10-13T02:36:24.1904495Z executor = <concurrent.futures.thread.ThreadPoolExecutor object at 0x7f17fdffcbb0> 2025-10-13T02:36:24.1906158Z chainparams = {'bip173_prefix': 'bcrt', 'chain_hash': '06226e46111a0b59caaf126043eb5bbf28c34f3a5e332a1fc7b2b73cf188910f', 'elements': False, 'example_addr': 'bcrt1qeyyk6sl5pr49ycpqyckvmttus5ttj25pd0zpvg', ...} 2025-10-13T02:36:24.1907285Z 2025-10-13T02:36:24.1907600Z @unittest.skipIf(TEST_NETWORK != 'regtest', 'elementsd anchors not supportd') 2025-10-13T02:36:24.1908344Z def test_peer_anchor_push(node_factory, bitcoind, executor, chainparams): 2025-10-13T02:36:24.1909033Z """Test that we use anchor on peer's commit to CPFP tx""" 2025-10-13T02:36:24.1909853Z l1, l2, l3 = node_factory.line_graph(3, opts=[{}, 2025-10-13T02:36:24.1910334Z {'min-emergency-msat': 546000, 2025-10-13T02:36:24.1910825Z 'dev-warn-on-overgrind': None, 2025-10-13T02:36:24.1911313Z 'broken_log': 'overgrind: short signature length'}, 2025-10-13T02:36:24.1911662Z {'disconnect': ['-WIRE_UPDATE_FULFILL_HTLC'], 2025-10-13T02:36:24.1911976Z 'dev-warn-on-overgrind': None, 2025-10-13T02:36:24.1912304Z 'broken_log': 'overgrind: short signature length'}], 2025-10-13T02:36:24.1912790Z wait_for_announce=True) 2025-10-13T02:36:24.1913041Z 2025-10-13T02:36:24.1913305Z # We splinter l2's funds so it's forced to use more than one UTXO to push. 2025-10-13T02:36:24.1914043Z fundsats = int(Millisatoshi(only_one(l2.rpc.listfunds()['outputs'])['amount_msat']).to_satoshi()) 2025-10-13T02:36:24.1914443Z OUTPUT_SAT = 10000 2025-10-13T02:36:24.1914647Z NUM_OUTPUTS = 10 2025-10-13T02:36:24.1914903Z psbt = l2.rpc.fundpsbt("all", "1000perkw", 1000)['psbt'] 2025-10-13T02:36:24.1915520Z # Pay 5k sats in fees. 2025-10-13T02:36:24.1916329Z psbt = l2.rpc.addpsbtoutput(fundsats - OUTPUT_SAT * NUM_OUTPUTS - 5000, psbt, destination=l3.rpc.newaddr()['bech32'])['psbt'] 2025-10-13T02:36:24.1917156Z for _ in range(NUM_OUTPUTS): 2025-10-13T02:36:24.1917638Z psbt = l2.rpc.addpsbtoutput(OUTPUT_SAT, psbt)['psbt'] 2025-10-13T02:36:24.1918194Z l2.rpc.sendpsbt(l2.rpc.signpsbt(psbt)['signed_psbt']) 2025-10-13T02:36:24.1918731Z bitcoind.generate_block(1, wait_for_mempool=1) 2025-10-13T02:36:24.1919232Z sync_blockheight(bitcoind, [l1, l2]) 2025-10-13T02:36:24.1919634Z 2025-10-13T02:36:24.1919847Z # Make sure all amounts are below OUTPUT_SAT sats! 2025-10-13T02:36:24.1920318Z assert [x for x in l2.rpc.listfunds()['outputs'] if x['amount_msat'] > Millisatoshi(str(OUTPUT_SAT) + "sat")] == [] 2025-10-13T02:36:24.1920735Z 2025-10-13T02:36:24.1920957Z # Get HTLC stuck, so l2 has reason to push commitment tx. 2025-10-13T02:36:24.1921244Z amt = 100_000_000 2025-10-13T02:36:24.1921517Z sticky_inv = l3.rpc.invoice(amt, 'sticky', 'sticky') 2025-10-13T02:36:24.1921842Z route = l1.rpc.getroute(l3.info['id'], amt, 1)['route'] 2025-10-13T02:36:24.1922277Z l1.rpc.sendpay(route, sticky_inv['payment_hash'], payment_secret=sticky_inv['payment_secret']) 2025-10-13T02:36:24.1922751Z l3.daemon.wait_for_log('dev_disconnect: -WIRE_UPDATE_FULFILL_HTLC') 2025-10-13T02:36:24.1923060Z 2025-10-13T02:36:24.1923241Z # Make sure HTLC expiry is what we expect! 2025-10-13T02:36:24.1923637Z l2.daemon.wait_for_log('Adding HTLC 0 amount=100000000msat cltv=119 gave CHANNEL_ERR_ADD_OK') 2025-10-13T02:36:24.1923998Z 2025-10-13T02:36:24.1924229Z # l3 drops to chain, but make sure it doesn't CPFP its own anchor. 2025-10-13T02:36:24.1924685Z wait_for(lambda: only_one(l3.rpc.listpeerchannels(l2.info['id'])['channels'])['htlcs'] != []) 2025-10-13T02:36:24.1925688Z closetx = l3.rpc.dev_sign_last_tx(l2.info['id'])['tx'] 2025-10-13T02:36:24.1926132Z l3.stop() 2025-10-13T02:36:24.1926337Z # We don't care about l1 any more, either 2025-10-13T02:36:24.1926579Z l1.stop() 2025-10-13T02:36:24.1926739Z 2025-10-13T02:36:24.1926941Z # We put l3's tx in the mempool, but won't mine it. 2025-10-13T02:36:24.1927316Z bitcoind.rpc.sendrawtransaction(closetx) 2025-10-13T02:36:24.1927754Z 2025-10-13T02:36:24.1928139Z # We aim for feerate ~3750, so this won't mine l3's unilateral close. 2025-10-13T02:36:24.1928991Z # HTLC's going to time out at block 120 (we give one block grace) 2025-10-13T02:36:24.1929527Z for block in range(110, 120): 2025-10-13T02:36:24.1929989Z bitcoind.generate_block(1, needfeerate=5000) 2025-10-13T02:36:24.1930510Z assert bitcoind.rpc.getblockcount() == block 2025-10-13T02:36:24.1931014Z sync_blockheight(bitcoind, [l2]) 2025-10-13T02:36:24.1931715Z assert only_one(l2.rpc.listpeerchannels(l3.info['id'])['channels'])['state'] == 'CHANNELD_NORMAL' 2025-10-13T02:36:24.1932368Z 2025-10-13T02:36:24.1932642Z # Drops to chain 2025-10-13T02:36:24.1933030Z bitcoind.generate_block(1, needfeerate=5000) 2025-10-13T02:36:24.1933824Z wait_for(lambda: only_one(l2.rpc.listpeerchannels(l3.info['id'])['channels'])['state'] == 'AWAITING_UNILATERAL') 2025-10-13T02:36:24.1934522Z 2025-10-13T02:36:24.1935254Z # But, l3's tx already there, and identical feerate will not RBF. 2025-10-13T02:36:24.1935839Z l2.daemon.wait_for_log("rejecting replacement") 2025-10-13T02:36:24.1936357Z wait_for(lambda: len(bitcoind.rpc.getrawmempool()) == 2) 2025-10-13T02:36:24.1936801Z 2025-10-13T02:36:24.1937127Z # As blocks pass, we will use anchor to boost l3's tx. 2025-10-13T02:36:24.1937666Z for block, feerate in zip(range(120, 124), (12000, 13000, 14000, 15000)): 2025-10-13T02:36:24.1938560Z l2.daemon.wait_for_log(fr"Worth fee [0-9]*sat for remote commit tx to get 100000000msat at block 125 \(\+{125 - block}\) at feerate {feerate}perkw") 2025-10-13T02:36:24.1939369Z l2.daemon.wait_for_log("sendrawtx exit 0") 2025-10-13T02:36:24.1939902Z # Check feerate for entire package (commitment tx + anchor) is ~ correct 2025-10-13T02:36:24.1940473Z details = bitcoind.rpc.getrawmempool(True).values() 2025-10-13T02:36:24.1940920Z print(f"mempool = {details}") 2025-10-13T02:36:24.1941347Z total_weight = sum([d['weight'] for d in details]) 2025-10-13T02:36:24.1941903Z total_fees = sum([float(d['fees']['base']) * 100_000_000 for d in details]) 2025-10-13T02:36:24.1942467Z total_feerate_perkw = total_fees / total_weight * 1000 2025-10-13T02:36:24.1942972Z > check_feerate([l3, l2], total_feerate_perkw, feerate) 2025-10-13T02:36:24.1943279Z 2025-10-13T02:36:24.1943411Z tests/test_closing.py:4064: 2025-10-13T02:36:24.1943813Z _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 2025-10-13T02:36:24.1944170Z 2025-10-13T02:36:24.1944685Z nodes = [<fixtures.LightningNode object at 0x7f17fdf684f0>, <fixtures.LightningNode object at 0x7f17fdf6af50>] 2025-10-13T02:36:24.1945708Z actual_feerate = 13005.66942869603, expected_feerate = 13000 2025-10-13T02:36:24.1946091Z 2025-10-13T02:36:24.1946247Z def check_feerate(nodes, actual_feerate, expected_feerate): 2025-10-13T02:36:24.1946558Z # Feerate can't be lower. 2025-10-13T02:36:24.1946870Z assert actual_feerate > expected_feerate - 2 2025-10-13T02:36:24.1947365Z if actual_feerate >= expected_feerate + 2: 2025-10-13T02:36:24.1947830Z if any([did_short_sig(n) for n in nodes]): 2025-10-13T02:36:24.1948239Z return 2025-10-13T02:36:24.1948589Z # Use assert as it shows the actual values on failure 2025-10-13T02:36:24.1949043Z > assert actual_feerate < expected_feerate + 2 2025-10-13T02:36:24.1949489Z E AssertionError 2025-10-13T02:36:24.1949704Z ``` Signed-off-by: Rusty Russell <[email protected]>
1 parent fc6bee8 commit 992771b

File tree

1 file changed

+19
-0
lines changed

1 file changed

+19
-0
lines changed

‎hsmd/libhsmd.c‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -606,6 +606,18 @@ static void sign_our_inputs(struct hsm_utxo **utxos, struct wally_psbt *psbt)
606606
}
607607
}
608608

609+
static void check_overgrind(const struct bitcoin_signature *sig)
610+
{
611+
u8 der[73];
612+
size_t len;
613+
614+
if (!dev_warn_on_overgrind)
615+
return;
616+
len = signature_to_der(der, sig);
617+
if (len != 71)
618+
hsmd_status_broken("overgrind: short signature length %zu", len);
619+
}
620+
609621
/*~ This covers several cases where onchaind is creating a transaction which
610622
* sends funds to our internal wallet. */
611623
/* FIXME: Derive output address for this client, and check it here! */
@@ -632,6 +644,7 @@ static u8 *handle_sign_to_us_tx(struct hsmd_client *c, const u8 *msg_in,
632644
return hsmd_status_bad_request(c, msg_in, "bad txinput count");
633645

634646
sign_tx_input(tx, 0, NULL, wscript, privkey, &pubkey, sighash_type, &sig);
647+
check_overgrind(&sig);
635648

636649
return towire_hsmd_sign_tx_reply(NULL, &sig);
637650
}
@@ -1401,6 +1414,7 @@ static u8 *handle_sign_mutual_close_tx(struct hsmd_client *c, const u8 *msg_in)
14011414
&secrets.funding_privkey,
14021415
&local_funding_pubkey,
14031416
SIGHASH_ALL, &sig);
1417+
check_overgrind(&sig);
14041418

14051419
return towire_hsmd_sign_tx_reply(NULL, &sig);
14061420
}
@@ -1435,6 +1449,7 @@ static u8 *handle_sign_splice_tx(struct hsmd_client *c, const u8 *msg_in)
14351449
&secrets.funding_privkey,
14361450
&local_funding_pubkey,
14371451
SIGHASH_ALL, &sig);
1452+
check_overgrind(&sig);
14381453

14391454
return towire_hsmd_sign_tx_reply(NULL, &sig);
14401455
}
@@ -1513,6 +1528,7 @@ static u8 *do_sign_local_htlc_tx(struct hsmd_client *c,
15131528
? (SIGHASH_SINGLE|SIGHASH_ANYONECANPAY)
15141529
: SIGHASH_ALL,
15151530
&sig);
1531+
check_overgrind(&sig);
15161532

15171533
return towire_hsmd_sign_tx_reply(NULL, &sig);
15181534
}
@@ -1605,6 +1621,7 @@ static u8 *handle_sign_remote_htlc_tx(struct hsmd_client *c, const u8 *msg_in)
16051621
option_anchor_outputs
16061622
? (SIGHASH_SINGLE|SIGHASH_ANYONECANPAY)
16071623
: SIGHASH_ALL, &sig);
1624+
check_overgrind(&sig);
16081625

16091626
return towire_hsmd_sign_tx_reply(NULL, &sig);
16101627
}
@@ -1662,6 +1679,7 @@ static u8 *handle_sign_remote_commitment_tx(struct hsmd_client *c, const u8 *msg
16621679
&local_funding_pubkey,
16631680
SIGHASH_ALL,
16641681
&sig);
1682+
check_overgrind(&sig);
16651683

16661684
return towire_hsmd_sign_tx_reply(NULL, &sig);
16671685
}
@@ -1855,6 +1873,7 @@ static u8 *handle_sign_commitment_tx(struct hsmd_client *c, const u8 *msg_in)
18551873
&local_funding_pubkey,
18561874
SIGHASH_ALL,
18571875
&sig);
1876+
check_overgrind(&sig);
18581877

18591878
return towire_hsmd_sign_commitment_tx_reply(NULL, &sig);
18601879
}

0 commit comments

Comments
 (0)