We previously tried to use the commitment tx to create an initial
feerate range, then refine it as we look at each HTLC tx. This was
wrong, because the commitment tx can underpay fees (if it can't afford
it), and our estimate of the maximum possible feerate would be too low.
Now, we only have two fees we need to figure out: HTLC timeout txs and
HTLC success txs, so simply grind them on first use.
Fixes: #1290
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In particular, the main daemon and subdaemons share the backtrace code,
with hooks for logging.
The daemon hook inserts the io_poll override, which means we no longer
need io_debug.[ch]. Though most daemons don't need it, they still link
against ccan/io, so it's harmess (suggested by @ZmnSCPxj).
This was tested manually to make sure we get backtraces still.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is necessary since we have onchaind tell us about the
their_unilateral/to_us output, after it is already in a block.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
I did a brief audit of tmpctx uses, and we do leak them in various
corner cases. Fortunely, all our daemons are based on some kind of
I/O loop, so it's fairly easy to clean a global tmpctx at that point.
This makes things a bit neater, and slightly more efficient, but also
clearer: I avoided creating a tmpctx in a few places because I didn't
want to add another allocation. With that penalty removed, I can use
it more freely and hopefully write clearer code.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We say "in N blocks" but we actually mean "N blocks after this tx" which is
actually N-1 or less. Change wording and tighten tests which misunderstood
this.
Also, the 'assert not l1.daemon.is_in_log('onchaind complete, forgetting peer')'
are unlikely to work until the daemon has actually seen the block, so add
sync_blockheight before all of those.
These changes reveal some sloppy testing, which we fix.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
With the following patch applied, we could clearly see onchaind try to
broadcast the timeout tx one block too early:
sendrawtx exit 26, gave error code: -26?error message:?non-final (code 64)?
This is because of an out-by-one error in calculating the relative
depth required, since the out->tx_blockheight is already 1 before the
current block.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The root cause of #1114 was that the restarted onchaind created a
different proposal to the one which had previously been mined:
2018-03-01T09:41:08.884Z lightningd(1): lightning_onchaind-020d3d5995a973c878e3f6e5f59da54078304c537f981d7dcef73367ecbea0e90e chan #1: STATUS_FAIL_INTERNAL_ERROR: THEIR_UNILATERAL/OUR_HTLC spent with weird witness 3
After the previous patches which fixed the output address difference,
we could identify proposals by their outputs, but during the
transition (onchaind started with old buggy version, restarted now)
that wouldn't be right, so we match the inputs, discarding signatures
which will be different. This works for all current cases.
Closes: #1114
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The billboard is now far more useful to tell what's going on, and this
gets us closer to a state == owner mapping.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This uses the permanent slot to indicate what's happening overall, and
the transient slot is updates with what we expect to happen next.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I generally tried not to alter internal logic to add billboards (to avoid
breakage), but these two make things neater.
1. Free ->proposal if it's not longer valid. That way we don't get confused
by reporting old proposals.
2. Change all_irrevocably_resolved() to num_not_irrevocably_resolved() so
we can report that number to the billboard.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We use the permanent slot to indicate our overall negotiation range,
and the transient slot to say what we're waiting for.
On success, we update the permanent slot to indicate the final value.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We always hand in "NULL" (which means use tal_len on the msg), except
for two places which do that manually for no good reason.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And now we can finally do the db upgrade to remove any OPENINGD
channels once, since we never put them back.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Now we have wirestring, this is much more natural. And with the
24M length limit, we needn't be so concerned about dumping 64k peer
messages in hex.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Our handling of SIGPIPE was incoherent and inconsistent, and we had much
cut & paste between the daemons. They should *ALL* ignore SIGPIPE, and
much of the rest of the boilerplate can be shared, so should be.
Reported-by: @ZmnSCPxjFixes: #528
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is the only case in which we don't respend to a simple keyindex'd
pubkey, so we need to handle this for future spends.
Signed-off-by: Christian Decker <decker.christian@gmail.com>
I was examining a test_onchain_timeout failure, and realized that we
were forgetting a peer even though we'd just spent the HTLC_TIMEOUT_TX!
This reveals that we weren't resolving an output when we stole the preimage
from it, like we should.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
OUR_HTLC_TIMEOUT_TO_US = normal tx, used to timeout htlc in their commit tx.
OUR_HTLC_TIMEOUT_TX = dual-sig tx with delay, used to timeout htlc in our commit tx.
Only one test looks at that string, so fix that too.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's just a sha256_double, but importantly when we convert it to a
string (in type_to_string, which is used in logging) we use
bitcoin_txid_to_hex() so it's reversed as people expect.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We create a temporary tx which is a child of the real tx, for simplicity of
marshalling. That's OK.
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>
There were two bugs here. First, grind_feerate() needs to check the
actual range of feerates, not the same rate over and over! Secondly,
we need to grind the feerate for the HTLC-success tx, too.
These were masked by the fact that our tests always use the same feerate!
"Untested code is buggy code"
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>
practicalswift
Rusty Russell
Christian Decker