From 52303029aa93dd052e627d07acbdb477c5f8d677 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sun, 29 Jul 2018 15:21:38 +0930 Subject: [PATCH] fundchannel: cap 'all' at 2^24-1 satoshi. The easiest way to do this is to play with the 'wallet_tx' semantics and have 'amount' have meaning even when 'all_funds' is set. Note that we change the string 'Cannot afford funding transaction' to 'Cannot afford transaction' as this code is also used for withdrawls. Inspired-by: molz on #c-lightning Signed-off-by: Rusty Russell --- CHANGELOG.md | 1 + common/wallet_tx.c | 47 ++++++++++++++++++++------------- common/wallet_tx.h | 2 +- doc/lightning-fundchannel.7 | 6 ++--- doc/lightning-fundchannel.7.txt | 6 ++--- lightningd/jsonrpc.c | 9 +++++-- lightningd/jsonrpc.h | 2 +- lightningd/opening_control.c | 9 ++----- lightningd/test/run-param.c | 2 +- tests/test_lightningd.py | 17 +++++++++--- wallet/walletrpc.c | 2 +- 11 files changed, 62 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5114617fb..53569a987 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,7 @@ changes. - Protocol: `node_announcement` multiple addresses are correctly ordered and uniquified. - JSON API: `listnodes`: now displays node aliases and colors even if they don't advertise a network address +- JSON API: `fundchannel all`: now restricts to 2^24-1 satoshis rather than failing. - When we reconnect and have to retransmit failing HTLCs, the errors weren't encrypted by us. - `lightningd_config` man page is now installed by `make install`. diff --git a/common/wallet_tx.c b/common/wallet_tx.c index 7e51b5dc8..46c9ecb86 100644 --- a/common/wallet_tx.c +++ b/common/wallet_tx.c @@ -12,17 +12,17 @@ void wtx_init(struct command *cmd, struct wallet_tx * wtx) wtx->all_funds = false; } -static bool check_amount(const struct wallet_tx *tx) +static bool check_amount(const struct wallet_tx *tx, u64 amount) { if (!tx->utxos) { command_fail(tx->cmd, FUND_CANNOT_AFFORD, - "Cannot afford funding transaction"); + "Cannot afford transaction"); return false; } - if (tx->amount < 546) { + if (amount < 546) { command_fail(tx->cmd, FUND_OUTPUT_IS_DUST, "Output %"PRIu64" satoshis would be dust", - tx->amount); + amount); return false; } return true; @@ -33,28 +33,37 @@ bool wtx_select_utxos(struct wallet_tx * tx, u32 fee_rate_per_kw, { u64 fee_estimate; if (tx->all_funds) { + u64 amount; tx->utxos = wallet_select_all(tx->cmd, tx->cmd->ld->wallet, fee_rate_per_kw, out_len, - &tx->amount, + &amount, &fee_estimate); - if (!check_amount(tx)) + if (!check_amount(tx, amount)) return false; - tx->change = 0; - } else { - tx->utxos = wallet_select_coins(tx->cmd, tx->cmd->ld->wallet, - tx->amount, - fee_rate_per_kw, out_len, - &fee_estimate, &tx->change); - if (!check_amount(tx)) - return false; - - if (tx->change < 546) { + if (amount <= tx->amount) { tx->change = 0; - tx->change_key_index = 0; - } else { - tx->change_key_index = wallet_get_newindex(tx->cmd->ld); + tx->amount = amount; + return true; } + + /* Too much? Try again, but ask for limit instead. */ + tx->all_funds = false; + tx->utxos = tal_free(tx->utxos); + } + + tx->utxos = wallet_select_coins(tx->cmd, tx->cmd->ld->wallet, + tx->amount, + fee_rate_per_kw, out_len, + &fee_estimate, &tx->change); + if (!check_amount(tx, tx->amount)) + return false; + + if (tx->change < 546) { + tx->change = 0; + tx->change_key_index = 0; + } else { + tx->change_key_index = wallet_get_newindex(tx->cmd->ld); } return true; } diff --git a/common/wallet_tx.h b/common/wallet_tx.h index 716fac36a..97027005c 100644 --- a/common/wallet_tx.h +++ b/common/wallet_tx.h @@ -16,7 +16,7 @@ struct wallet_tx { u32 change_key_index; const struct utxo **utxos; - bool all_funds; + bool all_funds; /* In this case, amount is a maximum. */ }; void wtx_init(struct command *cmd, struct wallet_tx *wtx); diff --git a/doc/lightning-fundchannel.7 b/doc/lightning-fundchannel.7 index 13addecba..68091a8bb 100644 --- a/doc/lightning-fundchannel.7 +++ b/doc/lightning-fundchannel.7 @@ -2,12 +2,12 @@ .\" Title: lightning-fundchannel .\" Author: [FIXME: author] [see http://docbook.sf.net/el/author] .\" Generator: DocBook XSL Stylesheets v1.79.1 -.\" Date: 06/17/2018 +.\" Date: 07/29/2018 .\" Manual: \ \& .\" Source: \ \& .\" Language: English .\" -.TH "LIGHTNING\-FUNDCHANN" "7" "06/17/2018" "\ \&" "\ \&" +.TH "LIGHTNING\-FUNDCHANN" "7" "07/29/2018" "\ \&" "\ \&" .\" ----------------------------------------------------------------- .\" * Define some portability stuff .\" ----------------------------------------------------------------- @@ -38,7 +38,7 @@ The \fBfundchannel\fR RPC command opens a payment channel with a peer by commiti .sp \fIid\fR is the peer id obtained from \fBconnect\fR\&. .sp -\fIsatoshi\fR is the amount in satoshis taken from the internal wallet to fund the channel\&. The string \fIall\fR can be used to specify all available funds\&. This value cannot be less than the dust limit, currently set to 546\&. And it must be less than 1<<24 (approximately 0\&.16778 BTC)\&. +\fIsatoshi\fR is the amount in satoshis taken from the internal wallet to fund the channel\&. The string \fIall\fR can be used to specify all available funds (or 16777215 satoshi if more is available)\&. The value cannot be less than the dust limit, currently set to 546, nor more than 16777215 satoshi\&. .SH "RETURN VALUE" .sp On success, the \fItx\fR and \fItxid\fR of the transaction is returned, as well as the \fIchannel_id\fR of the newly created channel\&. On failure, an error is reported and the channel is not funded\&. diff --git a/doc/lightning-fundchannel.7.txt b/doc/lightning-fundchannel.7.txt index ff3a5686b..d9c8b1db7 100644 --- a/doc/lightning-fundchannel.7.txt +++ b/doc/lightning-fundchannel.7.txt @@ -23,9 +23,9 @@ for the channel. 'id' is the peer id obtained from *connect*. 'satoshi' is the amount in satoshis taken from the internal wallet to fund the channel. -The string 'all' can be used to specify all available funds. -This value cannot be less than the dust limit, currently set to 546. -And it must be less than 1<<24 (approximately 0.16778 BTC). +The string 'all' can be used to specify all available funds (or 16777215 satoshi if more is available). +The value cannot be less than the dust limit, currently set to 546, nor more +than 16777215 satoshi. RETURN VALUE ------------ diff --git a/lightningd/jsonrpc.c b/lightningd/jsonrpc.c index 6dc1e4ff8..cce2eb79b 100644 --- a/lightningd/jsonrpc.c +++ b/lightningd/jsonrpc.c @@ -775,14 +775,19 @@ bool json_tok_newaddr(const char *buffer, const jsmntok_t *tok, bool *is_p2wpkh) } bool json_tok_wtx(struct wallet_tx * tx, const char * buffer, - const jsmntok_t *sattok) + const jsmntok_t *sattok, u64 max) { if (json_tok_streq(buffer, sattok, "all")) { tx->all_funds = true; + tx->amount = max; } else if (!json_tok_u64(buffer, sattok, &tx->amount)) { command_fail(tx->cmd, JSONRPC2_INVALID_PARAMS, "Invalid satoshis"); return false; - } + } else if (tx->amount > max) { + command_fail(tx->cmd, FUND_MAX_EXCEEDED, + "Amount exceeded %"PRIu64, max); + return false; + } return true; } diff --git a/lightningd/jsonrpc.h b/lightningd/jsonrpc.h index eaf12ec40..1960cb472 100644 --- a/lightningd/jsonrpc.h +++ b/lightningd/jsonrpc.h @@ -99,7 +99,7 @@ bool json_tok_newaddr(const char *buffer, const jsmntok_t *tok, bool *is_p2wpkh) /* Parse the satoshi token in wallet_tx. */ bool json_tok_wtx(struct wallet_tx * tx, const char * buffer, - const jsmntok_t * sattok); + const jsmntok_t * sattok, u64 max); AUTODATA_TYPE(json_command, struct json_command); #endif /* LIGHTNING_LIGHTNINGD_JSONRPC_H */ diff --git a/lightningd/opening_control.c b/lightningd/opening_control.c index 9ab4421a6..bb1359d47 100644 --- a/lightningd/opening_control.c +++ b/lightningd/opening_control.c @@ -932,7 +932,7 @@ static void json_fund_channel(struct command *cmd, NULL)) return; - if (!json_tok_wtx(&fc->wtx, buffer, sattok)) + if (!json_tok_wtx(&fc->wtx, buffer, sattok, MAX_FUNDING_SATOSHI)) return; if (!pubkey_from_hexstr(buffer + desttok->start, desttok->end - desttok->start, @@ -949,12 +949,7 @@ static void json_fund_channel(struct command *cmd, BITCOIN_SCRIPTPUBKEY_P2WSH_LEN)) return; - if (fc->wtx.amount > MAX_FUNDING_SATOSHI) { - command_fail(cmd, FUND_MAX_EXCEEDED, - "Funding satoshi must be <= %d", - MAX_FUNDING_SATOSHI); - return; - } + assert(fc->wtx.amount <= MAX_FUNDING_SATOSHI); list_add(&cmd->ld->fundchannels, &fc->list); tal_add_destructor(fc, remove_funding_channel_from_list); diff --git a/lightningd/test/run-param.c b/lightningd/test/run-param.c index eb8d26dc7..1c35b0e83 100644 --- a/lightningd/test/run-param.c +++ b/lightningd/test/run-param.c @@ -53,7 +53,7 @@ bool json_tok_newaddr(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED { fprintf(stderr, "json_tok_newaddr called!\n"); abort(); } /* Generated stub for json_tok_wtx */ bool json_tok_wtx(struct wallet_tx * tx UNNEEDED, const char * buffer UNNEEDED, - const jsmntok_t * sattok UNNEEDED) + const jsmntok_t * sattok UNNEEDED, u64 max UNNEEDED) { fprintf(stderr, "json_tok_wtx called!\n"); abort(); } /* AUTOGENERATED MOCKS END */ diff --git a/tests/test_lightningd.py b/tests/test_lightningd.py index fd59ce077..4c37d9752 100644 --- a/tests/test_lightningd.py +++ b/tests/test_lightningd.py @@ -3949,7 +3949,7 @@ class LightningDTests(BaseLightningDTests): # This should fail, can't even afford fee. self.assertRaises(RpcError, l1.rpc.withdraw, waddr, 'all') - l1.daemon.wait_for_log('Cannot afford funding transaction') + l1.daemon.wait_for_log('Cannot afford transaction') def test_funding_change(self): """Add some funds, fund a channel, and make sure we remember the change @@ -3984,6 +3984,17 @@ class LightningDTests(BaseLightningDTests): outputs = l1.db_query('SELECT value FROM outputs WHERE status=0;') assert len(outputs) == 0 + def test_funding_all_too_much(self): + """Add more than max possible funds, fund a channel using all funds we can. + """ + l1, l2 = self.connect() + + self.give_funds(l1, 2**24 + 10000) + l1.rpc.fundchannel(l2.info['id'], "all") + + assert only_one(l1.rpc.listfunds()['outputs'])['status'] == 'unconfirmed' + assert only_one(l1.rpc.listfunds()['channels'])['channel_total_sat'] == 2**24 - 1 + def test_funding_fail(self): """Add some funds, fund a channel without enough funds""" # Previous runs with same bitcoind can leave funds! @@ -4013,7 +4024,7 @@ class LightningDTests(BaseLightningDTests): l1.rpc.connect(l2.info['id'], 'localhost', l2.port) # We don't have enough left to cover fees if we try to spend it all. - self.assertRaisesRegex(RpcError, r'Cannot afford funding transaction', + self.assertRaisesRegex(RpcError, r'Cannot afford transaction', l1.rpc.fundchannel, l2.info['id'], funds) # Should still be connected. @@ -4042,7 +4053,7 @@ class LightningDTests(BaseLightningDTests): l1.rpc.fundchannel(l2.info['id'], amount) self.fail('Expected fundchannel to fail!') except RpcError as err: - assert 'Funding satoshi must be <= 16777215' in str(err) + assert 'Amount exceeded 16777215' in str(err) # This should work. amount = amount - 1 diff --git a/wallet/walletrpc.c b/wallet/walletrpc.c index f5776cf53..2b1dadb1d 100644 --- a/wallet/walletrpc.c +++ b/wallet/walletrpc.c @@ -104,7 +104,7 @@ static void json_withdraw(struct command *cmd, NULL)) return; - if (!json_tok_wtx(&withdraw->wtx, buffer, sattok)) + if (!json_tok_wtx(&withdraw->wtx, buffer, sattok, -1ULL)) return; /* Parse address. */