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>
It was once only called on failure, now it's always called (if set).
It was called different things in different places, so unify it.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We always call channel_fail_transient() on all channels when a peer
connects, to clean up any previous connections. However, when
we startup, this channel doesn't have an owner yet, resulting in
a fairly weird INFO level message.
Reported-by: Michael Schmook @mschmook
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: `lightningd`: don't log gratuitous "Peer transient failure" message on first connection after restart.
There are cases (difficult to reproduce with a test) where
a payment will fail one time and succeed later.
As far I understand in this case the groupid field of the payment
is the same, and the only thing that change is the status, so
our logic inside the delpay is ambiguous where it is not
possible to delete a payment as described in https://github.com/ElementsProject/lightning/issues/6114
A sequence of commands that explain the problem is
```
$ lc -k listpays payment_hash=H
{
"pays": [
{
"bolt11": "I",
"destination": "redacted",
"payment_hash": "H",
"status": "complete",
"created_at": redacted,
"completed_at": redacted,
"preimage": "P",
"amount_msat": "redacted",
"amount_sent_msat": "redacted"
}
]
}
$ lc delpay H complete
{
"code": 211,
"message": "Payment with hash H has failed status but it should be complete"
}
```
In this case, the delpay is not able to delete a payment because the
listpays is returning only the succeeded one, so by running the
listsendpays we may see the following result where our delpay logic
will be stuck because it works to ensure that all the payments stored
in the database has the status specified by the user
```
➜ VincentSSD clightning --testnet listsendpays -k payment_hash=7fc74bedbb78f2f3330155d919a54e730cf19c11bc73e96c027f5cd4a34e53f4
{
"payments": [
{
"id": 322,
"payment_hash": "7fc74bedbb78f2f3330155d919a54e730cf19c11bc73e96c027f5cd4a34e53f4",
"groupid": 1,
"partid": 1,
"destination": "030b686a163aa2bba03cebb8bab7778fac251536498141df0a436d688352d426f6",
"amount_msat": 300,
"amount_sent_msat": 1664,
"created_at": 1679510203,
"completed_at": 1679510205,
"status": "failed",
"bolt11": "lntb1pjpkj4xsp52trda39rfpe7qtqahx8jjplhnj3tatxy8rh6sc6afgvmdz7n0llspp50lr5hmdm0re0xvcp2hv3nf2wwvx0r8q3h3e7jmqz0awdfg6w206qdp0w3jhxarfdenjqargv5sxgetvwpshjgrzw4njqun9wphhyaqxqyjw5qcqp2rzjqtp28uqy77te96ylt7ek703h4ayldljsf8rnlztgf3p8mg7pd0qzwf8a3yqqpdqqqyqqqqt2qqqqqqgqqc9qxpqysgqgeya2lguaj6sflc4hx2d89jvah8mw9uax4j77d8rzkut3rkm0554x37fc7gy92ws9l76yprdva2lalrs7fqjp9lcx40zuty8gca0g5spme3dup"
},
{
"id": 323,
"payment_hash": "7fc74bedbb78f2f3330155d919a54e730cf19c11bc73e96c027f5cd4a34e53f4",
"groupid": 1,
"partid": 2,
"destination": "030b686a163aa2bba03cebb8bab7778fac251536498141df0a436d688352d426f6",
"amount_msat": 300,
"amount_sent_msat": 3663,
"created_at": 1679510205,
"completed_at": 1679510207,
"status": "failed"
},
{
"id": 324,
"payment_hash": "7fc74bedbb78f2f3330155d919a54e730cf19c11bc73e96c027f5cd4a34e53f4",
"groupid": 1,
"partid": 3,
"destination": "030b686a163aa2bba03cebb8bab7778fac251536498141df0a436d688352d426f6",
"amount_msat": 300,
"amount_sent_msat": 3663,
"created_at": 1679510207,
"completed_at": 1679510209,
"status": "failed"
},
{
"id": 325,
"payment_hash": "7fc74bedbb78f2f3330155d919a54e730cf19c11bc73e96c027f5cd4a34e53f4",
"groupid": 1,
"partid": 4,
"destination": "030b686a163aa2bba03cebb8bab7778fac251536498141df0a436d688352d426f6",
"amount_msat": 300,
"amount_sent_msat": 4663,
"created_at": 1679510209,
"completed_at": 1679510221,
"status": "complete",
"payment_preimage": "43f746f2d28d4902489cbde9b3b8f3d04db5db7e973f8a55b7229ce774bf33a7"
}
]
}
```
This commit solves the problem by forcing the delete query in the
database to specify status too, and work around this kind of
ambiguous case.
Fixes: f52ff07558 (lightningd: allow delpay to delete a specific payment.)
Reported-by: Antoine Poinsot <darosior@protonmail.com>
Link: https://github.com/ElementsProject/lightning/issues/6114
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Co-Developed-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: delpay be more pedantic about delete logic by allowing
delete payments by status directly on the database.
Changelog-Added: JSON-RPC: `listclosedchannels` to show old, dead channels we previously had with peers.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Vincenzo Palazzo
Rusty Russell
Christian Decker