Skip to content

Commit

Permalink
bcli: use a more urgent feerate for HTLCs and penalty transactions
Browse files Browse the repository at this point in the history 
A CONSERVATIVE/3 target for them.

Some noisy changes to the tests as we had to update the estimatesmartfee
mock.

Changelog-Changed: We now use a higher feerate for resolving onchain HTLCs and for penalty transactions
  • Loading branch information
@darosior @niftynei
darosior authored and niftynei committed Apr 2, 2020
1 parent 77e7bee commit 7c0af81
Show file tree
Hide file tree
Showing 6 changed files with 121 additions and 65 deletions.
13 changes: 8 additions & 5 deletions contrib/pyln-testing/pyln/testing/utils.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -860,10 +860,12 @@ def mock_estimatesmartfee(r):
params = r['params']
if params == [2, 'CONSERVATIVE']:
feerate = feerates[0] * 4
elif params == [4, 'ECONOMICAL']:
elif params == [3, 'CONSERVATIVE']:
feerate = feerates[1] * 4
elif params == [100, 'ECONOMICAL']:
elif params == [4, 'ECONOMICAL']:
feerate = feerates[2] * 4
elif params == [100, 'ECONOMICAL']:
feerate = feerates[3] * 4
else:
raise ValueError()
return {
Expand All @@ -878,13 +880,14 @@ def mock_estimatesmartfee(r):
# Technically, this waits until it's called, not until it's processed.
# We wait until all three levels have been called.
if wait_for_effect:
wait_for(lambda: self.daemon.rpcproxy.mock_counts['estimatesmartfee'] >= 3)
wait_for(lambda:
self.daemon.rpcproxy.mock_counts['estimatesmartfee'] >= 4)

# force new feerates by restarting and thus skipping slow smoothed process
# Note: testnode must be created with: opts={'may_reconnect': True}
def force_feerates(self, rate):
assert(self.may_reconnect)
self.set_feerates([rate] * 3, False)
self.set_feerates([rate] * 4, False)
self.restart()
self.daemon.wait_for_log('peer_out WIRE_UPDATE_FEE')
assert(self.rpc.feerates('perkw')['perkw']['opening'] == rate)
Expand Down Expand Up @@ -1005,7 +1008,7 @@ def get_nodes(self, num_nodes, opts=None):
return [j.result() for j in jobs]

def get_node(self, node_id=None, options=None, dbfile=None,
feerates=(15000, 7500, 3750), start=True,
feerates=(15000, 11000, 7500, 3750), start=True,
wait_for_bitcoind_sync=True, expect_fail=False, **kwargs):

node_id = self.get_node_id() if not node_id else node_id
Expand Down
42 changes: 33 additions & 9 deletions plugins/bcli.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ static struct command_result *process_getblockchaininfo(struct bitcoin_cli *bcli

struct estimatefees_stash {
/* FIXME: We use u64 but lightningd will store them as u32. */
u64 urgent, normal, slow;
u64 very_urgent, urgent, normal, slow;
};

static struct command_result *
Expand Down Expand Up @@ -532,10 +532,10 @@ static struct command_result *estimatefees_final_step(struct bitcoin_cli *bcli)
response = jsonrpc_stream_success(bcli->cmd);
json_add_u64(response, "opening", stash->normal);
json_add_u64(response, "mutual_close", stash->normal);
json_add_u64(response, "unilateral_close", stash->urgent);
json_add_u64(response, "unilateral_close", stash->very_urgent);
json_add_u64(response, "delayed_to_us", stash->normal);
json_add_u64(response, "htlc_resolution", stash->normal);
json_add_u64(response, "penalty", stash->normal);
json_add_u64(response, "htlc_resolution", stash->urgent);
json_add_u64(response, "penalty", stash->urgent);
/* We divide the slow feerate for the minimum acceptable, lightningd
* will use floor if it's hit, though. */
json_add_u64(response, "min_acceptable", stash->slow / 2);
Expand All @@ -546,13 +546,13 @@ static struct command_result *estimatefees_final_step(struct bitcoin_cli *bcli)
* margin (say 5x the expected fee requirement)
*/
json_add_u64(response, "max_acceptable",
stash->urgent * bitcoind->max_fee_multiplier);
stash->very_urgent * bitcoind->max_fee_multiplier);

return command_finished(bcli->cmd, response);
}

