In particular, we got a segv because we were measuring the wrong
wscript, then we miswired the inputs. It only worked because our
current steal tests don't have a to_us_idx output.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
db_forget_peer() was harmless, but we haven't been entered into the
database yet anyway, and it asserted that we should have been STATE_CLOSED.
Closes: #67
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We don't need it for testing at the moment, and if we do it'll have
to change to relative anyway now we're going to use time_mono().
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's possible that we won't have sent the anchor, but state is
committed in db. And our current philosophy is that we retransmit all
the txs dumbly, all the time.
Our --restart --timeout-anchor test trigger this case, too, so
re-enable that now.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Importantly, they're now entirely block driven. We don't use
dev-setmocktime at all any more.
This also fixes a bug if we run the test twice against the same
bitcoind; we need to extract the time from the block header rather
than assuming bitcoind is on the current time.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Instead of using wall-clock time, we use blocks. This is simpler and
better for database restores. And both sides will time out.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This allows us to add a new field for a callback at the end, but
more subtle, ensures broadcast in order (which simplifies testing).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Re-enabling the next test revealed bugs: if we need to retransmit the
initial open_commit_sig packet, we currently tried to send it as an
UPDATE_COMMIT, which isn't allowed. Fixing that revealed that if
we have to retransmit the initial open, we didn't do that either.
Thus the initial open should count towards the ack count, and we should
special case transmissions of 0 (pkt_open) and 1
(pkt_open_commit_sig).
We also save those early state changes to the database.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The simplest way is to always use peer_received_unexpected_pkt() which
sends the error packet, and ensure it doesn't do so in response to
pkt_err.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It doesn't actually help here; we only did it because we differentiate
the states later, and with refactoring we do that via the explicit
offer_anchor flag.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This means we can now do all database changes, including db_set_visible_state,
within a single transaction (ie. atomically).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Since we no longer feed it into state.c, we can just us a bool.
And that's the last of the CMD_* in the enum state_input, so remove them
all.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We can get weird errors when we try to load a database of a different
from. Just slap a git version in there for now.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Pierre points out that we don't handle this, and it can happen due
to race; the spec says we are not supposed to send PKT_CLOSE with
uncommitted changes.
Closes: #29
Reported-by: Pierre-Marie Padiou
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This means running 3 bitcoinds, which is slow enough to start on my laptop
that I need to increase the startup wait for 30 to 60 seconds, and similarly
the test.sh check loop.
Before: real 13m42.868s
After: real 8m19.563s (make -j3)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Waiting until lightningd is up is too long: do a --version test in setup,
and then check that all reported versions match later on.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Otherwise if they reconnect, we hit the assert in recv_body:
assert(!peer->inpkt);
Found by testing on my build box *without* valgrind (so it was fast
enough to do this).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Running on my build machine, without valgrind, it managed to exchange
closing sigs before restart, and spotted this bug.
Fixes: #76
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
broadcast_remainder() does two things: get the error message for the
previous transaction, and send the next one (shrinking the array).
But it has two bugs:
1) It logs results on the tx at the end of the array, which is the one
it is *about* to send, and
2) The initial caller (rebroadcast_txs) hands it the complete array,
so the first tx gets broadcast twice.
The correct thing to do is to strip the array, then send the tail for
the next callback. And use nicely-named vars to help document what
we're doing.
Reported-by: Christian Decker
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Now broadcast_tx() doesn't take ownership of the tx, make sure callers
free; a bit of refactoring to make it clear when we're making a new tx
vs. accessing an existing one, to make this clearer.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is in preparation for the next step.
Note that we now don't add it to the linked list of txs we've send
until after it's sent by the immediate callback; this means it won't
get broadcast by the timer until after it's been done by broadcast_tx.
Also, this means we no longer steal the tx in broadcast_tx(); but we'll fix
up the leaks 4 patches later.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We don't simply parent them on the object, we use a dummy object which tells
us not to call the callback if freed.
This would be better fixed by rewriting ccan/io to handle tal_free() of
a conn; then we could simply parent the entire thing and forget about it.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Testnet fees are all over the place: don't close a connection due to
that.
Closes: #59
Reported-by: Thomas Daede <daede003@umn.edu>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This fixes half of #59 (the part which crashes). It doesn't fix the
fact that we should never be doing this for testnet.
Reported-by: Thomas Daede <daede003@umn.edu>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Allocate the route off the current command, not dstate. And in the
case where the route is somehow not via a peer, don't leak memory.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Generally, the pattern is: everything returned is allocated off the return
value, which is the only thing allocated off the context. And it's always
freed.
Also, tal_free() returns NULL, so it's useful for one-line error
cleanups.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This shouldn't happen, but if our RNG is busted and get a duplicate,
r would be NULL.
(scan-build found this)
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
`listinvoice` was not handling numeric labels correctly if they are not
passed in as JSON strings (which was the behavior of
`lightning-cli`). So now we accept both string labels as well as numeric
labels.
