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.
- moves offset into GossipHeader hdr which is passed to all constuctors
- reads .flags as u16 instead of extracting it from the .length, see 0274d88ba
- adds zombie and ratelimit flag to GossipHeader
- bytes_read start at 0 instead of 1 which is more correct,
the one byte is then corrected for when setting the offset of new header.
- bytes_read is increased in pull_bytes as this is the only place where
something is read
- use new style for various format-strings
It's defined to be nonull:
```
channeld/channeld.c:2381:2: runtime error: null pointer passed as argument 1, which is declared to never be null
/usr/include/stdlib.h:856:3: note: nonnull attribute specified here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior channeld/channeld.c:2381:2 in
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
With the warning that we were trying to put "inf" into a u64, we can
see that this calculation was wrong to use integers!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
For example, if we use -fsanitize=undefined, we can't do unaligned
integer access, but since we didn't test with the sanitizer flags, we
didn't know this, and set `HAVE_UNALIGNED_ACCESS=1`.
Also, add -fno-sanitize-recover= in developer mode, so we actually
fail binaries if something is detected.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Most importantly, configurator used to use bitshifts on signed
integers which -fsanitize=undefined caught.
But also, tal played fast and loose with typing and aliases, which was
a signficant amount of rework.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
pubkey_from_hexstr() was failing, which we didn't notice because we
weren't checking the return value. The problem was that we were passing
it a strlen that was half the actual length.
Relevant error:
[libsecp256k1] illegal argument: !secp256k1_fe_is_zero(&ge->x)
==417723== ERROR: libFuzzer: deadly signal
#7 0x7f5deaacc7fb in abort
#8 0x51b0b0 in secp256k1_default_illegal_callback_fn secp256k1.c
#9 0x51bd8e in secp256k1_ec_pubkey_serialize
#10 0x4e235b in pubkey_to_der bitcoin/pubkey.c:29:7
#11 0x4e2941 in pubkey_cmp bitcoin/pubkey.c:89:2
#12 0x4e333d in bitcoin_redeem_2of2 bitcoin/script.c:144:6
#13 0x4f1396 in run tests/fuzz/fuzz-close_tx.c:78:19
