CI hit this issue where it would get a tx_abort in fundchannel. This
happens when the dualopend we use to query the feerates has not exited
yet (it waits for the tx_abort reply), and we mistakenly reuse it.
With multi-channel support, this is wrong: just run another one and it
all Just Works.
This means we need to rework our dual_open_control.c logic, since it
would previously create an unsaved channel then not clean up if
something went wrong.
Most people will never try to negotiate opening multiple channels to
the same peer at the same time (vs. having an established channel and
opening a new one), so this case is a bit weird.
```
rates = l1.rpc.dev_queryrates(l2.info['id'], amount, amount)
# l1 leases a channel from l2
l1.rpc.fundchannel(l2.info['id'], amount, request_amt=amount,
feerate='{}perkw'.format(feerate),
> compact_lease=rates['compact_lease'])
tests/test_opening.py:1611:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
contrib/pyln-client/pyln/client/lightning.py:833: in fundchannel
return self.call("fundchannel", payload)
contrib/pyln-testing/pyln/testing/utils.py:721: in call
res = LightningRpc.call(self, method, payload, cmdprefix, filter)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <pyln.testing.utils.PrettyPrintingLightningRpc object at 0x7f6cbcd97950>
method = 'fundchannel'
payload = {'amount': 500000, 'announce': True, 'compact_lease': '029a00640064000000644c4b40', 'feerate': '2000perkw', ...}
cmdprefix = None, filter = None
def call(self, method, payload=None, cmdprefix=None, filter=None):
"""Generic call API: you can set cmdprefix here, or set self.cmdprefix
...
if not isinstance(resp, dict):
raise ValueError("Malformed response, response is not a dictionary %s." % resp)
elif "error" in resp:
> raise RpcError(method, payload, resp['error'])
E pyln.client.lightning.RpcError: RPC call failed: method: fundchannel, payload: {'id': '022d223620a359a47ff7f7ac447c85c46c923da53389221a0054c11c1e3ca31d59', 'amount': 500000, 'feerate': '2000perkw', 'announce': True, 'request_amt': 500000, 'compact_lease': '029a00640064000000644c4b40'}, error: {'code': -1, 'message': 'Abort requested', 'data': {'id': '022d223620a359a47ff7f7ac447c85c46c923da53389221a0054c11c1e3ca31d59', 'method': 'openchannel_init'}}
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-EXPERIMENTAL: Build: all experimental features are now runtime-enabled; no more ./configure --enable-experimental-features
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And no longer insist on opt_quiesce.
Changelog-EXPERIMENTAL: Config: `--experimental-upgrade-protocol` enables simple channel upgrades.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is a regression that we introduced in this release
due to some dirty parts of our codebase.
For historical reasons (I think), we were using a `json_add_sat_only`
procedure defined in `peer_control.c`. So when @rustyrussell removed the _msat,
we thought that all the fields were reflecting the new behavior, but
we were wrong.
This PR fixes this bug and also removes the additional function
from `peer_control.c`. This way, we can be sure that there is no other part
of our codebase that uses this method (except for other `json_add` methods).
Link: https://github.com/ElementsProject/lightning/issues/6244
Reported-by: @hMsats
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
This finally happened on my test box; tests simply stopped. Turns out
we were spinning here, with waitpid returning -1.
Since this is during shutdown, that also explains why pytest under CI
would hang, since timeouts don't apply during test teardown!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We were debugging a number of issues related to the forwarding logic,
when using public scids on private channels, and we noticed that we
are very verbose everywhere, except where it counts, i.e., what
decisions are being taken. So we add a couple of debug logs, and a
final info one that tells the operator which resolution was chosen in
the end.
I tested this indeed breaks if we don't accept it, then implemented
the code to accept it.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: protocol: We now correctly accept the `option_scid_alias` bit in `open_channel` `channel_type`.
Changelog-Deprecated: protocol: Not setting `option_scid_alias` in `option_channel` `channel_type` for unannounced channels.
Now we've set everything up, the replacement code is quite simple.
Some tests now have to deal with RBF though, and our rbf tests need work
since they look for the old onchaind messages.
In particular, when we can't afford the fee we want, we back off to
the next blockcount estimate, rather than spending all on fees
(necessarily). So test_penalty_rbf_burn no longer applies.
Changelog-Changed: Protocol: spending unilateral close transactions now use dynamic fees based on deadlines (and RBF), instead of fixed fees.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We also do it on every block, but since bitcoind can't always be counted
to rebroadcast for us, we might as well be aggressive!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We would only set it the first time, which was OK for how we were using it
before. Now we want to also set it for rebroadcast.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Splitting into onchaind_tx() into onchaind_tx_unsigned() and
sign_and_get_witness() makes it easier to reuse for RBF.
Adding more information in onchain_signing_info is required too.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `feerates`: added `floor` field for current minimum feerate bitcoind will accept
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Fixes: #4473
Changelog-Deprecated: Plugins: `estimatefees` returning feerates by name (e.g. "opening"); use `fee_floor` and `feerates`.
Changelog-Fixed: Plugins: `bcli` now tells us the minimal possible feerate, such as with mempool congestion, rather than assuming 1 sat/vbyte.
Changelog-Added: Plugins: `estimatefees` can return explicit `fee_floor` and `feerates` by block number.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And consolidate descriptions into lightning-feerates().
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `close`, `fundchannel`, `fundpsbt`, `multifundchannel`, `multiwithdraw`, `txprepare`, `upgradewallet`, `withdraw` now allow "minimum" and NN"blocks" as `feerate` (`feerange` for `close`).
Drop try_get_feerate() in favor of explicit feerate_for_deadline() and
smoothed_feerate_for_deadline().
This shows us everywhere we deal with old-style feerates by names.
`delayed_to_us` and `htlc_resolution` will be moving to dynamic fees,
so deprecate those.
Note that "penalty" is still used for generating penalty txs for
watchtowers, and "unilateral_close" still used until we get zero-fee
anchors.
Changelog-Added: JSON-RPC: `feerates` `estimates` array shows fee estimates by blockcount from underlying plugin (usually *bcli*).
Changelog-Changed: JSON-RPC: `close`, `fundchannel`, `fundpsbt`, `multifundchannel`, `multiwithdraw`, `txprepare`, `upgradewallet`, `withdraw` `feerate` (`feerange` for `close`) value *slow* is now 100 block-estimate, not half of 100-block estimate.
Changelog-Deprecated: JSON-RPC: `close`, `fundchannel`, `fundpsbt`, `multifundchannel`, `multiwithdraw`, `txprepare`, `upgradewallet`, `withdraw` `feerate` (`feerange` for `close`) expressed as, "delayed_to_us", "htlc_resolution", "max_acceptable" or "min_acceptable". Use explicit block counts or *slow*/*normal*/*urgent*/*minimum*.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Rather than have specific-purpose levels, have an array of
[blockcount, feerate], and rebuild the specific-purpose levels
for now on top.
We also keep a *separate* smoothed feerate, so you can ask for that
explicitly.
Since all the plugins used the same formula to derive the different
named fee levels, we apply the reverse to return to the underlying
estimates: updating the interface comes next.
This is ugly for now, but various specific-purpose levels will be
going away, as we shift to deadline-driven fees.
This temporarily breaks the floor calculation, so that test is
disabled.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We have the FEERATE_FLOOR constant if you don't care, but usually you want
to use the current bitcoind lower limit, so call get_feerate_floor()
(which is currently the same, but coming!).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Turns out the two bcli replacements I checked (`sauron` and
`trustedcoin`) don't even implement this, and the multiplier makes
more sense in lightningd, especially as we move to bcli just providing
raw feerate estimates.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In particular:
- Bolt 4: add route blinding construction
- Bolt 4: add blinded payments
And this means it's not experimental, so we can turn it on
by default!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: Protocol: blinded payments are now supported by default (not just with `--experimental-onion-messages`)
"Allow nodes to overshoot final htlc amount and expiry (#1032)"
Note that this also renamed `min_final_cltv_expiry` to the more-correct
`min_final_cltv_expiry_delta`.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
"Allow nodes to overshoot the MPP `total_msat` when paying (#1031)"
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Protocol: Allow slight overpaying, even with MPP, as spec now recommends.
"BOLT 4: Remove legacy format, make var_onion_optin compulsory."
This also renamed the redundant "tlv_payload" to "payload", so we
replace "tlv_tlv_payload" with "tlv_payload" everyhere!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This is when they closed the channel, we can simply make our own tx to
expire the HTLC. (The other case is where we closed the channel, and
we have a special htlc_timeout tx which we have their signature for).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This breaks tests/test_closing.py::test_onchain_all_dust's accouting
checks.
That test doesn't really test what it claims to test; sure, onchaind
*says* it's going to ignore the output due to high fees, but the tx
still gets mined.
I cannot figure out what the test is supposed to look like, so I
simply disabled the accounting checks :(
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We add code for the case of spending a (timelocked) to-us output of an
HTLC output, so lightningd can do it (rather than onchaind doing all
the work itself).
onchaind still needs to know whether we bothered to create the tx
(fees might have caused it to evaporate, so it should consider it
immediately resolved rather than waiting for it), and what the
witnesses were, and which parts of the witnesses were signatures (as
these parts might change, with RBF or in future, combining other txs).
The inputs (known to onchaind) and the witnesses (told by lightningd)
uniquely identify the spend for the purposes of onchaind. In
particular, they definitely distinguish HTLC-timeout and HTLC-success
cases.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Importantly, the code in jsonrpc.c which actually does the io_break:
```
/* Once the stop_conn conn is drained, we can shut down. */
if (jcon->ld->stop_conn == conn && jcon->ld->state == LD_STATE_RUNNING) {
/* Return us to toplevel lightningd.c */
log_debug(jcon->ld->log, "io_break: %s", __func__);
io_break(jcon->ld);
```
By having the state not set until later, we avoid running this. Of course,
we need to avoid calling the main loop when we get there, if we've already
been told to shutdown.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Fun story. We're changing onchaind to hand txs to us, and we will
construct them and do the broadcast for it. lightningd tells onchaind
the witness it used (with flags to indicate which fields were
signatures so should be ignored) so onchaind can recognize the tx
when/if it is mined.
And when onchaind was waiting for a CLTV delay, it wouldn't tell
lightningd yet, but wait until the parent was sufficiently deep
But this caused bugs!
In particular, on replay, onchaind would see transactions which it
hasn't sent yet. This was not a problem before, as onchaind had
created the tx, even if it hadn't told lightningd to broadcast it, so
recognized the variant when it came in. When we're relying on
lightningd to tell us what the tx will look like, this doesn't work
any more.
The cause of this is that we fire off txowatches ("this output was
spent!") while we process blocks, and only fire off txwatches ("this
tx increased depth") once all the current blocks are processed. Often
this didn't matter, since we replay messages to onchaind from the
database, *but* we trim the last few blocks on restart (or, if there's
a small reorg while we're stopped), and we can hit this misordering.
Changing our topology code to only ever process one block at a time
would be a solution, but slows down catchup (and tests, where we often
mine a run of blocks).
So, this seems like a premature optimization, but it's really
required! And in future, lightningd can use this knowledge of pending
transactions to combine them in more clever ways.
Note that if a tx is valid at block N, we broadcast it once we see
block N-1, to get it in the mempool for block N.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>