Skip to content

bolt04: add explicit requirement for final nodes to reject short_channel_id#1303

Closed
erickcestari wants to merge 1 commit intolightning:masterfrom
erickcestari:must-not-include-shortchannelid-final-node
Closed

bolt04: add explicit requirement for final nodes to reject short_channel_id#1303
erickcestari wants to merge 1 commit intolightning:masterfrom
erickcestari:must-not-include-shortchannelid-final-node

Conversation

@erickcestari
Copy link
Copy Markdown
Contributor

@erickcestari erickcestari commented Nov 26, 2025

Final nodes should reject payments that include short_channel_id in their payload, as this field is only meaningful for forwarding nodes. While BOLT 4 specifies that writers MUST NOT include short_channel_id for the final node, it doesn't explicitly require final nodes to return an error when this field is present.

Current spec (writer requirements):

For the final node:
  - MUST NOT include `short_channel_id`

Current spec (reader requirements):

If it is the final node:
  - MUST treat `total_msat` as if it were equal to `amt_to_forward` if it is not present.
  - MUST return an error if:
    - incoming `amount_msat` < `amt_to_forward`.
    - incoming `cltv_expiry` < `outgoing_cltv_value`.
    - incoming `cltv_expiry` < `current_block_height` + `min_final_cltv_expiry_delta`

This ambiguity has led to implementation inconsistencies:

This inconsistency was discovered through differential fuzzing (bitcoinfuzz), where the same onion was rejected by LND and rust-lightning but accepted by Core Lightning.

This PR adds an explicit requirement for final nodes to return an error if short_channel_id is present in the payload, making the validation requirements consistent with the writer requirements.

…nel_id

Final nodes should reject payments where short_channel_id is present
in their payload, as this field is only meaningful for forwarding nodes.

This requirement was implicit but not explicitly documented in the
validation rules for final nodes.
@t-bast
Copy link
Copy Markdown
Collaborator

t-bast commented Dec 15, 2025

It does create undefined behavior, but I'm not sure it's much of an issue since this case should never happen with a BOLT-compliant writer and it doesn't hurt the reader?

As a general rule of thumb, we must be strict in what we write, but we can be lenient in what we read when it's harmless, which avoids unnecessarily listing every harmless case in the spec to keep it somewhat compact. We have a lot of similar cases for various messages, and it would add a lot of requirements to the BOLTs to "fix" them all, so I'm not really sure we should do it?

@erickcestari
Copy link
Copy Markdown
Contributor Author

Thanks for the review!

I'm not sure it's much of an issue since this case should never happen with a BOLT-compliant writer and it doesn't hurt the reader?

Yes.

As a general rule of thumb, we must be strict in what we write, but we can be lenient in what we read when it's harmless, which avoids unnecessarily listing every harmless case in the spec to keep it somewhat compact. We have a lot of similar cases for various messages, and it would add a lot of requirements to the BOLTs to "fix" them all, so I'm not really sure we should do it?

I'll close this PR. Although I would prefer a more detailed spec that explicitly covers all undefined behaviors, I understand this would add maintenance burden without much practical benefit.

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.

2 participants