Skip to content

fix: advertise the plain invoice_expiration_window - #914

Open
21Mill wants to merge 1 commit into
MostroP2P:mainfrom
21Mill:fix/info-invoice-expiration-window
Open

fix: advertise the plain invoice_expiration_window#914
21Mill wants to merge 1 commit into
MostroP2P:mainfrom
21Mill:fix/info-invoice-expiration-window

Conversation

@21Mill

@21Mill 21Mill commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Closes #895.

The bug

The kind-38385 info event filled the invoice_expiration_window tag from
ln_settings.hold_invoice_expiration_window — a copy-paste from the tag two
entries above, so both tags published the same number.

The two are independent settings: the hold invoice window is the lifetime of
the invoice the seller pays, while invoice_expiration_window is the minimum
remaining lifetime a buyer payout invoice must have, enforced in
src/lightning/invoice.rs. So the one number a client needs in order to build
an acceptable payout invoice was precisely the one the daemon misreported. A
client that trusted the info event got a cant-do back, and the order sat in
waiting-buyer-invoice until it was reaped.

The change

  • Read the field each tag names — the fix itself.
  • Extract the three ln_settings-derived tags into ln_policy_tags, mirroring
    the existing bond_policy_tags. The helper is spliced in at the original
    position, so the wire order of the info event's tags is unchanged.
  • Document what separates the two windows on the settings fields themselves
    (src/config/types.rs), where the doc comments only paraphrased the names.

Why the extraction

Without it the regression test would be vacuous. test_settings() builds
LightningSettings::default(), which leaves both windows at 0, and
MOSTRO_CONFIG is a process-wide OnceLock no test can mutate — so a test
driven through info_to_tags() passes just as happily with the fields swapped.
Testing the pure helper allows pinning each window against a distinct value.
This is the same reason bond_policy_tags was split out, and its doc comment
says so.

Verification

The regression test was confirmed to fail against the original bug before the
fix was applied:

---- nip33::tests::ln_policy_tags_reports_each_window_from_its_own_field stdout ----
assertion `left == right` failed: invoice_expiration_window must report the buyer payout window
  left: Some("120")
 right: Some("600")

With the fix, the full suite is green:

test result: ok. 1234 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out; finished in 16.44s

cargo clippy --all-targets --all-features -- -D warnings is clean and
cargo fmt has been applied. No schema or config changes: the tag name and the
settings keys are untouched, only the value the tag is filled from. Clients
pick up the corrected value on the next info event.

Summary by CodeRabbit

  • Bug Fixes

    • Corrected Lightning policy information so buyer invoice expiration windows are reported separately from seller-paid hold invoice lifetimes.
    • Ensured hold-invoice, hold-CLTV, and invoice-expiration settings are each represented accurately.
  • Documentation

    • Clarified the distinction between buyer payout invoice expiration and seller-paid hold invoice lifetime in Lightning settings documentation.
  • Tests

    • Added coverage to verify the two expiration values remain independent.

@coderabbitai

coderabbitai Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 63fe49aa-1de1-4844-8df0-d3bede211bbe

📥 Commits

Reviewing files that changed from the base of the PR and between 848ee3b and 7d246be.

📒 Files selected for processing (2)
  • src/config/types.rs
  • src/nip33.rs

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.


Walkthrough

Changes

The Lightning info tags now report invoice_expiration_window from its dedicated setting. A helper builds the policy tags, and a regression test verifies distinct invoice and hold-invoice values.

Lightning policy tag correction

Layer / File(s) Summary
Lightning policy tag construction
src/config/types.rs, src/nip33.rs
The documentation distinguishes buyer payout invoice expiration from seller-paid hold-invoice lifetime. info_to_tags delegates Lightning policy tag creation to ln_policy_tags, which uses the corresponding settings fields.
Distinct expiration regression coverage
src/nip33.rs
The test uses different invoice and hold-invoice windows and verifies both generated tags.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 7d246

This change corrects the advertised invoice expiration value while preserving tag names and ordering; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: grunch, arkanoider

Poem

A bunny checks each Lightning tag,
No copied window trails its lag.
Hold stays hold, invoices shine,
Two settings keep their values fine.
Tests hop neatly through the line.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the fix to advertise the correct invoice_expiration_window value.
Linked Issues check ✅ Passed The changes use the correct setting, preserve tag order, add distinct-value regression coverage, and clarify related documentation [#895].
Out of Scope Changes check ✅ Passed All changes support the linked issue by fixing tag construction, adding regression coverage, or clarifying related documentation.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

The kind-38385 info event filled the `invoice_expiration_window` tag from
`hold_invoice_expiration_window`, a copy-paste from the tag two entries
above. Those are independent settings, and the one a client needs to
build an acceptable payout invoice was the one misreported: a client that
trusted the info event got a cant-do back and the order sat in
`waiting-buyer-invoice` until reaped.

Extract the `ln_settings` tags into `ln_policy_tags`, mirroring the
existing `bond_policy_tags`, and read the field each tag names. The
extraction is what makes the regression test meaningful — the shared test
settings leave both windows at `0`, so a test driven by the global config
cannot tell the two fields apart. Wire order of the tags is unchanged.

Document what separates the two windows on the settings fields
themselves, where their doc comments only paraphrased their names.

Closes MostroP2P#895
@21Mill
21Mill force-pushed the fix/info-invoice-expiration-window branch from 7d246be to 00d58c0 Compare August 22, 2026 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Info event advertises hold_invoice_expiration_window under the invoice_expiration_window tag

1 participant