Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a commitment feerate parameter to multifundchannel #4139

Merged
merged 4 commits into from
Oct 21, 2020
Merged

Add a commitment feerate parameter to multifundchannel #4139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
037475e
mfc: Add a `commitment_feerate` optional parameter to multifundchannel
niftynei Oct 15, 2020
4a738b7
mfc: use declared constants
niftynei Oct 15, 2020
012cf55
tx,bugfix: correct signature length estimate
niftynei Oct 19, 2020
a45b7a2
mfc: blackbox test for commitment vs funding tx feerates
niftynei Oct 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions bitcoin/tx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,10 +835,10 @@ size_t bitcoin_tx_output_weight(size_t outscript_len)
return weight;
}

/* We grind signatures to get them down to 71 bytes (+1 for sighash flags) */
/* We grind signatures to get them down to 71 bytes */
size_t bitcoin_tx_input_sig_weight(void)
{
return 1 + 71 + 1;
return 1 + 71;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you overlooked the length prefix.

length prefix + sig + sighash_flag

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sighash_flag is included in the 71 count, see commit message

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right!

}

/* We only do segwit inputs, and we assume witness is sig + key */
Expand Down
13 changes: 9 additions & 4 deletions doc/lightning-multifundchannel.7
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 7 additions & 3 deletions doc/lightning-multifundchannel.7.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ lightning-multifundchannel -- Command for establishing many lightning channels
SYNOPSIS
--------

**multifundchannel** *destinations* \[*feerate*\] \[*minconf*\] \[*utxos*\] \[*minchannels*\]
**multifundchannel** *destinations* \[*feerate*\] \[*minconf*\] \[*utxos*\] \[*minchannels*\] \[*commitment_feerate*\]

DESCRIPTION
-----------
Expand Down Expand Up @@ -55,8 +55,9 @@ Readiness is indicated by **listpeers** reporting a *state* of
There must be at least one entry in *destinations*;
it cannot be an empty array.

*feerate* is an optional feerate used for the opening transaction and as
initial feerate for commitment and HTLC transactions. It can be one of
*feerate* is an optional feerate used for the opening transaction and, if
*commitment_feerate* is not set, as the initial feerate for
commitment and HTLC transactions. It can be one of
the strings *urgent* (aim for next block), *normal* (next 4 blocks or
so) or *slow* (next 100 blocks or so) to use lightningd’s internal
estimates: *normal* is the default.
Expand All @@ -77,6 +78,9 @@ this many peers remain (must not be zero).
The **multifundchannel** command will only fail if too many peers fail
the funding process.

*commitment_feerate* is the initial feerate for commitment and HTLC
transactions. See *feerate* for valid values.

RETURN VALUE
------------

