Drop `debug_assert` that would have us panic for replayed `PaymentClaimed`s by tnull · Pull Request #511 · lightningdevkit/ldk-node

tnull · 2025-03-27T12:35:00Z

In cd8958f we changed the internal API behavior of PaymentStore::update. While previously it would return Ok(true) the to-be-updated entry was found in the store, it now returns Ok(true) if not only the entry was found but it was actually updated and re-persisted. This was an improvement as it allows us to avoid unnecessary re-persists if nothing changed.

However, there were 1-2 places that implicitly relied on that behavior for logging purposes which we didn't correctly update to the new behavior. Unfortunately, one instance in handling PaymentClaimed events actually even debug_asserted on the return value, which lead to some unnecessary panics in debug in case PaymentClaimed got replayed. Here we rectify this by dropping the debug_assert.

G8XSU · 2025-03-27T20:03:52Z

- return Err(Error::InvalidPaymentHash);
+ match self.payment_store.update(&update) {
+ Ok(true) => (),
+ Ok(false) => {


slightly confused: how did we determine that it wasn't a hash for which fail_for_hash has already been called hence nothing changed?
iiuc, we will get Ok(false) in that case.

Yes, that's indeed the case, in which case the logging will be slightly off. I considered possible mitigations to this, but all I could think of were rather invasive just to fix this small edge case. Let me know if you think it's very important.

I took a look at the payment_store.update function, and I think it would make sense to return an error with a message like "payment hash not found" in an else block when the payment is not found in the store. This way, returning false would only indicate that no new update was made that is the payment exists but no changes were applied, while a missing payment would result in a clear error.

Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?

Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?
I think this is correct: previously, calling it twice would succeed and have us call fail_htlc_backwards twice if the payment was known at all. Now, we'd only have it succeed once, and would error out on the second attempt and/or if the payment had previously failed already. Added some clarifications to the docs.

I am not sure if this is optimal API design. I've seen that callers oftentimes also need to implement idempotency, and that is difficult now?

As discussed offline, now added a ternary enum that represents all update results.

(now also fixed CI)

vincenzopalazzo

LGMT, but not sure how the update info need to be return Ok(false) and not just a specific error?

Looks like to me that this will make the state machine a little bit more complex?

joostjager · 2025-04-17T08:50:30Z

- );
- debug_assert!(false);
- },
+ Ok(_) => (),


Perhaps document why we can ignore this result and mention the replay possbility?

Okay, if you prefer, although the comment is a bit redundant tbh. Added a fixup.

I actually meant to suggest to explain why this function is called multiple times and that that is the reason for ignoring the result value.

The main reason for ignoring it is that not doing so is a bug stemming from the slight shift in our internal API semantics. Seems weird to document something that changed by now?

Someone might still wonder why the return value is ignored, and to understand this they need to know that the method can be called multiple times. Perhaps it can work too as a comment on the method itself.
It is a question that I had while reviewing, so others might too.

Alright, now added a fixup that expands the comment and also discerns between the new PaymentStoreUpdateResult variants.

joostjager · 2025-04-17T08:56:26Z

