If send_htlc_out() fails, it doesn't initialize pc->out; that can
make us think it's still in progress.
Reported-by: Jonas Nick
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
When gossipd sends a message, have a gossip_index. When it gets back a
peer, the current gossip_index is included, so it can know exactly where
it's up to.
Most of this is mechanical plumbing through openingd, channeld and closingd,
even though openingd and closingd don't (currently) read gossip, so their
gossip_index will be unchanged.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
All peers come from gossipd, and maintain an fd to talk to it. Sometimes
we hand the peer back, but to avoid a race, we always recreated it.
The race was that a daemon closed the gossip_fd, which made gossipd
forget the peer, then master handed the peer back to gossipd. We stop
the race by never closing the gossipfd, but hand it back to gossipd
for closing.
Now gossipd has to accept two fds, but the handling of peers is far
clearer.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
As demonstrated in the test at the end of this series, openingd dying
spontaneously causes the conn to be freed which causes the subd to be
destroyed, which fails the peer, which hits the db.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Rather than using the destructor, hook up the cmd so we can close it.
peers are allocated off ld, so they are only destroyed explicitly.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We are still generating only char* style aliases, but the field is
defined to be unicode, which doesn't mix too well with char.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
We don't use it yet, but now we'll decode correctly.
See: https://github.com/lightningnetwork/lightning-rfc/pull/317
lightning-rfc commit: ef053c09431442697ab46e83f9d3f86e3510a18e
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Change all calls to use the correct serialization and deserialization
functions, include the correct headers and remove the control
messages.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
The master now hands channeld either an error code, and channeld
generates the error message, or an error message relayed from another
node to pass through.
This doesn't fill in the channel_update yet: we need to wire up gossipd
to give us that.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Currently lightningd does this, but channeld is perfectly capable of doing it.
channeld is also in a far better position to add channel_updates to it.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
estimatesmartfee 4 ECONOMICAL was too high for lnd, so drop it, with some
increased security risk.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The filter is being populated while initializing the daemon and by
adding new keys as they are being generated. The filter is then used
in connect_block to identify transactions of interest.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
This is mainly used to filter for transactions that may be of interest
to us, i.e., whether one of our keys is the recipient. It currently
does onyl simple scriptpubkey checks, but will eventually be extended
to use bloomfilters and add more sophisticated checks.
For now the goal is to speed up the processing of blocks during startup.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
This addresses a performance regression introduced by
6ceb375650. We were storing it in an
otherwise empty DB transaction, which means that DB transaction was no
longer a no-op. Now we defer storing until we need to store the
corresponding HTLC anyway, so we can just piggyback on top of that
transaction.
This is also more consistent since we'd be forgetting the payment
anyway if we restart between adding the HTLC and committing to it.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
We only send them when we're not awaiting revoke_and_ack: our
simplified handling can't deal with multiple in flights.
Closes: #244
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The wire protocol uses this, in the assumption that we'll never see feerates
in excess of 4294967 satoshi per kiloweight.
So let's use that consistently internally as well.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Depending on what we're doing, we can want different ones. So use
IMMEDIATE (estimatesmartfee 2 CONSERVATIVE), NORMAL (estimatesmartfee
4 ECONOMICAL) and SLOW (estimatesmartfee 100 ECONOMICAL).
If one isn't available, we try making each one half the previous.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This means we convert it when retrieving from bitcoind; internally it's
always satoshi-per-1000-weight aka millisatoshi-per-weight.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Test objects must be added to $(ALL_OBJS) so they correctly depend on
CCAN headers etc.
Also, each test in a subdir must depend on headers and src in the parent
directory, as it will often #include them directly.
Reported-by: Christian Decker
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Our testsuite uses --dev-fail-on-subdaemon-fail, so I didn't notice this
until I turned that off to chase a bug.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
All the callers need to pass it in: currently channeld and openingd just
fake it by copying the payment point.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
There were two bugs: we weren't returning the next from the given
label but the one matching the label, and we were appending new
invoices to the head instead of the tail, which meant we'd be
traversing in the wrong order.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Thought we don't handle it at the moment, nodes can certainly have multiple
addresses, and we should display them all.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We don't track them accurately when in onchaind, but we don't want to:
onchaind can be restarted at any time.
Once it's all settled, we're clear to clean them up.
Before this, valgrind could complain about deferncing hout->key.peer:
Valgrind error file: valgrind-errors.10876
==10876== Invalid read of size 4
==10876== at 0x41F8AF: peer_on_chain (peer_control.h:127)
==10876== by 0x42340D: notify_new_block (peer_htlcs.c:1461)
==10876== by 0x40A08D: connect_block (chaintopology.c:96)
==10876== by 0x40A96B: topology_changed (chaintopology.c:313)
==10876== by 0x40AC85: add_block (chaintopology.c:384)
==10876== by 0x40ABF0: gather_previous_blocks (chaintopology.c:363)
==10876== by 0x4051B3: process_rawblock (bitcoind.c:410)
==10876== by 0x4044DD: bcli_finished (bitcoind.c:155)
==10876== by 0x454665: destroy_conn (poll.c:183)
==10876== by 0x454685: destroy_conn_close_fd (poll.c:189)
==10876== by 0x45DF89: notify (tal.c:240)
==10876== by 0x45E43A: del_tree (tal.c:400)
==10876== Address 0x6929208 is 2,120 bytes inside a block of size 2,416 free'd
==10876== at 0x4C2EDEB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==10876== by 0x45E513: del_tree (tal.c:421)
==10876== by 0x45E849: tal_free (tal.c:509)
==10876== by 0x41A8E9: handle_irrevocably_resolved (peer_control.c:1172)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We are announcing that we are willing to accept incoming payments with
current_height + min_final_cltv_expiry + slack, assuming that the
sender adds some slack. In particular we'd reject the payment if
slack=0 which is allowed by the spec.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
We save location where transaction was started, in case we try to nest.
There's now no error case; db_exec_mayfail() is the only one.
This means the tests need to override fatal() if they want to intercept
these errors.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is a subset of a "bitcoind: wrap callbacks in transaction." from
the everything-in-transaction branch, but we need the ld pointer now.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And nail "make check-source" to that specific version (which is a commit id,
not a branch name, so needs a different syntax for git).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
These need to be different for testing the example in BOLT 11.
We also use the cltv_final instead of deadline_blocks in the final hop:
various tests assumed 5 was OK, so we tweak utils.py.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It crashes under valgrind, causing a valgrind error: valgrind gives us a
backtrace anyway, so we don't need it.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Normally, we get an error as soon as we send WIRE_REVOKE_AND_ACK. But if the
commit timer goes off, we get some extra cycles, during which the other side
can reconnect. In this case, we simply kill the channeld before it fails,
and never check for the permfail string.
b'lightning_channeld(18613): TRACE: dev_disconnect: -WIRE_REVOKE_AND_ACK'
b'lightning_channeld(18613): TRACE: Trying commit'
b'lightning_channeld(18613): TRACE: htlc 0: SENT_ADD_REVOCATION->SENT_ADD_ACK_COMMIT'
b'lightning_channeld(18613): TRACE: htlc added REMOTE: local +0 remote -200000000'
b'lightning_channeld(18613): TRACE: sending_commit: HTLC REMOTE 0 = SENT_ADD_ACK_COMMIT/RCVD_ADD_ACK_COMMIT'
b'lightning_gossipd(18590): TRACE: Responder: Act 1'
b'lightning_channeld(18613): TRACE: Derived key 034aab0b5cb755de836cffb34c053ba115fba6fe75414e8f56261e23c80eabb1fe from basepoint 03e0a7bb422b254f54bc954be05bd6823a7b7a4b996ff8d3079ca211590fb5df39, point 02f3bf525b6ca595bf85d63e89c95fc59c0fde3ae434b55c8093bbb5c64849da37'
b'lightningd(18465): Connected json input'
b'lightningd(18465):jcon fd 16: Success'
b'lightningd(18465):jcon fd 16: Closing (Bad file descriptor)'
b'lightning_gossipd(18590): TRACE: Responder: Act 2'
b'lightning_gossipd(18590): TRACE: Responder: Act 3'
b'lightning_gossipd(18590): UPDATE WIRE_GOSSIP_PEER_CONNECTED'
b'lightning_gossipd(18590): UPDATE WIRE_GOSSIP_PEER_CONNECTED'
b'lightningd(18465): peer 0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518: Peer has reconnected, state CHANNELD_NORMAL'
b'lightning_channeld(18613): Status closed, but not exited. Killing'
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And we report these through the getpeers JSON RPC again (carefully: in
our reconnect tests we can get duplicates which this patch now filters
out).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In future it will have TOR support, so the name will be awkward.
We collect the to/fromwire functions in common/wireaddr.c, and the
parsing functions in lightningd/netaddress.c.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
They don't currently, since callers check, but be safe. In addition,
handle NULL returns from these in the bitcoind code.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
There are others, but they really are casued by bad failure. We need a
parachute system for these.
Closes: #176
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is a bit messier than I'd like, but we want to clearly remove all
dev code (not just have it uncalled), so we remove fields and functions
altogether rather than stub them out. This means we put #ifdefs in callers
in some places, but at least it's explicit.
We still run tests, but only a subset, and we run with NO_VALGRIND under
Travis to avoid increasing test times too much.
See-also: #176
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
test_routing_gossip (__main__.LightningDTests) ... lightningd: Outstanding taken pointers: lightningd/peer_control.c:2352:towire_errorfmt(ld, ((void *)0), "Can't resolve your address")
This caused by the other end closing due to the next bug.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
There is a race we see sometimes under valgrind on Travis which shows
gossipd receiving the node_announce from master before it reads the
channel_announce from channeld, and thus fails. The simplest solution
is to send the channel_announce and channel_update to master as well,
so it can ensure it sends them to gossipd in order
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It makes it impossible to embed an ipaddr in another structure, since we
always try to skip over any zeroes, which may swallow a following field.
Do the skip specially for the case where we're parsing routing messages:
we never use padding for our own internal messages anyway.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Makes it easier to compare before/after failures. Ideally, we should
run under Travis both with this option and with the seed based on the
entire tmp path (which is still reproducible with determination, but
not fixed every run like this is).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
There are now only two kinds of subdaemons: global ones (hsmd, gossipd) and
per-peer ones. We can handle many callbacks internally now.
We can have a handler to set a new peer owner, and automatically do
the cleanup of the old one if necessary, since we now know which ones
are per-peer.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We currently rely on a zero exit status. That's the only difference between
onchain finished handling and other per-peer daemons, so instead we should
have an explicit "done" message. This is both clearer, and allows us to
unify.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Now the flow is much simpler from a lightningd POV:
1. If we want to connect to a peer, just send gossipd `gossipctl_reach_peer`.
2. Every new peer, gossipd hands up to lightningd, with global/local features
and the peer fd and a gossip fd using `gossip_peer_connected`
3. If lightningd doesn't want it, it just hands the peerfd and global/local
features back to gossipd using `gossipctl_handle_peer`
4. If a peer sends a non-gossip msg (eg `open_channel`) the gossipd sends
it up using `gossip_peer_nongossip`.
5. If lightningd wants to fund a channel, it simply calls `release_channel`.
Notes:
* There's no more "unique_id": we use the peer id.
* For the moment, we don't ask gossipd when we're told to list peers, so
connected peers without a channel don't appear in the JSON getpeers API.
* We add a `gossipctl_peer_addrhint` for the moment, so you can connect to
a specific ip/port, but using other sources is a TODO.
* We now (correctly) only give up on reaching a peer after we exchange init
messages, which changes the test_disconnect case.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We're going to make the ip/port optional, so they should go at the end.
In addition, using ip:port is nicer, for gethostbyaddr().
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We have to do a dance when we get a reconnect in openingd, because we
don't normally expect to free both owner and peer. It's a layering
violation: freeing a peer should clean up the owner's pointer to it,
to avoid a double free, and we can eliminate this dance.
The free order is now different, and the test_reconnect_openingd was
overprecise.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This fixes the only case where the master currently has to write directly
to the peer: re-sending an error. We make gossipd do it, by adding
a new gossipctl_fail_peer message.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In particular, the main daemon needs to pass it about (marshal/unmarshal)
but it won't need to actually use it after the next patch.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We pull them from the database on-demand, where we're storing them
anyway. No need to keep them in memory as well.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
No idea why we were iterating over the list of stubs and then passing
in the index instead of a pointer to the stub directly.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
This wires in the loading of `struct htlc_stub`s on-demand when
starting `onchaind` so that we don't need to keep them in memory.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
So far we were tracking the status by including it either in the paid
or the unpaid list. This refactor makes the state explicit, which
matches the planned DB schema much better.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
While loading HTLCs from the database we might not yet have all the
incoming HTLCs loaded when loading a dependent htlc_out. So we defer
the wiring of the HTLCs until we are sure we have them loaded.
This is also the first step towards keeping that association only in
the database, since otherwise we cannot selectively load channels from
DB.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Especially when testing we might want to disable the automatic
reconnection logic in order not to masquerade bugs that disappear when
reconnecting.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Seems to go out to lunch on reorgs:
+136792.168286138 lightningd(9465):BROKEN: bitcoin-cli getchaintips exited 28: 'error code: -28
error message:
Rewinding blocks...
Closes: #286
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We don't hit this in testing, since we wait for startup already. Hacking
tests to avoid that, I tested this code by hand.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Using pc after free in the pay_command_destroyed destructor, so
we just steal cmd onto pc so free order is the one we want.
[ Edit: expanded comment, split commit ]
Signed-off-by: Christian Decker <decker.christian@gmail.com>
So far only happens during normal shutdown, but it may happen in other
cases as well. We simply define a new destructor that unregisters the
`cmd` from the `jcon`.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
These were fun to hunt down. The jcon and the conn are allocated off
of ld, so the free order is unspecified and if conn is freed before
conn then the finish_jcon destructor uses conn after free.
[ Edit: split commit, modified to use a destructor directly on jcon,
which is more robust than relying on it only being freed via conn --RR ]
Signed-off-by: Christian Decker <decker.christian@gmail.com>
peer_fail_permanent() frees peer->owner, but for bad_peer() we're
being called by the sd->badpeercb(), which then goes on to
io_close(conn) which is a child of sd.
We need to detach the two for this case, so neither tries to free the
other.
This leads to a corner case when the subd exits after the peer is gone:
subd->peer is NULL, so we have to handle that too.
Fixes: #282
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We have a race where we start onchaind, but state is unchanged, so checks
like peer_control.c's:
peer_ready = (peer->owner && peer->state == CHANNELD_AWAITING_LOCKIN);
if (!peer_ready) {
log_unusual(peer->log,
"Funding tx confirmed, but peer state %s %s",
peer_state_name(peer->state),
peer->owner ? peer->owner->name : "unowned");
} else {
subd_send_msg(peer->owner,
take(towire_channel_funding_locked(peer,
peer->scid)));
}
Can send to the wrong daemon.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We were sending a channeld message to onchaind, which was v. confusing
due to overlap. We make all the numbers distinct, which means we can
also add an assert() that it's valid for that daemon, which catches
such errors immediately.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We re-use the value for reasonable_depth given by the master, and we
tell it when our timeout transactions reach that depth.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
When we see an offered HTLC onchain, we need to use the preimage if we
know it. So we dump all the known HTLC preimages at startup, and send
new ones as we discover them.
This doesn't cover preimages we know because we're the final
recipient; that can happen if an HTLC hasn't been irrevocably
committed yet. We'll do that in a followup patch.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
If the HSM is slow it might happen that the timestamp has changed the
second time we come around, so we generate the timestamp externally
and pass it in so we're sure it won't change between calls.
Reported-by: Rusty Russell
Signed-off-by: Christian Decker <decker.christian@gmail.com>
lightningd can crash on shutdown if it's in the middle of getchaintips;
we free the conn, the finished callback is called (process_chaintips),
and it reports that it received an empty result.
The simplest fix is to set a flag in the struct bitcoind destructor,
and avoid the callback.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Either when it exits with a signal, or sends an error status message.
Then we make test_lightningd.py use it.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This change is really to allow us to have a --dev-fail-on-subdaemon-fail option
so we can handle failures from subdaemons generically.
It also neatens handling so we can have an explicit callback for "peer
did something wrong" (which matters if we want to close the channel in
that case).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
1. Remove reference to old $(LIGHTNINGD_OLD_LIB_OBJS) var (in handshaked too).
2. Make check depend directly on unit tests, insteadof weird lightningd/tests
variable.
3. check-source-bolt and check-whitespace are automatic for $(ALL_TEST_PROGRAMS)
so we don't need them here.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is the step where we broadcast the transaction to the network and
a nice place to extract the change from the transaction.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
Rusty Russell
Christian Decker
practicalswift
ZmnSCPxj