added wallet paths to change outputs in funding tx. (#27)

* added wallet paths to change outputs in funding tx.
fixed txid formatting (reversed) in debugging output.
added missing redeemscripts to tx submitted to handle_sign_withdrawal
marked some tests as skip when remote_hsmd because of policies
marked many tests as flaky

* turned off SLOW_MACHINE in run-all-tests

* removed the extra flaky designations

Author: ksedgwic

contrib/remote_hsmd/dump.cc

@@ -44,7 +44,10 @@ string dump_hex(const void *vptr, size_t sz)
 
 string dump_bitcoin_txid(const struct bitcoin_txid *txid)
 {
-	return dump_hex(txid->shad.sha.u.u8, sizeof(txid->shad.sha.u.u8));
+	// reverse the bytes, a-la bitcoind
+	struct sha256_double rev = txid->shad;
+	reverse_bytes(rev.sha.u.u8, sizeof(rev.sha.u.u8));
+	return dump_hex(rev.sha.u.u8, sizeof(rev.sha.u.u8));
 }
 
 string dump_bitcoin_signature(const struct bitcoin_signature *sp)
@@ -213,6 +216,7 @@ string dump_wally_tx_witness_stack(const struct wally_tx_witness_stack *sp)
 string dump_wally_keypath_item(const struct wally_map_item *ip)
 {
 	size_t npath = (ip->value_len - BIP32_KEY_FINGERPRINT_LEN) / sizeof(uint32_t);
+	uint32_t * path = (uint32_t *) (ip->value + BIP32_KEY_FINGERPRINT_LEN);
 	ostringstream ostrm;
 	ostrm << "{ ";
 	ostrm << "\"pubkey\":" << dump_hex(ip->key, ip->key_len);
@@ -223,9 +227,7 @@ string dump_wally_keypath_item(const struct wally_map_item *ip)
 	for (size_t ii = 0; ii < npath; ++ii) {
 		if (ii != 0)
 			ostrm << ",";
-		uint32_t pelem = *(uint32_t *)
-			ip->value + BIP32_KEY_FINGERPRINT_LEN + ii * sizeof(uint32_t);
-		ostrm << pelem;
+		ostrm << le32_to_cpu(path[ii]);
 	}
 	ostrm << " ]";
 	ostrm << " }";
@@ -481,3 +483,15 @@ string dump_rhashes(const struct sha256 *rhashes, size_t num_rhashes)
 	ostrm << "]";
 	return ostrm.str();
 }
+
+/* <sigh>.  Bitcoind represents hashes as little-endian for RPC. */
+void reverse_bytes(u8 *arr, size_t len)
+{
+	unsigned int i;
+
+	for (i = 0; i < len / 2; i++) {
+		unsigned char tmp = arr[i];
+		arr[i] = arr[len - 1 - i];
+		arr[len - 1 - i] = tmp;
+	}
+}

contrib/remote_hsmd/dump.hpp

@@ -38,4 +38,7 @@ std::string dump_wally_psbt(const struct wally_psbt *psbt);
 std::string dump_tx(const struct bitcoin_tx *tx);
 std::string dump_rhashes(const struct sha256 *rhashes, size_t num_rhashes);
 
+// needed for formatting txid
+void reverse_bytes(u8 *arr, size_t len);
+
 #endif /* LIGHTNING_CONTRIB_REMOTE_HSMD_DUMP_H */

contrib/remote_hsmd/hsmd.c

@@ -1288,11 +1288,11 @@ static struct io_plan *handle_ready_channel(struct io_conn *conn,
 		&push_value,
 		&funding_txid,
 		funding_txout,
-		local_to_self_delay,
+		local_to_self_delay,	// locally imposed on counterparty to_self outputs
 		local_shutdown_script,
 		&remote_basepoints,
 		&remote_funding_pubkey,
-		remote_to_self_delay,
+		remote_to_self_delay,	// counterparty imposed on our to_self outputs
 		remote_shutdown_script,
 		option_static_remotekey,
 		option_anchor_outputs);
@@ -1334,7 +1334,7 @@ static struct io_plan *handle_sign_withdrawal_tx(struct io_conn *conn,
 
 	u8 *** wits;
 	proxy_stat rv = proxy_handle_sign_withdrawal_tx(
-		&c->id, c->dbid, outputs, utxos, psbt, &wits);
+		outputs, utxos, psbt, &wits);
 	if (PROXY_PERMANENT(rv))
 		status_failed(STATUS_FAIL_INTERNAL_ERROR,
 			      "proxy_%s failed: %s", __FUNCTION__,

contrib/remote_hsmd/proxy.cc

@@ -9,6 +9,7 @@ extern "C" {
 #include <bitcoin/chainparams.h>
 #include <bitcoin/privkey.h>
 #include <bitcoin/psbt.h>
+#include <bitcoin/script.h>
 #include <bitcoin/short_channel_id.h>
 #include <bitcoin/tx.h>
 #include <common/derive_basepoints.h>
@@ -588,12 +589,12 @@ proxy_stat proxy_handle_ready_channel(
 	req.set_push_value_msat(push_value->millisatoshis);
 	marshal_outpoint(funding_txid,
 			 funding_txout, req.mutable_funding_outpoint());
-	req.set_holder_to_self_delay(holder_to_self_delay);
+	req.set_holder_selected_contest_delay(holder_to_self_delay);
 	marshal_script(holder_shutdown_script,
 		       req.mutable_holder_shutdown_script());
 	marshal_basepoints(counterparty_basepoints, counterparty_funding_pubkey,
 			   req.mutable_counterparty_basepoints());
-	req.set_counterparty_to_self_delay(counterparty_to_self_delay);
+	req.set_counterparty_selected_contest_delay(counterparty_to_self_delay);
 	marshal_script(counterparty_shutdown_script,
 		       req.mutable_counterparty_shutdown_script());
 	if (option_anchor_outputs)
@@ -623,33 +624,58 @@ proxy_stat proxy_handle_ready_channel(
 }
 
 proxy_stat proxy_handle_sign_withdrawal_tx(
-	struct node_id *peer_id,
-	u64 dbid,
 	struct bitcoin_tx_output **outputs,
 	struct utxo **utxos,
 	struct wally_psbt *psbt,
 	u8 ****o_wits)
 {
 	STATUS_DEBUG(
 		"%s:%d %s { "
-		"\"self_id\":%s, \"peer_id\":%s, \"dbid\":%" PRIu64 ", "
+		"\"self_id\":%s, "
 		"\"utxos\":%s, \"outputs\":%s, \"psbt\":%s }",
 		__FILE__, __LINE__, __FUNCTION__,
 		dump_node_id(&self_id).c_str(),
-		dump_node_id(peer_id).c_str(),
-		dbid,
 		dump_utxos((const struct utxo **)utxos).c_str(),
 		dump_bitcoin_tx_outputs(
 			(const struct bitcoin_tx_output **)outputs).c_str(),
 		dump_wally_psbt(psbt).c_str()
 		);
 
 	last_message = "";
+
+	// This code is mimicking psbt_txid at bitcoin/psbt.c:796:
+	//
+	/* You can *almost* take txid of global tx.  But @niftynei thought
+	 * about this far more than me and pointed out that P2SH
+	 * inputs would not be represented, so here we go. */
+	struct wally_tx *tx;
+	tal_wally_start();
+	wally_tx_clone_alloc(psbt->tx, 0, &tx);
+	for (size_t i = 0; i < tx->num_inputs; i++) {
+		if (psbt->inputs[i].final_scriptsig) {
+			wally_tx_set_input_script(tx, i,
+						  psbt->inputs[i].final_scriptsig,
+						  psbt->inputs[i].final_scriptsig_len);
+		} else if (psbt->inputs[i].redeem_script) {
+			u8 *script;
+
+			/* P2SH requires push of the redeemscript, from libwally src */
+			script = tal_arr(tmpctx, u8, 0);
+			script_push_bytes(&script,
+					  psbt->inputs[i].redeem_script,
+					  psbt->inputs[i].redeem_script_len);
+			wally_tx_set_input_script(tx, i, script, tal_bytelen(script));
+		}
+	}
+	tal_wally_end(tal_steal(psbt, tx));
+
+
 	SignFundingTxRequest req;
 	marshal_node_id(&self_id, req.mutable_node_id());
-	marshal_channel_nonce(peer_id, dbid, req.mutable_channel_nonce());
 
-	req.mutable_tx()->set_raw_tx_bytes(serialized_wtx(psbt->tx, true));
+	// Serialize the tx we modified above which includes witscripts.
+	req.mutable_tx()->set_raw_tx_bytes(serialized_wtx(tx, true));
+
 	assert(psbt->tx->num_inputs >= tal_count(utxos));
 	size_t uu = 0;
 	for (size_t ii = 0; ii < psbt->tx->num_inputs; ++ii) {
@@ -664,6 +690,20 @@ proxy_stat proxy_handle_sign_withdrawal_tx(
 	}
 	assert(uu == tal_count(utxos));
 
+	for (size_t ii = 0; ii < psbt->tx->num_outputs; ++ii) {
+		OutputDescriptor *odesc = req.mutable_tx()->add_output_descs();
+		struct wally_map *mp = &psbt->outputs[ii].keypaths;
+		if (mp->num_items == 1) {
+			const struct wally_map_item *ip = &mp->items[0];
+			size_t npath =
+				(ip->value_len - BIP32_KEY_FINGERPRINT_LEN) / sizeof(uint32_t);
+			uint32_t *path = (uint32_t *) (ip->value + BIP32_KEY_FINGERPRINT_LEN);
+			for (size_t jj = 0; jj < npath; ++jj) {
+				odesc->mutable_key_loc()->add_key_path(le32_to_cpu(path[jj]));
+			}
+		}
+	}
+
 	ClientContext context;
 	SignFundingTxReply rsp;
 	Status status = stub->SignFundingTx(&context, req, &rsp);

contrib/remote_hsmd/proxy.hpp

@@ -79,7 +79,6 @@ proxy_stat proxy_handle_ready_channel(
 	bool option_anchor_outputs);
 
 proxy_stat proxy_handle_sign_withdrawal_tx(
-	struct node_id *peer_id, u64 dbid,
 	struct bitcoin_tx_output **outputs,
 	struct utxo **utxos,
 	struct wally_psbt *psbt,

contrib/remote_hsmd/scripts/run-all-tests

@@ -1,14 +1,12 @@
 #!/usr/bin/sh
 
-
-
 SUBDAEMON='hsmd:remote_hsmd' \
 REMOTE_SIGNER_CMD=$(pwd)/../rust-lightning-signer/target/debug/server \
 make \
 -j32 PYTEST_PAR=64 \
 DEVELOPER=1 \
 VALGRIND=0 \
-SLOW_MACHINE=1 \
+SLOW_MACHINE=0 \
 PYTEST_MOREOPTS='--timeout=300 --timeout_method=thread' \
 pytest
 

tests/test_closing.py

@@ -2435,6 +2435,7 @@ def test_permfail_htlc_out(node_factory, bitcoind, executor):
 
 
 @pytest.mark.developer("needs DEVELOPER=1")
+@unittest.skipIf(os.getenv('SUBDAEMON') == 'hsmd:remote_hsmd', "policy: can't withdraw to non-wallet address")
 def test_permfail(node_factory, bitcoind):
     l1, l2 = node_factory.line_graph(2)
 

tests/test_connection.py

@@ -2628,6 +2628,7 @@ def mock_sendrawtransaction(r):
 
 
 @pytest.mark.developer("needs dev_fail")
+@unittest.skipIf(os.getenv('SUBDAEMON') == 'hsmd:remote_hsmd', "policy: can't withdraw to non-wallet address")
 def test_no_fee_estimate(node_factory, bitcoind, executor):
     l1 = node_factory.get_node(start=False, options={'dev-no-fake-fees': True})
 

tests/test_misc.py

@@ -492,6 +492,7 @@ def is_p2wpkh(output):
     assert only_one(fundingtx['vin'])['txid'] == res['wallettxid']
 
 
+@unittest.skipIf(os.getenv('SUBDAEMON') == 'hsmd:remote_hsmd', "policy: can't withdraw to non-wallet address")
 def test_withdraw_misc(node_factory, bitcoind, chainparams):
     def dont_spend_outputs(n, txid):
         """Reserve both outputs (we assume there are two!) in case any our ours, so we don't spend change: wrecks accounting checks"""

tests/test_plugin.py

@@ -2090,6 +2090,7 @@ def test_3847_repro(node_factory, bitcoind):
     l1.rpc.paystatus(i1)
 
 
+@unittest.skipIf(os.getenv('SUBDAEMON') == 'hsmd:remote_hsmd', "trouble managing remotesigner when node killed this way")
 def test_important_plugin(node_factory):
     # Cache it here.
     pluginsdir = os.path.join(os.path.dirname(__file__), "plugins")

tests/test_wallet.py

@@ -19,6 +19,7 @@
 
 
 @unittest.skipIf(TEST_NETWORK != 'regtest', "Test relies on a number of example addresses valid only in regtest")
+@unittest.skipIf(os.getenv('SUBDAEMON') == 'hsmd:remote_hsmd', "policy: can't withdraw to non-wallet address")
 def test_withdraw(node_factory, bitcoind):
     amount = 1000000
     # Don't get any funds from previous runs.
@@ -672,6 +673,7 @@ def test_utxopsbt(node_factory, bitcoind, chainparams):
                     reservedok=True)
 
 
+@unittest.skipIf(os.getenv('SUBDAEMON') == 'hsmd:remote_hsmd', "policy: can't withdraw to non-wallet address")
 def test_sign_and_send_psbt(node_factory, bitcoind, chainparams):
     """
     Tests for the sign + send psbt RPCs
@@ -874,6 +876,7 @@ def test_sign_and_send_psbt(node_factory, bitcoind, chainparams):
     check_coin_moves(l1, 'wallet', wallet_coin_mvts, chainparams)
 
 
+@unittest.skipIf(os.getenv('SUBDAEMON') == 'hsmd:remote_hsmd', "policy: can't withdraw to non-wallet address")
 def test_txsend(node_factory, bitcoind, chainparams):
     amount = 1000000
     l1 = node_factory.get_node(random_hsm=True)
@@ -1291,6 +1294,7 @@ def test_withdraw_nlocktime_fuzz(node_factory, bitcoind):
         raise Exception("No transaction with fuzzed nLockTime !")
 
 
+@unittest.skipIf(os.getenv('SUBDAEMON') == 'hsmd:remote_hsmd', "policy: can't withdraw to non-wallet address")
 def test_multiwithdraw_simple(node_factory, bitcoind):
     """
     Test simple multiwithdraw usage.

wallet/wallet.c

@@ -681,6 +681,30 @@ bool wallet_can_spend(struct wallet *w, const u8 *script,
 	return false;
 }
 
+void wallet_set_keypath(struct wallet *w, u32 index, struct wally_map *map_in)
+{
+	log_debug(w->log, "wallet_set_keypath index=%d", index);
+
+	struct ext_key ext;
+	if (bip32_key_from_parent(w->bip32_base, index, BIP32_FLAG_KEY_PUBLIC, &ext) != WALLY_OK) {
+		abort();
+	}
+
+	u8 fingerprint[BIP32_KEY_FINGERPRINT_LEN];
+	if (bip32_key_get_fingerprint(&ext, fingerprint, sizeof(fingerprint)) != WALLY_OK) {
+		abort();
+	}
+
+	u32 path[1];
+	path[0] = index;
+	if (wally_map_add_keypath_item(map_in,
+				       ext.pub_key, sizeof(ext.pub_key),
+				       fingerprint, sizeof(fingerprint),
+				       path, 1) != WALLY_OK) {
+		abort();
+	}
+}
+
 s64 wallet_get_newindex(struct lightningd *ld)
 {
 	u64 newidx = db_get_intvar(ld->wallet->db, "bip32_max_index", 0) + 1;

wallet/wallet.h

@@ -463,6 +463,11 @@ void wallet_confirm_utxos(struct wallet *w, const struct utxo **utxos);
 bool wallet_can_spend(struct wallet *w, const u8 *script,
 		      u32 *index, bool *output_is_p2sh);
 
+/**
+ * wallet_set_keypath - set the keypath to the given wallet entry.
+ */
+void wallet_set_keypath(struct wallet *w, u32 index, struct wally_map *map_in);
+
 /**
  * wallet_get_newindex - get a new index from the wallet.
  * @ld: (in) lightning daemon

wallet/walletrpc.c

@@ -660,6 +660,30 @@ static struct command_result *match_psbt_inputs_to_utxos(struct command *cmd,
 	return NULL;
 }
 
+static struct command_result *match_psbt_outputs_to_wallet(struct wally_psbt *psbt,
+							   struct wallet *w)
+{
+	assert(psbt->tx->num_outputs == psbt->num_outputs);
+	tal_wally_start();
+	for (size_t outndx = 0; outndx < psbt->num_outputs; ++outndx) {
+		u32 index;
+		bool is_p2sh;
+		const u8 *script;
+
+		script = wally_tx_output_get_script(tmpctx,
+						    &psbt->tx->outputs[outndx]);
+		if (!script)
+			continue;
+
+		if (!wallet_can_spend(w, script, &index, &is_p2sh))
+			continue;
+
+		wallet_set_keypath(w, index, &psbt->outputs[outndx].keypaths);
+	}
+	tal_wally_end(psbt);
+	return NULL;
+}
+
 static struct command_result *param_input_numbers(struct command *cmd,
 						  const char *name,
 						  const char *buffer,
@@ -723,6 +747,11 @@ static struct command_result *json_signpsbt(struct command *cmd,
 		return command_fail(cmd, LIGHTNINGD,
 				    "No wallet inputs to sign");
 
+	// Update the keypaths on any outputs that are in our wallet (change addresses).
+	res = match_psbt_outputs_to_wallet(psbt, cmd->ld->wallet);
+	if (res)
+		return res;
+
 	/* FIXME: hsm will sign almost anything, but it should really
 	 * fail cleanly (not abort!) and let us report the error here. */
 	u8 *msg = towire_hsmd_sign_withdrawal(cmd,