From e96eb07ef417a260bc8edd2a7202d83dbad61b9d Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 20 Jul 2022 11:58:25 +0930 Subject: [PATCH] lightningd: test that hsm_secret is as expected, at startup. If you get the wrong hsm_secret, your node_id will change, and peers won't know who you are, bitcoind will reject your transaction signatures, and other madness. Catch this as soon as it happens, by storing our node_id in the db. Suggested-by: @cdecker, @fiatjaf Signed-off-by: Rusty Russell Changelog-Changed: Config: `lightningd` will refuse to start with the wrong node_id (i.e. hsm_secret changes). --- lightningd/lightningd.c | 9 +++++--- lightningd/test/run-find_my_abspath.c | 6 +++--- tests/test_db.py | 29 +++++++++++++++++++++++++- tests/test_wallet.py | 4 ++-- wallet/wallet.c | 30 ++++++++++++++++++++++++++- wallet/wallet.h | 10 +++++---- 6 files changed, 74 insertions(+), 14 deletions(-) diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index e6b70c675..07d79128f 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -1048,9 +1048,12 @@ int main(int argc, char *argv[]) /*~ Our default names, eg. for the database file, are not dependent on * the network. Instead, the db knows what chain it belongs to, and we - * simple barf here if it's wrong. */ - if (!wallet_network_check(ld->wallet)) - errx(1, "Wallet network check failed."); + * simple barf here if it's wrong. + * + * We also check that our node_id is what we expect: otherwise a change + * in hsm_secret will have strange consequences! */ + if (!wallet_sanity_check(ld->wallet)) + errx(1, "Wallet sanity check failed."); /*~ Initialize the transaction filter with our pubkeys. */ init_txfilter(ld->wallet, ld->owned_txfilter); diff --git a/lightningd/test/run-find_my_abspath.c b/lightningd/test/run-find_my_abspath.c index 278c6d0c7..585eb5328 100644 --- a/lightningd/test/run-find_my_abspath.c +++ b/lightningd/test/run-find_my_abspath.c @@ -223,13 +223,13 @@ void waitblockheight_notify_new_block(struct lightningd *ld UNNEEDED, /* Generated stub for wallet_blocks_heights */ void wallet_blocks_heights(struct wallet *w UNNEEDED, u32 def UNNEEDED, u32 *min UNNEEDED, u32 *max UNNEEDED) { fprintf(stderr, "wallet_blocks_heights called!\n"); abort(); } -/* Generated stub for wallet_network_check */ -bool wallet_network_check(struct wallet *w UNNEEDED) -{ fprintf(stderr, "wallet_network_check called!\n"); abort(); } /* Generated stub for wallet_new */ struct wallet *wallet_new(struct lightningd *ld UNNEEDED, struct timers *timers UNNEEDED, struct ext_key *bip32_base UNNEEDED) { fprintf(stderr, "wallet_new called!\n"); abort(); } +/* Generated stub for wallet_sanity_check */ +bool wallet_sanity_check(struct wallet *w UNNEEDED) +{ fprintf(stderr, "wallet_sanity_check called!\n"); abort(); } /* AUTOGENERATED MOCKS END */ struct log *crashlog; diff --git a/tests/test_db.py b/tests/test_db.py index 95ae6fad2..1c4b7c35e 100644 --- a/tests/test_db.py +++ b/tests/test_db.py @@ -2,7 +2,7 @@ from decimal import Decimal from fixtures import * # noqa: F401,F403 from fixtures import TEST_NETWORK from pyln.client import RpcError -from utils import wait_for, sync_blockheight, COMPAT, VALGRIND, DEVELOPER, only_one +from utils import wait_for, sync_blockheight, COMPAT, VALGRIND, DEVELOPER, TIMEOUT, only_one import base64 import os @@ -413,3 +413,30 @@ def test_sqlite3_builtin_backup(bitcoind, node_factory): # Should still see the funds. assert(len(l1.rpc.listfunds()['outputs']) == 1) + + +@unittest.skipIf(os.getenv('TEST_DB_PROVIDER', 'sqlite3') != 'sqlite3', "Don't know how to swap dbs in Postgres") +def test_db_sanity_checks(bitcoind, node_factory): + l1, l2 = node_factory.get_nodes(2, opts=[{'allow_broken_log': True, + 'may_fail': True}, {}]) + + l1.stop() + l2.stop() + + # Provide the --wallet option and start with wrong db + l1.daemon.opts['wallet'] = "sqlite3://" + l2.db.path + l1.daemon.start(wait_for_initialized=False) + l1.daemon.wait_for_log(r'\*\*BROKEN\*\* wallet: Wallet node_id does not match HSM') + # Will have exited with non-zero status. + assert l1.daemon.proc.wait(TIMEOUT) != 0 + assert l1.daemon.is_in_stderr('Wallet sanity check failed') + + # Now try wrong network, + l1.daemon.opts['wallet'] = "sqlite3://" + l1.db.path + l1.daemon.opts['network'] = "bitcoin" + + l1.daemon.start(wait_for_initialized=False) + l1.daemon.wait_for_log(r'\*\*BROKEN\*\* wallet: Wallet blockchain hash does not match network blockchain hash') + # Will have exited with non-zero status. + assert l1.daemon.proc.wait(TIMEOUT) != 0 + assert l1.daemon.is_in_stderr('Wallet sanity check failed') diff --git a/tests/test_wallet.py b/tests/test_wallet.py index 893d2f6e0..c50c39b40 100644 --- a/tests/test_wallet.py +++ b/tests/test_wallet.py @@ -1220,8 +1220,7 @@ def test_hsmtool_dump_descriptors(node_factory, bitcoind): @unittest.skipIf(VALGRIND, "It does not play well with prompt and key derivation.") def test_hsmtool_generatehsm(node_factory): - l1 = node_factory.get_node() - l1.stop() + l1 = node_factory.get_node(start=False) hsm_path = os.path.join(l1.daemon.lightning_dir, TEST_NETWORK, "hsm_secret") @@ -1248,6 +1247,7 @@ def test_hsmtool_generatehsm(node_factory): # We can start the node with this hsm_secret l1.start() + assert l1.info['id'] == '02244b73339edd004bc6dfbb953a87984c88e9e7c02ca14ef6ec593ca6be622ba7' # this test does a 'listtransactions' on a yet unconfirmed channel diff --git a/wallet/wallet.c b/wallet/wallet.c index b9d670ad2..aae846635 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -3643,7 +3643,7 @@ void wallet_htlc_sigs_save(struct wallet *w, u64 channel_id, } } -bool wallet_network_check(struct wallet *w) +bool wallet_sanity_check(struct wallet *w) { struct bitcoin_blkid chainhash; struct db_stmt *stmt = db_prepare_v2( @@ -3676,6 +3676,34 @@ bool wallet_network_check(struct wallet *w) db_bind_sha256d(stmt, 0, &chainparams->genesis_blockhash.shad); db_exec_prepared_v2(take(stmt)); } + + stmt = db_prepare_v2(w->db, + SQL("SELECT blobval FROM vars WHERE name='node_id'")); + db_query_prepared(stmt); + + if (db_step(stmt)) { + struct node_id id; + db_col_node_id(stmt, "blobval", &id); + tal_free(stmt); + + if (!node_id_eq(&id, &w->ld->id)) { + log_broken(w->log, "Wallet node_id does not " + "match HSM: %s " + "!= %s. " + "Did your hsm_secret change?", + type_to_string(tmpctx, struct node_id, &id), + type_to_string(tmpctx, struct node_id, + &w->ld->id)); + return false; + } + } else { + tal_free(stmt); + /* Still a pristine wallet, claim it for the node_id we are now */ + stmt = db_prepare_v2(w->db, SQL("INSERT INTO vars (name, blobval) " + "VALUES ('node_id', ?);")); + db_bind_node_id(stmt, 0, &w->ld->id); + db_exec_prepared_v2(take(stmt)); + } return true; } diff --git a/wallet/wallet.h b/wallet/wallet.h index 4003adc74..425c8637d 100644 --- a/wallet/wallet.h +++ b/wallet/wallet.h @@ -1198,13 +1198,15 @@ void wallet_htlc_sigs_save(struct wallet *w, u64 channel_id, const struct bitcoin_signature *htlc_sigs); /** - * wallet_network_check - Check that the wallet is setup for this chain + * wallet_sanity_check - Check that the wallet is setup for this node_id and chain * * Ensure that the genesis_hash from the chainparams matches the - * genesis_hash with which the DB was initialized. Returns false if - * the check failed, i.e., if the genesis hashes do not match. + * genesis_hash with which the DB was initialized, and that the HSM + * gave us the same node_id as the one is the db. + * + * Returns false if the checks failed. */ -bool wallet_network_check(struct wallet *w); +bool wallet_sanity_check(struct wallet *w); /** * wallet_block_add - Add a block to the blockchain tracked by this wallet