From 35d1b13cde95a9b6be6b92670cabb47606d55d7e Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 10 May 2016 06:30:11 +0930 Subject: [PATCH] daemon: commit outstanding changes via timer. While useful for testing, it doesn't make sense to have an explicit commit command; we should commit whenever there are outstanding changes. We have a 10ms timer to allow limited batching, however. Signed-off-by: Rusty Russell --- Makefile | 3 +- daemon/lightningd.c | 6 +++ daemon/lightningd.h | 3 ++ daemon/packets.c | 8 +++- daemon/peer.c | 52 +++++++++++++++++++----- daemon/peer.h | 6 +++ daemon/test/test.sh | 97 +++++++++++++++++++++++++++------------------ 7 files changed, 123 insertions(+), 52 deletions(-) diff --git a/Makefile b/Makefile index 2ffd810a9..e4959bd73 100644 --- a/Makefile +++ b/Makefile @@ -192,7 +192,8 @@ daemon-test-%: daemon-test-steal: daemon-test-dump-onchain daemon-test-dump-onchain: daemon-test-timeout-anchor daemon-test-timeout-anchor: daemon-test-normal -daemon-test-normal: daemon-all +daemon-test-normal: daemon-test-manual-commit +daemon-test-manual-commit: daemon-all daemon-tests: daemon-test-steal diff --git a/daemon/lightningd.c b/daemon/lightningd.c index 63b9375ed..577209607 100644 --- a/daemon/lightningd.c +++ b/daemon/lightningd.c @@ -103,6 +103,9 @@ static void config_register_opts(struct lightningd_state *dstate) opt_register_arg("--bitcoind-poll", opt_set_time, opt_show_time, &dstate->config.poll_time, "Time between polling for new transactions"); + opt_register_arg("--commit-time", opt_set_time, opt_show_time, + &dstate->config.commit_time, + "Time after changes before sending out COMMIT"); } #define MINUTES 60 @@ -154,6 +157,9 @@ static void default_config(struct config *config) config->max_expiry = 5 * DAYS; config->poll_time = time_from_sec(30); + + /* Send commit 10msec after receiving; almost immediately. */ + config->commit_time = time_from_msec(10); } static void check_config(struct lightningd_state *dstate) diff --git a/daemon/lightningd.h b/daemon/lightningd.h index 2799407c6..434c518fe 100644 --- a/daemon/lightningd.h +++ b/daemon/lightningd.h @@ -43,6 +43,9 @@ struct config { /* How long between polling bitcoind. */ struct timerel poll_time; + + /* How long between changing commit and sending COMMIT message.. */ + struct timerel commit_time; }; /* Here's where the global variables hide! */ diff --git a/daemon/packets.c b/daemon/packets.c index 1f3d63840..82b73cd8e 100644 --- a/daemon/packets.c +++ b/daemon/packets.c @@ -217,7 +217,8 @@ void queue_pkt_htlc_add(struct peer *peer, fatal("Could not add HTLC?"); peer_add_htlc_expiry(peer, &htlc_prog->stage.add.htlc.expiry); - + + their_commit_changed(peer); queue_pkt_with_ack(peer, PKT__PKT_UPDATE_ADD_HTLC, u, add_our_htlc_ourside, tal_dup(peer, struct channel_htlc, @@ -248,6 +249,7 @@ void queue_pkt_htlc_fulfill(struct peer *peer, /* We're about to send this, so their side will have it from now on. */ n = funding_htlc_by_id(&peer->them.staging_cstate->a, f->id); funding_a_fulfill_htlc(peer->them.staging_cstate, n); + their_commit_changed(peer); queue_pkt_with_ack(peer, PKT__PKT_UPDATE_FULFILL_HTLC, f, fulfill_their_htlc_ourside, int2ptr(f->id)); @@ -280,6 +282,7 @@ void queue_pkt_htlc_fail(struct peer *peer, n = funding_htlc_by_id(&peer->them.staging_cstate->a, f->id); funding_a_fail_htlc(peer->them.staging_cstate, n); + their_commit_changed(peer); queue_pkt_with_ack(peer, PKT__PKT_UPDATE_FAIL_HTLC, f, fail_their_htlc_ourside, int2ptr(f->id)); } @@ -605,6 +608,7 @@ Pkt *accept_pkt_htlc_add(struct peer *peer, const Pkt *pkt) u->amount_msat); peer_add_htlc_expiry(peer, &expiry); + their_commit_changed(peer); /* FIXME: Fees must be sufficient. */ return NULL; @@ -651,6 +655,7 @@ Pkt *accept_pkt_htlc_fail(struct peer *peer, const Pkt *pkt) funding_a_fail_htlc(peer->us.staging_cstate, n_us); funding_b_fail_htlc(peer->them.staging_cstate, n_them); + their_commit_changed(peer); return NULL; } @@ -677,6 +682,7 @@ Pkt *accept_pkt_htlc_fulfill(struct peer *peer, const Pkt *pkt) funding_a_fulfill_htlc(peer->us.staging_cstate, n_us); funding_b_fulfill_htlc(peer->them.staging_cstate, n_them); + their_commit_changed(peer); return NULL; } diff --git a/daemon/peer.c b/daemon/peer.c index bb601a90e..7d6e4a9a6 100644 --- a/daemon/peer.c +++ b/daemon/peer.c @@ -414,6 +414,45 @@ static void peer_disconnect(struct io_conn *conn, struct peer *peer) } } +static void do_commit(struct peer *peer, struct command *jsoncmd) +{ + /* We can have changes we suggested, or changes they suggested. */ + if (peer->them.staging_cstate + && peer->them.commit + && peer->them.commit->cstate + && peer->them.staging_cstate->changes + != peer->them.commit->cstate->changes) { + log_debug(peer->log, "do_commit: sending commit command"); + set_current_command(peer, CMD_SEND_COMMIT, NULL, jsoncmd); + return; + } + log_debug(peer->log, "do_commit: no changes to commit"); + + if (jsoncmd) { + command_fail(jsoncmd, "no changes to commit"); + return; + } +} + +static void try_commit(struct peer *peer) +{ + peer->commit_timer = NULL; + queue_cmd(peer, do_commit, (struct command *)NULL); +} + +void their_commit_changed(struct peer *peer) +{ + log_debug(peer->log, "their_commit_changed: changes=%u", + peer->them.staging_cstate->changes); + if (!peer->commit_timer) { + log_debug(peer->log, "their_commit_changed: adding timer"); + peer->commit_timer = new_reltimer(peer->dstate, peer, + peer->dstate->config.commit_time, + try_commit, peer); + } else + log_debug(peer->log, "their_commit_changed: timer already exists"); +} + static struct peer *new_peer(struct lightningd_state *dstate, struct io_conn *conn, int addr_type, int addr_protocol, @@ -452,6 +491,7 @@ static struct peer *new_peer(struct lightningd_state *dstate, peer->closing_onchain.tx = NULL; peer->closing_onchain.resolved = NULL; peer->closing_onchain.ci = NULL; + peer->commit_timer = NULL; /* Make it different from other node (to catch bugs!), but a * round number for simple eyeballing. */ peer->htlc_id_counter = pseudorand(1ULL << 32) * 1000; @@ -488,6 +528,7 @@ static struct peer *new_peer(struct lightningd_state *dstate, peer->log = new_log(peer, dstate->log_record, "%s%s:%s:", log_prefix(dstate->base_log), in_or_out, netaddr_name(peer, &peer->addr)); + return peer; } @@ -2370,17 +2411,6 @@ const struct json_command failhtlc_command = { "Returns an empty result on success" }; -static void do_commit(struct peer *peer, struct command *jsoncmd) -{ - /* We can have changes we suggested, or changes they suggested. */ - if (peer->them.staging_cstate->changes == peer->them.commit->cstate->changes) { - command_fail(jsoncmd, "no changes to commit"); - return; - } - - set_current_command(peer, CMD_SEND_COMMIT, NULL, jsoncmd); -} - static void json_commit(struct command *cmd, const char *buffer, const jsmntok_t *params) { diff --git a/daemon/peer.h b/daemon/peer.h index 201c92fcf..7950766ac 100644 --- a/daemon/peer.h +++ b/daemon/peer.h @@ -212,6 +212,9 @@ struct peer { /* Timeout for close_watch. */ struct oneshot *close_watch_timeout; + + /* Timeout for collecting changes before sending commit. */ + struct oneshot *commit_timer; /* Private keys for dealing with this peer. */ struct peer_secrets *secrets; @@ -241,5 +244,8 @@ struct bitcoin_tx *peer_create_close_tx(struct peer *peer, u64 fee); uint64_t commit_tx_fee(const struct bitcoin_tx *commit, uint64_t anchor_satoshis); +/* We have something pending for them. */ +void their_commit_changed(struct peer *peer); + bool resolve_one_htlc(struct peer *peer, u64 id, const struct sha256 *preimage); #endif /* LIGHTNING_DAEMON_PEER_H */ diff --git a/daemon/test/test.sh b/daemon/test/test.sh index 69bbb2652..9012d10e5 100755 --- a/daemon/test/test.sh +++ b/daemon/test/test.sh @@ -55,6 +55,9 @@ while [ $# != 0 ]; do x"--steal") STEAL=1 ;; + x"--manual-commit") + MANUALCOMMIT=1 + ;; x"--normal") ;; x"--verbose") @@ -93,17 +96,17 @@ lcli2() $LCLI2 "$@" } -# Usage: ... +# Usage: ... check() { local i=0 - local node="$1" - shift while ! eval "$@"; do - # Try making time pass for the node (if on mocktime), then sleeping. + # Try making time pass for the nodes (if on mocktime), then sleeping. if [ -n "$MOCKTIME" ]; then MOCKTIME=$(($MOCKTIME + 1)) - $node dev-mocktime $MOCKTIME + # Some tests kill nodes, so ignore failure here. + lcli1 dev-mocktime $MOCKTIME > /dev/null 2>&1 || true + lcli2 dev-mocktime $MOCKTIME > /dev/null 2>&1 || true fi sleep 1 i=$(($i + 1)) @@ -123,7 +126,7 @@ check_status_single() them_fee=$6 them_htlcs="$7" - if check $lcli "$lcli getpeers | tr -s '\012\011\" ' ' ' | $FGREP \"our_amount : $us_pay, our_fee : $us_fee, their_amount : $them_pay, their_fee : $them_fee, our_htlcs : [ $us_htlcs], their_htlcs : [ $them_htlcs]\""; then :; else + if check "$lcli getpeers | tr -s '\012\011\" ' ' ' | $FGREP \"our_amount : $us_pay, our_fee : $us_fee, their_amount : $them_pay, their_fee : $them_fee, our_htlcs : [ $us_htlcs], their_htlcs : [ $them_htlcs]\""; then :; else echo Cannot find $lcli output: '"our_amount" : '$us_pay', "our_fee" : '$us_fee', "their_amount" : '$them_pay', "their_fee" : '$them_fee', "our_htlcs" : [ '"$us_htlcs"'], "their_htlcs" : [ '"$them_htlcs"']' >&2 $lcli getpeers | tr -s '\012\011 ' ' ' >&2 return 1 @@ -148,7 +151,7 @@ check_staged() lcli="$1" num_htlcs="$2" - if check $lcli "$lcli getpeers | tr -s '\012\011\" ' ' ' | $FGREP 'staged_changes : '$num_htlcs"; then :; else + if check "$lcli getpeers | tr -s '\012\011\" ' ' ' | $FGREP 'staged_changes : '$num_htlcs"; then :; else echo Cannot find $lcli output: '"staged_changes" : '$num_htlcs >&2 $lcli getpeers | tr -s '\012\011 ' ' ' >&2 return 1 @@ -157,7 +160,7 @@ check_staged() check_tx_spend() { - if check "$1" "$CLI getrawmempool | $FGREP '\"'"; then :; + if check "$CLI getrawmempool | $FGREP '\"'"; then :; else echo "No tx in mempool:" >&2 $CLI getrawmempool >&2 @@ -167,7 +170,7 @@ check_tx_spend() check_peerstate() { - if check "$1" "$1 getpeers | $FGREP -w $2"; then : + if check "$1 getpeers | $FGREP -w $2"; then : else echo "$1" not in state "$2": >&2 $1 getpeers >&2 @@ -177,7 +180,7 @@ check_peerstate() check_peerconnected() { - if check "$1" "$1 getpeers | tr -s '\012\011\" ' ' ' | $FGREP -w 'connected : '$2"; then : + if check "$1 getpeers | tr -s '\012\011\" ' ' ' | $FGREP -w 'connected : '$2"; then : else echo "$1" not connected "$2": >&2 $1 getpeers >&2 @@ -187,7 +190,7 @@ check_peerconnected() check_no_peers() { - if check "$1" "$1 getpeers | tr -s '\012\011\" ' ' ' | $FGREP 'peers : [ ]'"; then : + if check "$1 getpeers | tr -s '\012\011\" ' ' ' | $FGREP 'peers : [ ]'"; then : else echo "$1" still has peers: >&2 $1 getpeers >&2 @@ -209,12 +212,20 @@ all_ok() trap "echo Results in $DIR1 and $DIR2 >&2; cat $DIR1/errors $DIR2/errors >&2" EXIT mkdir $DIR1 $DIR2 +if [ -n "$MANUALCOMMIT" ]; then + # Aka. never. + COMMIT_TIME=1h +else + COMMIT_TIME=10ms +fi + cat > $DIR1/config < $DIR2/config < $REDIR2 2> $REDIRERR2 & fi -if ! check "$LCLI1" "$LCLI1 getlog 2>/dev/null | $FGREP Hello"; then +if ! check "$LCLI1 getlog 2>/dev/null | $FGREP Hello"; then echo Failed to start daemon 1 >&2 exit 1 fi -if ! check "$LCLI2" "$LCLI2 getlog 2>/dev/null | $FGREP Hello"; then +if ! check "$LCLI2 getlog 2>/dev/null | $FGREP Hello"; then echo Failed to start daemon 2 >&2 exit 1 fi @@ -336,27 +348,34 @@ SECRET=1de08917a61cb2b62ed5937d38577f6a7bfe59c176781c6d8128018e8b5ccdfd RHASH=`lcli1 dev-rhash $SECRET | sed 's/.*"\([0-9a-f]*\)".*/\1/'` lcli1 newhtlc $ID2 $HTLC_AMOUNT $EXPIRY $RHASH -# Nothing should have changed! -check_status $A_AMOUNT $A_FEE "" $B_AMOUNT $B_FEE "" -# But 2 should register a staged htlc. -check_staged lcli2 1 +if [ -n "$MANUALCOMMIT" ]; then + # Nothing should have changed! + check_status $A_AMOUNT $A_FEE "" $B_AMOUNT $B_FEE "" + # But 2 should register a staged htlc. + check_staged lcli2 1 -# Now commit it. -lcli1 commit $ID2 + # Now commit it. + lcli1 commit $ID2 -# Node 1 hasn't got it committed, but node2 should have told it to stage. -check_status_single lcli1 $A_AMOUNT $A_FEE "" $B_AMOUNT $B_FEE "" -check_staged lcli1 1 + # Node 1 hasn't got it committed, but node2 should have told it to stage. + check_status_single lcli1 $A_AMOUNT $A_FEE "" $B_AMOUNT $B_FEE "" + check_staged lcli1 1 -# Check channel status -A_AMOUNT=$(($A_AMOUNT - $EXTRA_FEE - $HTLC_AMOUNT)) -A_FEE=$(($A_FEE + $EXTRA_FEE)) + # Check channel status + A_AMOUNT=$(($A_AMOUNT - $EXTRA_FEE - $HTLC_AMOUNT)) + A_FEE=$(($A_FEE + $EXTRA_FEE)) -# Node 2 has it committed. -check_status_single lcli2 $B_AMOUNT $B_FEE "" $A_AMOUNT $A_FEE '{ "msatoshis" : '$HTLC_AMOUNT', "expiry" : { "second" : '$EXPIRY' }, "rhash" : "'$RHASH'" } ' + # Node 2 has it committed. + check_status_single lcli2 $B_AMOUNT $B_FEE "" $A_AMOUNT $A_FEE '{ "msatoshis" : '$HTLC_AMOUNT', "expiry" : { "second" : '$EXPIRY' }, "rhash" : "'$RHASH'" } ' -# Now node2 gives commitment to node1. -lcli2 commit $ID1 + # Now node2 gives commitment to node1. + lcli2 commit $ID1 +else + A_AMOUNT=$(($A_AMOUNT - $EXTRA_FEE - $HTLC_AMOUNT)) + A_FEE=$(($A_FEE + $EXTRA_FEE)) +fi + +# Both should have committed tx. check_status $A_AMOUNT $A_FEE '{ "msatoshis" : '$HTLC_AMOUNT', "expiry" : { "second" : '$EXPIRY' }, "rhash" : "'$RHASH'" } ' $B_AMOUNT $B_FEE "" if [ -n "$STEAL" ]; then @@ -407,8 +426,8 @@ if [ -n "$DUMP_ONCHAIN" ]; then fi lcli2 fulfillhtlc $ID1 $SECRET -lcli2 commit $ID1 -lcli1 commit $ID2 +[ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1 +[ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2 # We've transferred the HTLC amount to 2, who now has to pay fees, # so no net change for A who saves on fees. @@ -423,8 +442,8 @@ check_status $A_AMOUNT $A_FEE "" $B_AMOUNT $B_FEE "" HTLC_AMOUNT=100000000 lcli1 newhtlc $ID2 $HTLC_AMOUNT $EXPIRY $RHASH -lcli1 commit $ID2 -lcli2 commit $ID1 +[ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2 +[ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1 # Check channel status A_AMOUNT=$(($A_AMOUNT - $EXTRA_FEE - $HTLC_AMOUNT)) @@ -432,8 +451,8 @@ A_FEE=$(($A_FEE + $EXTRA_FEE)) check_status $A_AMOUNT $A_FEE '{ "msatoshis" : '$HTLC_AMOUNT', "expiry" : { "second" : '$EXPIRY' }, "rhash" : "'$RHASH'" } ' $B_AMOUNT $B_FEE "" lcli2 failhtlc $ID1 $RHASH -lcli2 commit $ID1 -lcli1 commit $ID2 +[ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1 +[ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2 # Back to how we were before. A_AMOUNT=$(($A_AMOUNT + $EXTRA_FEE + $HTLC_AMOUNT)) @@ -443,8 +462,8 @@ check_status $A_AMOUNT $A_FEE "" $B_AMOUNT $B_FEE "" # Same again, but this time it expires. HTLC_AMOUNT=10000001 lcli1 newhtlc $ID2 $HTLC_AMOUNT $EXPIRY $RHASH -lcli1 commit $ID2 -lcli2 commit $ID1 +[ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2 +[ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1 # Check channel status A_AMOUNT=$(($A_AMOUNT - $EXTRA_FEE - $HTLC_AMOUNT)) @@ -458,8 +477,8 @@ lcli1 dev-mocktime $MOCKTIME # This should make node2 send it. MOCKTIME=$(($MOCKTIME + 31)) lcli2 dev-mocktime $MOCKTIME -lcli2 commit $ID1 -lcli1 commit $ID2 +[ ! -n "$MANUALCOMMIT" ] || lcli2 commit $ID1 +[ ! -n "$MANUALCOMMIT" ] || lcli1 commit $ID2 # Back to how we were before. A_AMOUNT=$(($A_AMOUNT + $EXTRA_FEE + $HTLC_AMOUNT))