Expand Down
34 changes: 21 additions & 13 deletions plugins/spender/multifundchannel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,17 @@ struct multifundchannel_command {
*/
size_t pending;

/* The feerate desired by the user. */
/* The feerate desired by the user.
* If cmtmt_feerate_str is present, will only be used
* for the funding transaction. */
const char *feerate_str;

/* The feerate desired by the user for
* the channel commitment and HTLC txs.
* If not provided, defaults to the feerate_str
* value. */
const char *cmtmt_feerate_str;

/* The minimum number of confirmations for owned
UTXOs to be selected.
*/
Expand Down Expand Up @@ -877,13 +886,10 @@ perform_fundpsbt(struct multifundchannel_command *mfc)
* inputs, that estimation should be correct.
*/
startweight = bitcoin_tx_core_weight(1, num_outs)
+ ( bitcoin_tx_output_weight( 1 /* OP_0 */
+ 1 /* OP_PUSHDATA */
+ 32 /* P2WSH */
)
+ ( bitcoin_tx_output_weight(
BITCOIN_SCRIPTPUBKEY_P2WSH_LEN)
* num_outs
)
;
);
json_add_string(req->js, "startweight",
tal_fmt(tmpctx, "%zu", startweight));
}
Expand Down Expand Up @@ -1014,10 +1020,8 @@ handle_mfc_change(struct multifundchannel_command *mfc)
* Get the weight of a change output and how much it
* costs.
*/
change_weight = bitcoin_tx_output_weight( 1 /* OP_0 */
+ 1 /* OP_PUSHDATA */
+ 20 /* P2WPKH */
);
change_weight = bitcoin_tx_output_weight(
BITCOIN_SCRIPTPUBKEY_P2WPKH_LEN);
change_fee = amount_tx_fee(mfc->feerate_per_kw, change_weight);
/* The limit is equal to the change_fee plus the dust limit. */
if (!amount_sat_add(&change_min_limit,
Expand Down Expand Up @@ -1164,7 +1168,9 @@ fundchannel_start_dest(struct multifundchannel_destination *dest)
json_add_string(req->js, "amount",
fmt_amount_sat(tmpctx, &dest->amount));

if (mfc->feerate_str)
if (mfc->cmtmt_feerate_str)
json_add_string(req->js, "feerate", mfc->cmtmt_feerate_str);
else if (mfc->feerate_str)
json_add_string(req->js, "feerate", mfc->feerate_str);
json_add_bool(req->js, "announce", dest->announce);
json_add_string(req->js, "push_msat",
Expand Down Expand Up @@ -1988,7 +1994,7 @@ json_multifundchannel(struct command *cmd,
const jsmntok_t *params)
{
struct multifundchannel_destination *dests;
const char *feerate_str;
const char *feerate_str, *cmtmt_feerate_str;
u32 *minconf;
const jsmntok_t *utxos_tok;
u32 *minchannels;
Expand All @@ -2001,6 +2007,7 @@ json_multifundchannel(struct command *cmd,
p_opt_def("minconf", param_number, &minconf, 1),
p_opt("utxos", param_tok, &utxos_tok),
p_opt("minchannels", param_positive_number, &minchannels),
p_opt("commitment_feerate", param_string, &cmtmt_feerate_str),
NULL))
return command_param_failed();

Expand All @@ -2017,6 +2024,7 @@ json_multifundchannel(struct command *cmd,
mfc->destinations[i].mfc = mfc;

mfc->feerate_str = feerate_str;
mfc->cmtmt_feerate_str = cmtmt_feerate_str;
mfc->minconf = *minconf;
if (utxos_tok)
mfc->utxos_str = tal_strndup(mfc, json_tok_full(buf, utxos_tok),
Expand Down
55 changes: 55 additions & 0 deletions tests/test_connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1366,6 +1366,61 @@ def test_multifunding_wumbo(node_factory):
l1.rpc.multifundchannel(destinations)


@unittest.skipIf(TEST_NETWORK == 'liquid-regtest', "Fees on elements are different")
@unittest.skipIf(not DEVELOPER, "uses dev-fail")
def test_multifunding_feerates(node_factory, bitcoind):
'''
Test feerate parameters for multifundchannel
'''
funding_tx_feerate = '10000perkw'
commitment_tx_feerate = '2000perkw'

l1, l2, l3 = node_factory.get_nodes(3, opts={'log-level': 'debug'})

l1.fundwallet(1 << 26)

def _connect_str(node):
return '{}@localhost:{}'.format(node.info['id'], node.port)

destinations = [{"id": _connect_str(l2), 'amount': 50000}]

res = l1.rpc.multifundchannel(destinations, feerate=funding_tx_feerate,
commitment_feerate=commitment_tx_feerate)

entry = bitcoind.rpc.getmempoolentry(res['txid'])
weight = entry['weight']

expected_fee = int(funding_tx_feerate[:-5]) * weight // 1000
assert expected_fee == entry['fees']['base'] * 10 ** 8

# We get the expected close txid, force close the channel, then fish
# the details about the transaction out of the mempoool entry
close_txid = only_one(only_one(l1.rpc.listpeers(l2.info['id'])['peers'])['channels'])['scratch_txid']
l1.rpc.dev_fail(l2.info['id'])
l1.wait_for_channel_onchain(l2.info['id'])
entry = bitcoind.rpc.getmempoolentry(close_txid)

# Because of how the anchor outputs protocol is designed,
# we *always* pay for 2 anchor outs and their weight
if EXPERIMENTAL_FEATURES: # opt_anchor_outputs
weight = 1124
else:
# the commitment transactions' feerate is calculated off
# of this fixed weight
weight = 724

expected_fee = int(commitment_tx_feerate[:-5]) * weight // 1000

# At this point we only have one anchor output on the
# tx, but we subtract out the extra anchor output amount
# from the to_us output, so it ends up inflating
# our fee by that much.
if EXPERIMENTAL_FEATURES: # opt_anchor_outputs
expected_fee += 330

assert expected_fee == entry['fees']['base'] * 10 ** 8


def test_multifunding_param_failures(node_factory):
'''
Test that multifunding handles errors in parameters.
Expand Down
34 changes: 17 additions & 17 deletions tests/test_misc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,46 +660,46 @@ def dont_spend_outputs(n, txid):
{'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
[
{'type': 'chain_mvt', 'credit': 0, 'debit': 2000000000, 'tag': 'withdrawal'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 1993745000, 'tag': 'withdrawal'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 1993760000, 'tag': 'withdrawal'},
],
{'type': 'chain_mvt', 'credit': 0, 'debit': 6255000, 'tag': 'chain_fees'},
{'type': 'chain_mvt', 'credit': 1993745000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 6240000, 'tag': 'chain_fees'},
{'type': 'chain_mvt', 'credit': 1993760000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
[
{'type': 'chain_mvt', 'credit': 0, 'debit': 2000000000, 'tag': 'withdrawal'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 1993745000, 'tag': 'withdrawal'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 1993760000, 'tag': 'withdrawal'},
],
{'type': 'chain_mvt', 'credit': 0, 'debit': 6255000, 'tag': 'chain_fees'},
{'type': 'chain_mvt', 'credit': 1993745000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 6240000, 'tag': 'chain_fees'},
{'type': 'chain_mvt', 'credit': 1993760000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
[
{'type': 'chain_mvt', 'credit': 0, 'debit': 2000000000, 'tag': 'withdrawal'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 1993745000, 'tag': 'withdrawal'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 1993760000, 'tag': 'withdrawal'},
],
{'type': 'chain_mvt', 'credit': 0, 'debit': 6255000, 'tag': 'chain_fees'},
{'type': 'chain_mvt', 'credit': 1993745000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 6240000, 'tag': 'chain_fees'},
{'type': 'chain_mvt', 'credit': 1993760000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
[
{'type': 'chain_mvt', 'credit': 0, 'debit': 1993385000, 'tag': 'withdrawal'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 1993400000, 'tag': 'withdrawal'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 2000000000, 'tag': 'withdrawal'},
],
{'type': 'chain_mvt', 'credit': 0, 'debit': 6615000, 'tag': 'chain_fees'},
{'type': 'chain_mvt', 'credit': 1993385000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 6600000, 'tag': 'chain_fees'},
{'type': 'chain_mvt', 'credit': 1993400000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 11961135000, 'tag': 'withdrawal'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 13485000, 'tag': 'chain_fees'},
{'type': 'chain_mvt', 'credit': 11961135000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 11961240000, 'tag': 'withdrawal'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 13440000, 'tag': 'chain_fees'},
{'type': 'chain_mvt', 'credit': 11961240000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 11957490000, 'tag': 'withdrawal'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 3645000, 'tag': 'chain_fees'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 11957603000, 'tag': 'withdrawal'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 3637000, 'tag': 'chain_fees'},
]
check_coin_moves(l1, 'wallet', wallet_moves, chainparams)

Expand Down
18 changes: 9 additions & 9 deletions tests/test_plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1606,11 +1606,11 @@ def test_coin_movement_notices(node_factory, bitcoind, chainparams):
{'type': 'chain_mvt', 'credit': 2000000000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
[
{'type': 'chain_mvt', 'credit': 0, 'debit': 991900000, 'tag': 'withdrawal'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 991908000, 'tag': 'withdrawal'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 1000000000, 'tag': 'withdrawal'},
],
{'type': 'chain_mvt', 'credit': 0, 'debit': 8100000, 'tag': 'chain_fees'},
{'type': 'chain_mvt', 'credit': 991900000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 8092000, 'tag': 'chain_fees'},
{'type': 'chain_mvt', 'credit': 991908000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 100001000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 945523000, 'debit': 0, 'tag': 'deposit'},
]
Expand All @@ -1629,11 +1629,11 @@ def test_coin_movement_notices(node_factory, bitcoind, chainparams):
{'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
# Could go in either order
[
{'type': 'chain_mvt', 'credit': 0, 'debit': 995425000, 'tag': 'withdrawal'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 995433000, 'tag': 'withdrawal'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 1000000000, 'tag': 'withdrawal'},
],
{'type': 'chain_mvt', 'credit': 0, 'debit': 4575000, 'tag': 'chain_fees'},
{'type': 'chain_mvt', 'credit': 995425000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 4567000, 'tag': 'chain_fees'},
{'type': 'chain_mvt', 'credit': 995433000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 100001000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 945785000, 'debit': 0, 'tag': 'deposit'},
]
Expand All @@ -1651,11 +1651,11 @@ def test_coin_movement_notices(node_factory, bitcoind, chainparams):
{'type': 'chain_mvt', 'credit': 0, 'debit': 0, 'tag': 'spend_track'},
# Could go in either order
[
{'type': 'chain_mvt', 'credit': 0, 'debit': 995425000, 'tag': 'withdrawal'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 995433000, 'tag': 'withdrawal'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 1000000000, 'tag': 'withdrawal'},
],
{'type': 'chain_mvt', 'credit': 0, 'debit': 4575000, 'tag': 'chain_fees'},
{'type': 'chain_mvt', 'credit': 995425000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 0, 'debit': 4567000, 'tag': 'chain_fees'},
{'type': 'chain_mvt', 'credit': 995433000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 100001000, 'debit': 0, 'tag': 'deposit'},
{'type': 'chain_mvt', 'credit': 947285000, 'debit': 0, 'tag': 'deposit'},
]
Expand Down
2 changes: 1 addition & 1 deletion tests/test_wallet.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ def test_utxopsbt(node_factory, bitcoind, chainparams):
assert psbt2['tx']['vin'] == psbt['tx']['vin']
if chainparams['elements']:
# elements includes the fee as an output
addl_fee = Millisatoshi(fee_val * start_weight // 1000 * 1000)
addl_fee = Millisatoshi((fee_val * start_weight + 999) // 1000 * 1000)
assert psbt2['tx']['vout'][0]['value'] == psbt['tx']['vout'][0]['value'] + addl_fee.to_btc()
else:
assert psbt2['tx']['vout'] == psbt['tx']['vout']
Expand Down