/* We got the response for the normal feerate, now treat the slow one. */
static struct command_result *estimatefees_third_step(struct bitcoin_cli *bcli)
static struct command_result *estimatefees_fourth_step(struct bitcoin_cli *bcli)
{
struct command_result *err;
struct estimatefees_stash *stash = bcli->stash;
Expand All @@ -575,7 +575,7 @@ static struct command_result *estimatefees_third_step(struct bitcoin_cli *bcli)
}

/* We got the response for the urgent feerate, now treat the normal one. */
static struct command_result *estimatefees_second_step(struct bitcoin_cli *bcli)
static struct command_result *estimatefees_third_step(struct bitcoin_cli *bcli)
{
struct command_result *err;
struct estimatefees_stash *stash = bcli->stash;
Expand All @@ -591,6 +591,29 @@ static struct command_result *estimatefees_second_step(struct bitcoin_cli *bcli)

params[0] = "4";
params[1] = "ECONOMICAL";
start_bitcoin_cli(NULL, bcli->cmd, estimatefees_fourth_step, true,
BITCOIND_LOW_PRIO, "estimatesmartfee", params, stash);

return command_still_pending(bcli->cmd);
}

/* We got the response for the very urgent feerate, now treat the urgent one. */
static struct command_result *estimatefees_second_step(struct bitcoin_cli *bcli)
{
struct command_result *err;
struct estimatefees_stash *stash = bcli->stash;
const char **params = tal_arr(bcli->cmd, const char *, 2);

/* If we cannot estimatefees, no need to continue bothering bitcoind. */
if (*bcli->exitstatus != 0)
return estimatefees_null_response(bcli);

err = estimatefees_parse_feerate(bcli, &stash->very_urgent);
if (err)
return err;

params[0] = "3";
params[1] = "CONSERVATIVE";
start_bitcoin_cli(NULL, bcli->cmd, estimatefees_third_step, true,
BITCOIND_LOW_PRIO, "estimatesmartfee", params, stash);

Expand Down Expand Up @@ -726,8 +749,9 @@ static struct command_result *getchaininfo(struct command *cmd,
return command_still_pending(cmd);
}

/* Get the current feerates. We use an urgent feerate for unilateral_close and max,
* a slow feerate for min, and a normal for all others.
/* Get the current feerates. We us an urgent feerate for unilateral_close and max,
* a slightly less urgent feerate for htlc_resolution and penalty transactions,
* a slow feerate for min, and a normal one for all others.
*
* Calls `estimatesmartfee` with targets 2/CONSERVATIVE (urgent),
* 4/ECONOMICAL (normal), and 100/ECONOMICAL (slow) then returns the
Expand Down
48 changes: 30 additions & 18 deletions tests/test_closing.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,9 +216,9 @@ def test_closing_torture(node_factory, executor, bitcoind):
def test_closing_different_fees(node_factory, bitcoind, executor):
l1 = node_factory.get_node()

# Default feerate = 15000/7500/1000
# Default feerate = 15000/11000/7500/1000
# It will start at the second number, accepting anything above the first.
feerates = [[20000, 15000, 7400], [8000, 1001, 100]]
feerates = [[20000, 11000, 15000, 7400], [8000, 6000, 1001, 100]]
amounts = [0, 545999, 546000]
num_peers = len(feerates) * len(amounts)

Expand Down Expand Up @@ -362,10 +362,10 @@ def test_closing_specified_destination(node_factory, bitcoind, chainparams):

def closing_fee(node_factory, bitcoind, chainparams, opts):
rate = opts['funder_feerate_per_kw']
funder = node_factory.get_node(feerates=(rate, rate, rate))
funder = node_factory.get_node(feerates=(rate, rate, rate, rate))

rate = opts['fundee_feerate_per_kw']
fundee = node_factory.get_node(feerates=(rate, rate, rate))
fundee = node_factory.get_node(feerates=(rate, rate, rate, rate))

funder_id = funder.info['id']
fundee_id = fundee.info['id']
Expand Down Expand Up @@ -446,7 +446,9 @@ def test_penalty_inhtlc(node_factory, bitcoind, executor, chainparams):
"""Test penalty transaction with an incoming HTLC"""
# We suppress each one after first commit; HTLC gets added not fulfilled.
# Feerates identical so we don't get gratuitous commit to update them
l1 = node_factory.get_node(disconnect=['=WIRE_COMMITMENT_SIGNED-nocommit'], may_fail=True, feerates=(7500, 7500, 7500), allow_broken_log=True)
l1 = node_factory.get_node(disconnect=['=WIRE_COMMITMENT_SIGNED-nocommit'],
may_fail=True, feerates=(7500, 7500, 7500, 7500),
allow_broken_log=True)
l2 = node_factory.get_node(disconnect=['=WIRE_COMMITMENT_SIGNED-nocommit'])

l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
Expand Down Expand Up @@ -487,6 +489,7 @@ def test_penalty_inhtlc(node_factory, bitcoind, executor, chainparams):
bitcoind.generate_block(1)

l2.daemon.wait_for_log(' to ONCHAIN')

# FIXME: l1 should try to stumble along!
wait_for(lambda: len(l2.getactivechannels()) == 0)

Expand All @@ -509,7 +512,7 @@ def test_penalty_inhtlc(node_factory, bitcoind, executor, chainparams):
outputs = l2.rpc.listfunds()['outputs']
assert [o['status'] for o in outputs] == ['confirmed'] * 2
# Allow some lossage for fees.
slack = 27000 if chainparams['elements'] else 15000
slack = 30000 if chainparams['elements'] else 20000
assert sum(o['value'] for o in outputs) < 10**6
assert sum(o['value'] for o in outputs) > 10**6 - slack

Expand All @@ -519,7 +522,9 @@ def test_penalty_outhtlc(node_factory, bitcoind, executor, chainparams):
"""Test penalty transaction with an outgoing HTLC"""
# First we need to get funds to l2, so suppress after second.
# Feerates identical so we don't get gratuitous commit to update them
l1 = node_factory.get_node(disconnect=['=WIRE_COMMITMENT_SIGNED*3-nocommit'], may_fail=True, feerates=(7500, 7500, 7500), allow_broken_log=True)
l1 = node_factory.get_node(disconnect=['=WIRE_COMMITMENT_SIGNED*3-nocommit'],
may_fail=True, feerates=(7500, 7500, 7500, 7500),
allow_broken_log=True)
l2 = node_factory.get_node(disconnect=['=WIRE_COMMITMENT_SIGNED*3-nocommit'])

l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
Expand Down Expand Up @@ -589,7 +594,7 @@ def test_penalty_outhtlc(node_factory, bitcoind, executor, chainparams):
outputs = l2.rpc.listfunds()['outputs']
assert [o['status'] for o in outputs] == ['confirmed'] * 3
# Allow some lossage for fees.
slack = 27000 if chainparams['elements'] else 15000
slack = 30000 if chainparams['elements'] else 20000
assert sum(o['value'] for o in outputs) < 10**6
assert sum(o['value'] for o in outputs) > 10**6 - slack

Expand Down Expand Up @@ -687,7 +692,8 @@ def test_onchaind_replay(node_factory, bitcoind):
disconnects = ['+WIRE_REVOKE_AND_ACK', 'permfail']
options = {'watchtime-blocks': 201, 'cltv-delta': 101}
# Feerates identical so we don't get gratuitous commit to update them
l1 = node_factory.get_node(options=options, disconnect=disconnects, feerates=(7500, 7500, 7500))
l1 = node_factory.get_node(options=options, disconnect=disconnects,
feerates=(7500, 7500, 7500, 7500))
l2 = node_factory.get_node(options=options)

l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
Expand Down Expand Up @@ -740,7 +746,8 @@ def test_onchain_dust_out(node_factory, bitcoind, executor):
# HTLC 1->2, 1 fails after it's irrevocably committed
disconnects = ['@WIRE_REVOKE_AND_ACK', 'permfail']
# Feerates identical so we don't get gratuitous commit to update them
l1 = node_factory.get_node(disconnect=disconnects, feerates=(7500, 7500, 7500))
l1 = node_factory.get_node(disconnect=disconnects,
feerates=(7500, 7500, 7500, 7500))
l2 = node_factory.get_node()

l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
Expand Down Expand Up @@ -803,7 +810,8 @@ def test_onchain_timeout(node_factory, bitcoind, executor):
# HTLC 1->2, 1 fails just after it's irrevocably committed
disconnects = ['+WIRE_REVOKE_AND_ACK*3', 'permfail']
# Feerates identical so we don't get gratuitous commit to update them
l1 = node_factory.get_node(disconnect=disconnects, feerates=(7500, 7500, 7500))
l1 = node_factory.get_node(disconnect=disconnects,
feerates=(7500, 7500, 7500, 7500))
l2 = node_factory.get_node()

l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
Expand Down Expand Up @@ -1038,7 +1046,8 @@ def test_onchain_all_dust(node_factory, bitcoind, executor):
# is generated on-the-fly, and is thus feerate sensitive.
disconnects = ['-WIRE_UPDATE_FAIL_HTLC', 'permfail']
# Feerates identical so we don't get gratuitous commit to update them
l1 = node_factory.get_node(options={'dev-no-reconnect': None}, feerates=(7500, 7500, 7500))
l1 = node_factory.get_node(options={'dev-no-reconnect': None},
feerates=(7500, 7500, 7500, 7500))
l2 = node_factory.get_node(disconnect=disconnects)

l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
Expand All @@ -1060,7 +1069,7 @@ def test_onchain_all_dust(node_factory, bitcoind, executor):
l2.wait_for_channel_onchain(l1.info['id'])

# Make l1's fees really high (and wait for it to exceed 50000)
l1.set_feerates((100000, 100000, 100000))
l1.set_feerates((100000, 100000, 100000, 100000))
l1.daemon.wait_for_log('Feerate estimate for unilateral_close set to [56789][0-9]{4}')

bitcoind.generate_block(1)
Expand Down Expand Up @@ -1097,12 +1106,12 @@ def test_onchain_different_fees(node_factory, bitcoind, executor):
p1 = executor.submit(l1.pay, l2, 1000000000)
l1.daemon.wait_for_log('htlc 0: RCVD_ADD_ACK_COMMIT->SENT_ADD_ACK_REVOCATION')

l1.set_feerates((16000, 7500, 3750))
l1.set_feerates((16000, 11000, 7500, 3750))
p2 = executor.submit(l1.pay, l2, 900000000)
l1.daemon.wait_for_log('htlc 1: RCVD_ADD_ACK_COMMIT->SENT_ADD_ACK_REVOCATION')

# Restart with different feerate for second HTLC.
l1.set_feerates((5000, 5000, 3750))
l1.set_feerates((5000, 5000, 5000, 3750))
l1.restart()
l1.daemon.wait_for_log('peer_out WIRE_UPDATE_FEE')

Expand Down Expand Up @@ -1156,7 +1165,8 @@ def test_permfail_new_commit(node_factory, bitcoind, executor):
# Test case where we have two possible commits: it will use new one.
disconnects = ['-WIRE_REVOKE_AND_ACK', 'permfail']
# Feerates identical so we don't get gratuitous commit to update them
l1 = node_factory.get_node(options={'dev-no-reconnect': None}, feerates=(7500, 7500, 7500))
l1 = node_factory.get_node(options={'dev-no-reconnect': None},
feerates=(7500, 7500, 7500, 7500))
l2 = node_factory.get_node(disconnect=disconnects)

l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
Expand Down Expand Up @@ -1457,7 +1467,8 @@ def test_permfail_htlc_in(node_factory, bitcoind, executor):
# Test case where we fail with unsettled incoming HTLC.
disconnects = ['-WIRE_UPDATE_FULFILL_HTLC', 'permfail']
# Feerates identical so we don't get gratuitous commit to update them
l1 = node_factory.get_node(options={'dev-no-reconnect': None}, feerates=(7500, 7500, 7500))
l1 = node_factory.get_node(options={'dev-no-reconnect': None},
feerates=(7500, 7500, 7500, 7500))
l2 = node_factory.get_node(disconnect=disconnects)

l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
Expand Down Expand Up @@ -1503,7 +1514,8 @@ def test_permfail_htlc_out(node_factory, bitcoind, executor):
disconnects = ['+WIRE_REVOKE_AND_ACK', 'permfail']
l1 = node_factory.get_node(options={'dev-no-reconnect': None})
# Feerates identical so we don't get gratuitous commit to update them
l2 = node_factory.get_node(disconnect=disconnects, feerates=(7500, 7500, 7500))
l2 = node_factory.get_node(disconnect=disconnects,
feerates=(7500, 7500, 7500, 7500))

l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
l2.daemon.wait_for_log('openingd-chan#1: Handed peer, entering loop'.format(l1.info['id']))
Expand Down
Loading

0 comments on commit 7c0af81

Please sign in to comment.