- return Err(Error::InvalidPaymentHash);
+ match self.payment_store.update(&update) {
+ Ok(true) => (),
+ Ok(false) => {


Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?

tnull · 2025-04-17T10:19:29Z

CI failure is unrelated and should be fixed by #524

…imed`s In cd8958f we changed the internal API behavior of `PaymentStore::update`. While previously it would return `Ok(true)` the to-be-updated entry was found in the store, it now returns `Ok(true)` if not only the entry was found but it was actually updated and re-persisted. This was an improvement as it allows us to avoid unnecessary re-persists if nothing changed. However, there were 1-2 places that implicitly relied on that behavior for logging purposes which we didn't correctly update to the new behavior. Unfortunately, one instance in handling `PaymentClaimed` events actually even `debug_assert`ed on the return value, which lead to some unnecessary panics in `debug` in case `PaymentClaimed` got replayed. Here we rectify this by dropping the `debug_assert`.

tnull · 2025-04-18T09:40:29Z

Squashed fixups without further changes.

tnull · 2025-03-27T12:35:00Z

Fixes#506

In cd8958f we changed the internal API behavior of PaymentStore::update. While previously it would return Ok(true) the to-be-updated entry was found in the store, it now returns Ok(true) if not only the entry was found but it was actually updated and re-persisted. This was an improvement as it allows us to avoid unnecessary re-persists if nothing changed.

However, there were 1-2 places that implicitly relied on that behavior for logging purposes which we didn't correctly update to the new behavior. Unfortunately, one instance in handling PaymentClaimed events actually even debug_asserted on the return value, which lead to some unnecessary panics in debug in case PaymentClaimed got replayed. Here we rectify this by dropping the debug_assert.

G8XSU · 2025-03-27T20:03:52Z

- return Err(Error::InvalidPaymentHash);
+ match self.payment_store.update(&update) {
+ Ok(true) => (),
+ Ok(false) => {


slightly confused: how did we determine that it wasn't a hash for which fail_for_hash has already been called hence nothing changed?
iiuc, we will get Ok(false) in that case.

Yes, that's indeed the case, in which case the logging will be slightly off. I considered possible mitigations to this, but all I could think of were rather invasive just to fix this small edge case. Let me know if you think it's very important.

I took a look at the payment_store.update function, and I think it would make sense to return an error with a message like "payment hash not found" in an else block when the payment is not found in the store. This way, returning false would only indicate that no new update was made that is the payment exists but no changes were applied, while a missing payment would result in a clear error.

Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?

Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?
I think this is correct: previously, calling it twice would succeed and have us call fail_htlc_backwards twice if the payment was known at all. Now, we'd only have it succeed once, and would error out on the second attempt and/or if the payment had previously failed already. Added some clarifications to the docs.

I am not sure if this is optimal API design. I've seen that callers oftentimes also need to implement idempotency, and that is difficult now?

As discussed offline, now added a ternary enum that represents all update results.

(now also fixed CI)

vincenzopalazzo

LGMT, but not sure how the update info need to be return Ok(false) and not just a specific error?

Looks like to me that this will make the state machine a little bit more complex?

joostjager · 2025-04-17T08:50:30Z

- );
- debug_assert!(false);
- },
+ Ok(_) => (),


Perhaps document why we can ignore this result and mention the replay possbility?

Okay, if you prefer, although the comment is a bit redundant tbh. Added a fixup.

I actually meant to suggest to explain why this function is called multiple times and that that is the reason for ignoring the result value.

The main reason for ignoring it is that not doing so is a bug stemming from the slight shift in our internal API semantics. Seems weird to document something that changed by now?

Someone might still wonder why the return value is ignored, and to understand this they need to know that the method can be called multiple times. Perhaps it can work too as a comment on the method itself.
It is a question that I had while reviewing, so others might too.

Alright, now added a fixup that expands the comment and also discerns between the new PaymentStoreUpdateResult variants.

joostjager · 2025-04-17T08:56:26Z

- return Err(Error::InvalidPaymentHash);
+ match self.payment_store.update(&update) {
+ Ok(true) => (),
+ Ok(false) => {


Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?

tnull · 2025-04-17T10:19:29Z

CI failure is unrelated and should be fixed by #524

…imed`s In cd8958f we changed the internal API behavior of `PaymentStore::update`. While previously it would return `Ok(true)` the to-be-updated entry was found in the store, it now returns `Ok(true)` if not only the entry was found but it was actually updated and re-persisted. This was an improvement as it allows us to avoid unnecessary re-persists if nothing changed. However, there were 1-2 places that implicitly relied on that behavior for logging purposes which we didn't correctly update to the new behavior. Unfortunately, one instance in handling `PaymentClaimed` events actually even `debug_assert`ed on the return value, which lead to some unnecessary panics in `debug` in case `PaymentClaimed` got replayed. Here we rectify this by dropping the `debug_assert`.

tnull · 2025-04-18T09:40:29Z

Squashed fixups without further changes.

tnull · 2025-03-27T12:35:00Z

Fixes#506

In cd8958f we changed the internal API behavior of PaymentStore::update. While previously it would return Ok(true) the to-be-updated entry was found in the store, it now returns Ok(true) if not only the entry was found but it was actually updated and re-persisted. This was an improvement as it allows us to avoid unnecessary re-persists if nothing changed.

However, there were 1-2 places that implicitly relied on that behavior for logging purposes which we didn't correctly update to the new behavior. Unfortunately, one instance in handling PaymentClaimed events actually even debug_asserted on the return value, which lead to some unnecessary panics in debug in case PaymentClaimed got replayed. Here we rectify this by dropping the debug_assert.

G8XSU · 2025-03-27T20:03:52Z

- return Err(Error::InvalidPaymentHash);
+ match self.payment_store.update(&update) {
+ Ok(true) => (),
+ Ok(false) => {


slightly confused: how did we determine that it wasn't a hash for which fail_for_hash has already been called hence nothing changed?
iiuc, we will get Ok(false) in that case.

Yes, that's indeed the case, in which case the logging will be slightly off. I considered possible mitigations to this, but all I could think of were rather invasive just to fix this small edge case. Let me know if you think it's very important.

I took a look at the payment_store.update function, and I think it would make sense to return an error with a message like "payment hash not found" in an else block when the payment is not found in the store. This way, returning false would only indicate that no new update was made that is the payment exists but no changes were applied, while a missing payment would result in a clear error.

Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?

Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?
I think this is correct: previously, calling it twice would succeed and have us call fail_htlc_backwards twice if the payment was known at all. Now, we'd only have it succeed once, and would error out on the second attempt and/or if the payment had previously failed already. Added some clarifications to the docs.

I am not sure if this is optimal API design. I've seen that callers oftentimes also need to implement idempotency, and that is difficult now?

As discussed offline, now added a ternary enum that represents all update results.

(now also fixed CI)

vincenzopalazzo

LGMT, but not sure how the update info need to be return Ok(false) and not just a specific error?

Looks like to me that this will make the state machine a little bit more complex?

joostjager · 2025-04-17T08:50:30Z

- );
- debug_assert!(false);
- },
+ Ok(_) => (),


Perhaps document why we can ignore this result and mention the replay possbility?

Okay, if you prefer, although the comment is a bit redundant tbh. Added a fixup.

I actually meant to suggest to explain why this function is called multiple times and that that is the reason for ignoring the result value.

The main reason for ignoring it is that not doing so is a bug stemming from the slight shift in our internal API semantics. Seems weird to document something that changed by now?

Someone might still wonder why the return value is ignored, and to understand this they need to know that the method can be called multiple times. Perhaps it can work too as a comment on the method itself.
It is a question that I had while reviewing, so others might too.

Alright, now added a fixup that expands the comment and also discerns between the new PaymentStoreUpdateResult variants.

joostjager · 2025-04-17T08:56:26Z

- return Err(Error::InvalidPaymentHash);
+ match self.payment_store.update(&update) {
+ Ok(true) => (),
+ Ok(false) => {


Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?

tnull · 2025-04-17T10:19:29Z

CI failure is unrelated and should be fixed by #524

…imed`s In cd8958f we changed the internal API behavior of `PaymentStore::update`. While previously it would return `Ok(true)` the to-be-updated entry was found in the store, it now returns `Ok(true)` if not only the entry was found but it was actually updated and re-persisted. This was an improvement as it allows us to avoid unnecessary re-persists if nothing changed. However, there were 1-2 places that implicitly relied on that behavior for logging purposes which we didn't correctly update to the new behavior. Unfortunately, one instance in handling `PaymentClaimed` events actually even `debug_assert`ed on the return value, which lead to some unnecessary panics in `debug` in case `PaymentClaimed` got replayed. Here we rectify this by dropping the `debug_assert`.

tnull · 2025-04-18T09:40:29Z

Squashed fixups without further changes.

tnull · 2025-03-27T12:35:00Z

Fixes#506

In cd8958f we changed the internal API behavior of PaymentStore::update. While previously it would return Ok(true) the to-be-updated entry was found in the store, it now returns Ok(true) if not only the entry was found but it was actually updated and re-persisted. This was an improvement as it allows us to avoid unnecessary re-persists if nothing changed.

However, there were 1-2 places that implicitly relied on that behavior for logging purposes which we didn't correctly update to the new behavior. Unfortunately, one instance in handling PaymentClaimed events actually even debug_asserted on the return value, which lead to some unnecessary panics in debug in case PaymentClaimed got replayed. Here we rectify this by dropping the debug_assert.

G8XSU · 2025-03-27T20:03:52Z

- return Err(Error::InvalidPaymentHash);
+ match self.payment_store.update(&update) {
+ Ok(true) => (),
+ Ok(false) => {


slightly confused: how did we determine that it wasn't a hash for which fail_for_hash has already been called hence nothing changed?
iiuc, we will get Ok(false) in that case.

Yes, that's indeed the case, in which case the logging will be slightly off. I considered possible mitigations to this, but all I could think of were rather invasive just to fix this small edge case. Let me know if you think it's very important.

I took a look at the payment_store.update function, and I think it would make sense to return an error with a message like "payment hash not found" in an else block when the payment is not found in the store. This way, returning false would only indicate that no new update was made that is the payment exists but no changes were applied, while a missing payment would result in a clear error.

Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?

Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?
I think this is correct: previously, calling it twice would succeed and have us call fail_htlc_backwards twice if the payment was known at all. Now, we'd only have it succeed once, and would error out on the second attempt and/or if the payment had previously failed already. Added some clarifications to the docs.

I am not sure if this is optimal API design. I've seen that callers oftentimes also need to implement idempotency, and that is difficult now?

As discussed offline, now added a ternary enum that represents all update results.

(now also fixed CI)

vincenzopalazzo

LGMT, but not sure how the update info need to be return Ok(false) and not just a specific error?

Looks like to me that this will make the state machine a little bit more complex?

joostjager · 2025-04-17T08:50:30Z

- );
- debug_assert!(false);
- },
+ Ok(_) => (),


Perhaps document why we can ignore this result and mention the replay possbility?

Okay, if you prefer, although the comment is a bit redundant tbh. Added a fixup.

I actually meant to suggest to explain why this function is called multiple times and that that is the reason for ignoring the result value.

The main reason for ignoring it is that not doing so is a bug stemming from the slight shift in our internal API semantics. Seems weird to document something that changed by now?

Someone might still wonder why the return value is ignored, and to understand this they need to know that the method can be called multiple times. Perhaps it can work too as a comment on the method itself.
It is a question that I had while reviewing, so others might too.

Alright, now added a fixup that expands the comment and also discerns between the new PaymentStoreUpdateResult variants.

joostjager · 2025-04-17T08:56:26Z

- return Err(Error::InvalidPaymentHash);
+ match self.payment_store.update(&update) {
+ Ok(true) => (),
+ Ok(false) => {


Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?

tnull · 2025-04-17T10:19:29Z

CI failure is unrelated and should be fixed by #524

…imed`s In cd8958f we changed the internal API behavior of `PaymentStore::update`. While previously it would return `Ok(true)` the to-be-updated entry was found in the store, it now returns `Ok(true)` if not only the entry was found but it was actually updated and re-persisted. This was an improvement as it allows us to avoid unnecessary re-persists if nothing changed. However, there were 1-2 places that implicitly relied on that behavior for logging purposes which we didn't correctly update to the new behavior. Unfortunately, one instance in handling `PaymentClaimed` events actually even `debug_assert`ed on the return value, which lead to some unnecessary panics in `debug` in case `PaymentClaimed` got replayed. Here we rectify this by dropping the `debug_assert`.

tnull · 2025-04-18T09:40:29Z

Squashed fixups without further changes.

tnull · 2025-03-27T12:35:00Z

Fixes#506

In cd8958f we changed the internal API behavior of PaymentStore::update. While previously it would return Ok(true) the to-be-updated entry was found in the store, it now returns Ok(true) if not only the entry was found but it was actually updated and re-persisted. This was an improvement as it allows us to avoid unnecessary re-persists if nothing changed.

However, there were 1-2 places that implicitly relied on that behavior for logging purposes which we didn't correctly update to the new behavior. Unfortunately, one instance in handling PaymentClaimed events actually even debug_asserted on the return value, which lead to some unnecessary panics in debug in case PaymentClaimed got replayed. Here we rectify this by dropping the debug_assert.

G8XSU · 2025-03-27T20:03:52Z

- return Err(Error::InvalidPaymentHash);
+ match self.payment_store.update(&update) {
+ Ok(true) => (),
+ Ok(false) => {


slightly confused: how did we determine that it wasn't a hash for which fail_for_hash has already been called hence nothing changed?
iiuc, we will get Ok(false) in that case.

Yes, that's indeed the case, in which case the logging will be slightly off. I considered possible mitigations to this, but all I could think of were rather invasive just to fix this small edge case. Let me know if you think it's very important.

I took a look at the payment_store.update function, and I think it would make sense to return an error with a message like "payment hash not found" in an else block when the payment is not found in the store. This way, returning false would only indicate that no new update was made that is the payment exists but no changes were applied, while a missing payment would result in a clear error.

Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?

Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?
I think this is correct: previously, calling it twice would succeed and have us call fail_htlc_backwards twice if the payment was known at all. Now, we'd only have it succeed once, and would error out on the second attempt and/or if the payment had previously failed already. Added some clarifications to the docs.

I am not sure if this is optimal API design. I've seen that callers oftentimes also need to implement idempotency, and that is difficult now?

As discussed offline, now added a ternary enum that represents all update results.

(now also fixed CI)

vincenzopalazzo

LGMT, but not sure how the update info need to be return Ok(false) and not just a specific error?

Looks like to me that this will make the state machine a little bit more complex?

joostjager · 2025-04-17T08:50:30Z

- );
- debug_assert!(false);
- },
+ Ok(_) => (),


Perhaps document why we can ignore this result and mention the replay possbility?

Okay, if you prefer, although the comment is a bit redundant tbh. Added a fixup.

I actually meant to suggest to explain why this function is called multiple times and that that is the reason for ignoring the result value.

The main reason for ignoring it is that not doing so is a bug stemming from the slight shift in our internal API semantics. Seems weird to document something that changed by now?

Someone might still wonder why the return value is ignored, and to understand this they need to know that the method can be called multiple times. Perhaps it can work too as a comment on the method itself.
It is a question that I had while reviewing, so others might too.

Alright, now added a fixup that expands the comment and also discerns between the new PaymentStoreUpdateResult variants.

joostjager · 2025-04-17T08:56:26Z

- return Err(Error::InvalidPaymentHash);
+ match self.payment_store.update(&update) {
+ Ok(true) => (),
+ Ok(false) => {


Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?

tnull · 2025-04-17T10:19:29Z

CI failure is unrelated and should be fixed by #524

…imed`s In cd8958f we changed the internal API behavior of `PaymentStore::update`. While previously it would return `Ok(true)` the to-be-updated entry was found in the store, it now returns `Ok(true)` if not only the entry was found but it was actually updated and re-persisted. This was an improvement as it allows us to avoid unnecessary re-persists if nothing changed. However, there were 1-2 places that implicitly relied on that behavior for logging purposes which we didn't correctly update to the new behavior. Unfortunately, one instance in handling `PaymentClaimed` events actually even `debug_assert`ed on the return value, which lead to some unnecessary panics in `debug` in case `PaymentClaimed` got replayed. Here we rectify this by dropping the `debug_assert`.

tnull · 2025-04-18T09:40:29Z

Squashed fixups without further changes.

tnull · 2025-03-27T12:35:00Z

Fixes#506

In cd8958f we changed the internal API behavior of PaymentStore::update. While previously it would return Ok(true) the to-be-updated entry was found in the store, it now returns Ok(true) if not only the entry was found but it was actually updated and re-persisted. This was an improvement as it allows us to avoid unnecessary re-persists if nothing changed.

However, there were 1-2 places that implicitly relied on that behavior for logging purposes which we didn't correctly update to the new behavior. Unfortunately, one instance in handling PaymentClaimed events actually even debug_asserted on the return value, which lead to some unnecessary panics in debug in case PaymentClaimed got replayed. Here we rectify this by dropping the debug_assert.

G8XSU · 2025-03-27T20:03:52Z

- return Err(Error::InvalidPaymentHash);
+ match self.payment_store.update(&update) {
+ Ok(true) => (),
+ Ok(false) => {


slightly confused: how did we determine that it wasn't a hash for which fail_for_hash has already been called hence nothing changed?
iiuc, we will get Ok(false) in that case.

Yes, that's indeed the case, in which case the logging will be slightly off. I considered possible mitigations to this, but all I could think of were rather invasive just to fix this small edge case. Let me know if you think it's very important.

I took a look at the payment_store.update function, and I think it would make sense to return an error with a message like "payment hash not found" in an else block when the payment is not found in the store. This way, returning false would only indicate that no new update was made that is the payment exists but no changes were applied, while a missing payment would result in a clear error.

Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?

Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?
I think this is correct: previously, calling it twice would succeed and have us call fail_htlc_backwards twice if the payment was known at all. Now, we'd only have it succeed once, and would error out on the second attempt and/or if the payment had previously failed already. Added some clarifications to the docs.

I am not sure if this is optimal API design. I've seen that callers oftentimes also need to implement idempotency, and that is difficult now?

As discussed offline, now added a ternary enum that represents all update results.

(now also fixed CI)

vincenzopalazzo

LGMT, but not sure how the update info need to be return Ok(false) and not just a specific error?

Looks like to me that this will make the state machine a little bit more complex?

joostjager · 2025-04-17T08:50:30Z

- );
- debug_assert!(false);
- },
+ Ok(_) => (),


Perhaps document why we can ignore this result and mention the replay possbility?

Okay, if you prefer, although the comment is a bit redundant tbh. Added a fixup.

I actually meant to suggest to explain why this function is called multiple times and that that is the reason for ignoring the result value.

The main reason for ignoring it is that not doing so is a bug stemming from the slight shift in our internal API semantics. Seems weird to document something that changed by now?

Someone might still wonder why the return value is ignored, and to understand this they need to know that the method can be called multiple times. Perhaps it can work too as a comment on the method itself.
It is a question that I had while reviewing, so others might too.

Alright, now added a fixup that expands the comment and also discerns between the new PaymentStoreUpdateResult variants.

joostjager · 2025-04-17T08:56:26Z

- return Err(Error::InvalidPaymentHash);
+ match self.payment_store.update(&update) {
+ Ok(true) => (),
+ Ok(false) => {


Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?

tnull · 2025-04-17T10:19:29Z

CI failure is unrelated and should be fixed by #524

…imed`s In cd8958f we changed the internal API behavior of `PaymentStore::update`. While previously it would return `Ok(true)` the to-be-updated entry was found in the store, it now returns `Ok(true)` if not only the entry was found but it was actually updated and re-persisted. This was an improvement as it allows us to avoid unnecessary re-persists if nothing changed. However, there were 1-2 places that implicitly relied on that behavior for logging purposes which we didn't correctly update to the new behavior. Unfortunately, one instance in handling `PaymentClaimed` events actually even `debug_assert`ed on the return value, which lead to some unnecessary panics in `debug` in case `PaymentClaimed` got replayed. Here we rectify this by dropping the `debug_assert`.

tnull · 2025-04-18T09:40:29Z

Squashed fixups without further changes.

tnull · 2025-03-27T12:35:00Z

Fixes#506

In cd8958f we changed the internal API behavior of PaymentStore::update. While previously it would return Ok(true) the to-be-updated entry was found in the store, it now returns Ok(true) if not only the entry was found but it was actually updated and re-persisted. This was an improvement as it allows us to avoid unnecessary re-persists if nothing changed.

However, there were 1-2 places that implicitly relied on that behavior for logging purposes which we didn't correctly update to the new behavior. Unfortunately, one instance in handling PaymentClaimed events actually even debug_asserted on the return value, which lead to some unnecessary panics in debug in case PaymentClaimed got replayed. Here we rectify this by dropping the debug_assert.

G8XSU · 2025-03-27T20:03:52Z

- return Err(Error::InvalidPaymentHash);
+ match self.payment_store.update(&update) {
+ Ok(true) => (),
+ Ok(false) => {


slightly confused: how did we determine that it wasn't a hash for which fail_for_hash has already been called hence nothing changed?
iiuc, we will get Ok(false) in that case.

Yes, that's indeed the case, in which case the logging will be slightly off. I considered possible mitigations to this, but all I could think of were rather invasive just to fix this small edge case. Let me know if you think it's very important.

I took a look at the payment_store.update function, and I think it would make sense to return an error with a message like "payment hash not found" in an else block when the payment is not found in the store. This way, returning false would only indicate that no new update was made that is the payment exists but no changes were applied, while a missing payment would result in a clear error.

Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?

Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?
I think this is correct: previously, calling it twice would succeed and have us call fail_htlc_backwards twice if the payment was known at all. Now, we'd only have it succeed once, and would error out on the second attempt and/or if the payment had previously failed already. Added some clarifications to the docs.

I am not sure if this is optimal API design. I've seen that callers oftentimes also need to implement idempotency, and that is difficult now?

As discussed offline, now added a ternary enum that represents all update results.

(now also fixed CI)

vincenzopalazzo

LGMT, but not sure how the update info need to be return Ok(false) and not just a specific error?

Looks like to me that this will make the state machine a little bit more complex?

joostjager · 2025-04-17T08:50:30Z

- );
- debug_assert!(false);
- },
+ Ok(_) => (),


Perhaps document why we can ignore this result and mention the replay possbility?

Okay, if you prefer, although the comment is a bit redundant tbh. Added a fixup.

I actually meant to suggest to explain why this function is called multiple times and that that is the reason for ignoring the result value.

The main reason for ignoring it is that not doing so is a bug stemming from the slight shift in our internal API semantics. Seems weird to document something that changed by now?

Someone might still wonder why the return value is ignored, and to understand this they need to know that the method can be called multiple times. Perhaps it can work too as a comment on the method itself.
It is a question that I had while reviewing, so others might too.

Alright, now added a fixup that expands the comment and also discerns between the new PaymentStoreUpdateResult variants.

joostjager · 2025-04-17T08:56:26Z

- return Err(Error::InvalidPaymentHash);
+ match self.payment_store.update(&update) {
+ Ok(true) => (),
+ Ok(false) => {


Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?

tnull · 2025-04-17T10:19:29Z

CI failure is unrelated and should be fixed by #524

…imed`s In cd8958f we changed the internal API behavior of `PaymentStore::update`. While previously it would return `Ok(true)` the to-be-updated entry was found in the store, it now returns `Ok(true)` if not only the entry was found but it was actually updated and re-persisted. This was an improvement as it allows us to avoid unnecessary re-persists if nothing changed. However, there were 1-2 places that implicitly relied on that behavior for logging purposes which we didn't correctly update to the new behavior. Unfortunately, one instance in handling `PaymentClaimed` events actually even `debug_assert`ed on the return value, which lead to some unnecessary panics in `debug` in case `PaymentClaimed` got replayed. Here we rectify this by dropping the `debug_assert`.

tnull · 2025-04-18T09:40:29Z

Squashed fixups without further changes.

tnull · 2025-03-27T12:35:00Z

Fixes#506

In cd8958f we changed the internal API behavior of PaymentStore::update. While previously it would return Ok(true) the to-be-updated entry was found in the store, it now returns Ok(true) if not only the entry was found but it was actually updated and re-persisted. This was an improvement as it allows us to avoid unnecessary re-persists if nothing changed.

However, there were 1-2 places that implicitly relied on that behavior for logging purposes which we didn't correctly update to the new behavior. Unfortunately, one instance in handling PaymentClaimed events actually even debug_asserted on the return value, which lead to some unnecessary panics in debug in case PaymentClaimed got replayed. Here we rectify this by dropping the debug_assert.

G8XSU · 2025-03-27T20:03:52Z

- return Err(Error::InvalidPaymentHash);
+ match self.payment_store.update(&update) {
+ Ok(true) => (),
+ Ok(false) => {


slightly confused: how did we determine that it wasn't a hash for which fail_for_hash has already been called hence nothing changed?
iiuc, we will get Ok(false) in that case.

Yes, that's indeed the case, in which case the logging will be slightly off. I considered possible mitigations to this, but all I could think of were rather invasive just to fix this small edge case. Let me know if you think it's very important.

I took a look at the payment_store.update function, and I think it would make sense to return an error with a message like "payment hash not found" in an else block when the payment is not found in the store. This way, returning false would only indicate that no new update was made that is the payment exists but no changes were applied, while a missing payment would result in a clear error.

Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?

Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?
I think this is correct: previously, calling it twice would succeed and have us call fail_htlc_backwards twice if the payment was known at all. Now, we'd only have it succeed once, and would error out on the second attempt and/or if the payment had previously failed already. Added some clarifications to the docs.

I am not sure if this is optimal API design. I've seen that callers oftentimes also need to implement idempotency, and that is difficult now?

As discussed offline, now added a ternary enum that represents all update results.

(now also fixed CI)

vincenzopalazzo

LGMT, but not sure how the update info need to be return Ok(false) and not just a specific error?

Looks like to me that this will make the state machine a little bit more complex?

joostjager · 2025-04-17T08:50:30Z

- );
- debug_assert!(false);
- },
+ Ok(_) => (),


Perhaps document why we can ignore this result and mention the replay possbility?

Okay, if you prefer, although the comment is a bit redundant tbh. Added a fixup.

I actually meant to suggest to explain why this function is called multiple times and that that is the reason for ignoring the result value.

The main reason for ignoring it is that not doing so is a bug stemming from the slight shift in our internal API semantics. Seems weird to document something that changed by now?

Someone might still wonder why the return value is ignored, and to understand this they need to know that the method can be called multiple times. Perhaps it can work too as a comment on the method itself.
It is a question that I had while reviewing, so others might too.

Alright, now added a fixup that expands the comment and also discerns between the new PaymentStoreUpdateResult variants.

joostjager · 2025-04-17T08:56:26Z

- return Err(Error::InvalidPaymentHash);
+ match self.payment_store.update(&update) {
+ Ok(true) => (),
+ Ok(false) => {


Is it just a logging edge case, or also that the user gets an InvalidPaymentHash error when calling this function twice indeed?

tnull · 2025-04-17T10:19:29Z

CI failure is unrelated and should be fixed by #524

…imed`s In cd8958f we changed the internal API behavior of `PaymentStore::update`. While previously it would return `Ok(true)` the to-be-updated entry was found in the store, it now returns `Ok(true)` if not only the entry was found but it was actually updated and re-persisted. This was an improvement as it allows us to avoid unnecessary re-persists if nothing changed. However, there were 1-2 places that implicitly relied on that behavior for logging purposes which we didn't correctly update to the new behavior. Unfortunately, one instance in handling `PaymentClaimed` events actually even `debug_assert`ed on the return value, which lead to some unnecessary panics in `debug` in case `PaymentClaimed` got replayed. Here we rectify this by dropping the `debug_assert`.

tnull · 2025-04-18T09:40:29Z

Squashed fixups without further changes.

ldk-reviews-bot requested a review from joostjager March 27, 2025 19:00

G8XSU reviewed Mar 27, 2025
View reviewed changes

tnull removed the request for review from joostjager March 28, 2025 08:57

tnull added this to the 0.5 milestone Mar 28, 2025

tnull requested a review from G8XSU March 31, 2025 13:26

vincenzopalazzo approved these changes Apr 5, 2025
View reviewed changes

tnull requested a review from joostjager April 17, 2025 08:18

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from def24fd to 6af64b6Compare April 17, 2025 08:19

joostjager reviewed Apr 17, 2025
View reviewed changes

tnull requested a review from joostjager April 18, 2025 09:20

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from ebccab7 to 3711ed1Compare April 18, 2025 09:25

joostjager reviewed Apr 18, 2025
View reviewed changes

Comment threadsrc/payment/store.rs Outdated
Comment threadsrc/payment/bolt11.rs Outdated

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from 58ba5bf to 8195d2eCompare April 18, 2025 09:37

joostjager approved these changes Apr 18, 2025
View reviewed changes

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from 8195d2e to 4c2e5f3Compare April 18, 2025 09:40

tnull merged commit 273106d into lightningdevkit:mainApr 18, 2025

ldk-reviews-bot requested a review from joostjager March 27, 2025 19:00

G8XSU reviewed Mar 27, 2025
View reviewed changes

tnull removed the request for review from joostjager March 28, 2025 08:57

tnull added this to the 0.5 milestone Mar 28, 2025

tnull requested a review from G8XSU March 31, 2025 13:26

vincenzopalazzo approved these changes Apr 5, 2025
View reviewed changes

tnull requested a review from joostjager April 17, 2025 08:18

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from def24fd to 6af64b6Compare April 17, 2025 08:19

joostjager reviewed Apr 17, 2025
View reviewed changes

tnull requested a review from joostjager April 18, 2025 09:20

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from ebccab7 to 3711ed1Compare April 18, 2025 09:25

joostjager reviewed Apr 18, 2025
View reviewed changes

Comment threadsrc/payment/store.rs Outdated
Comment threadsrc/payment/bolt11.rs Outdated

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from 58ba5bf to 8195d2eCompare April 18, 2025 09:37

joostjager approved these changes Apr 18, 2025
View reviewed changes

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from 8195d2e to 4c2e5f3Compare April 18, 2025 09:40

tnull merged commit 273106d into lightningdevkit:mainApr 18, 2025

ldk-reviews-bot requested a review from joostjager March 27, 2025 19:00

G8XSU reviewed Mar 27, 2025
View reviewed changes

tnull removed the request for review from joostjager March 28, 2025 08:57

tnull added this to the 0.5 milestone Mar 28, 2025

tnull requested a review from G8XSU March 31, 2025 13:26

vincenzopalazzo approved these changes Apr 5, 2025
View reviewed changes

tnull requested a review from joostjager April 17, 2025 08:18

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from def24fd to 6af64b6Compare April 17, 2025 08:19

joostjager reviewed Apr 17, 2025
View reviewed changes

tnull requested a review from joostjager April 18, 2025 09:20

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from ebccab7 to 3711ed1Compare April 18, 2025 09:25

joostjager reviewed Apr 18, 2025
View reviewed changes

Comment threadsrc/payment/store.rs Outdated
Comment threadsrc/payment/bolt11.rs Outdated

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from 58ba5bf to 8195d2eCompare April 18, 2025 09:37

joostjager approved these changes Apr 18, 2025
View reviewed changes

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from 8195d2e to 4c2e5f3Compare April 18, 2025 09:40

tnull merged commit 273106d into lightningdevkit:mainApr 18, 2025

ldk-reviews-bot requested a review from joostjager March 27, 2025 19:00

G8XSU reviewed Mar 27, 2025
View reviewed changes

tnull removed the request for review from joostjager March 28, 2025 08:57

tnull added this to the 0.5 milestone Mar 28, 2025

tnull requested a review from G8XSU March 31, 2025 13:26

vincenzopalazzo approved these changes Apr 5, 2025
View reviewed changes

tnull requested a review from joostjager April 17, 2025 08:18

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from def24fd to 6af64b6Compare April 17, 2025 08:19

joostjager reviewed Apr 17, 2025
View reviewed changes

tnull requested a review from joostjager April 18, 2025 09:20

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from ebccab7 to 3711ed1Compare April 18, 2025 09:25

joostjager reviewed Apr 18, 2025
View reviewed changes

Comment threadsrc/payment/store.rs Outdated
Comment threadsrc/payment/bolt11.rs Outdated

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from 58ba5bf to 8195d2eCompare April 18, 2025 09:37

joostjager approved these changes Apr 18, 2025
View reviewed changes

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from 8195d2e to 4c2e5f3Compare April 18, 2025 09:40

tnull merged commit 273106d into lightningdevkit:mainApr 18, 2025

ldk-reviews-bot requested a review from joostjager March 27, 2025 19:00

G8XSU reviewed Mar 27, 2025
View reviewed changes

tnull removed the request for review from joostjager March 28, 2025 08:57

tnull added this to the 0.5 milestone Mar 28, 2025

tnull requested a review from G8XSU March 31, 2025 13:26

vincenzopalazzo approved these changes Apr 5, 2025
View reviewed changes

tnull requested a review from joostjager April 17, 2025 08:18

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from def24fd to 6af64b6Compare April 17, 2025 08:19

joostjager reviewed Apr 17, 2025
View reviewed changes

tnull requested a review from joostjager April 18, 2025 09:20

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from ebccab7 to 3711ed1Compare April 18, 2025 09:25

joostjager reviewed Apr 18, 2025
View reviewed changes

Comment threadsrc/payment/store.rs Outdated
Comment threadsrc/payment/bolt11.rs Outdated

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from 58ba5bf to 8195d2eCompare April 18, 2025 09:37

joostjager approved these changes Apr 18, 2025
View reviewed changes

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from 8195d2e to 4c2e5f3Compare April 18, 2025 09:40

tnull merged commit 273106d into lightningdevkit:mainApr 18, 2025

ldk-reviews-bot requested a review from joostjager March 27, 2025 19:00

G8XSU reviewed Mar 27, 2025
View reviewed changes

tnull removed the request for review from joostjager March 28, 2025 08:57

tnull added this to the 0.5 milestone Mar 28, 2025

tnull requested a review from G8XSU March 31, 2025 13:26

vincenzopalazzo approved these changes Apr 5, 2025
View reviewed changes

tnull requested a review from joostjager April 17, 2025 08:18

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from def24fd to 6af64b6Compare April 17, 2025 08:19

joostjager reviewed Apr 17, 2025
View reviewed changes

tnull requested a review from joostjager April 18, 2025 09:20

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from ebccab7 to 3711ed1Compare April 18, 2025 09:25

joostjager reviewed Apr 18, 2025
View reviewed changes

Comment threadsrc/payment/store.rs Outdated
Comment threadsrc/payment/bolt11.rs Outdated

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from 58ba5bf to 8195d2eCompare April 18, 2025 09:37

joostjager approved these changes Apr 18, 2025
View reviewed changes

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from 8195d2e to 4c2e5f3Compare April 18, 2025 09:40

tnull merged commit 273106d into lightningdevkit:mainApr 18, 2025

ldk-reviews-bot requested a review from joostjager March 27, 2025 19:00

G8XSU reviewed Mar 27, 2025
View reviewed changes

tnull removed the request for review from joostjager March 28, 2025 08:57

tnull added this to the 0.5 milestone Mar 28, 2025

tnull requested a review from G8XSU March 31, 2025 13:26

vincenzopalazzo approved these changes Apr 5, 2025
View reviewed changes

tnull requested a review from joostjager April 17, 2025 08:18

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from def24fd to 6af64b6Compare April 17, 2025 08:19

joostjager reviewed Apr 17, 2025
View reviewed changes

tnull requested a review from joostjager April 18, 2025 09:20

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from ebccab7 to 3711ed1Compare April 18, 2025 09:25

joostjager reviewed Apr 18, 2025
View reviewed changes

Comment threadsrc/payment/store.rs Outdated
Comment threadsrc/payment/bolt11.rs Outdated

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from 58ba5bf to 8195d2eCompare April 18, 2025 09:37

joostjager approved these changes Apr 18, 2025
View reviewed changes

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from 8195d2e to 4c2e5f3Compare April 18, 2025 09:40

tnull merged commit 273106d into lightningdevkit:mainApr 18, 2025

ldk-reviews-bot requested a review from joostjager March 27, 2025 19:00

G8XSU reviewed Mar 27, 2025
View reviewed changes

tnull removed the request for review from joostjager March 28, 2025 08:57

tnull added this to the 0.5 milestone Mar 28, 2025

tnull requested a review from G8XSU March 31, 2025 13:26

vincenzopalazzo approved these changes Apr 5, 2025
View reviewed changes

tnull requested a review from joostjager April 17, 2025 08:18

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from def24fd to 6af64b6Compare April 17, 2025 08:19

joostjager reviewed Apr 17, 2025
View reviewed changes

tnull requested a review from joostjager April 18, 2025 09:20

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from ebccab7 to 3711ed1Compare April 18, 2025 09:25

joostjager reviewed Apr 18, 2025
View reviewed changes

Comment threadsrc/payment/store.rs Outdated
Comment threadsrc/payment/bolt11.rs Outdated

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from 58ba5bf to 8195d2eCompare April 18, 2025 09:37

joostjager approved these changes Apr 18, 2025
View reviewed changes

tnull force-pushed the 2025-03-fix-paymentdetails-panic-debug-assert branch from 8195d2e to 4c2e5f3Compare April 18, 2025 09:40

tnull merged commit 273106d into lightningdevkit:mainApr 18, 2025

Uh oh!

Conversation

tnull commented Mar 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjagerApr 17, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joostjagerApr 18, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull commented Apr 17, 2025

Uh oh!

Uh oh!

Uh oh!

tnull commented Apr 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

tnull commented Mar 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

joostjagerApr 17, 2025•
edited
Loading

joostjagerApr 18, 2025•
edited
Loading

joostjagerApr 17, 2025•
edited
Loading

joostjagerApr 18, 2025•
edited
Loading