Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NodeId #601

Merged
merged 3 commits into from
Feb 15, 2024
Merged

NodeId #601

merged 3 commits into from
Feb 15, 2024

Conversation

thomash-acinq
Copy link
Member

Adds the NodeId interface that can be either a public key or a pair channel id, direction that's more compact. This is used as introduction node for blinded routes. We also use NodeId for the next node to relay a message to. Which is not in the spec but is necessary for us since we have no way to resolve the compact node id to a public key ourselves. It requires a compatible peer that accepts this format.

Adds the NodeId interface that can be either a public key or a pair channel id, direction that's more compact.
This is used as introduction node for blinded routes.
We also use NodeId for the next node to relay a message to. Which is not in the spec but is necessary for us since we have no way to resolve the compact node id to a public key ourselves. It requires a compatible peer that accepts this format.
src/commonMain/kotlin/fr/acinq/lightning/NodeId.kt Outdated Show resolved Hide resolved
@@ -22,14 +23,14 @@ sealed class RouteBlindingEncryptedDataTlv : Tlv {
}

/** Id of the next node. */
data class OutgoingNodeId(val nodeId: PublicKey) : RouteBlindingEncryptedDataTlv() {
data class OutgoingNodeId(val nodeId: NodeId) : RouteBlindingEncryptedDataTlv() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're modifying the official spec TLV here (tag = 4). We need to get agreement on this change at the spec level for that, otherwise we may have painful compatibility issues to handle in the future...I believe that changing the outgoing_node_id TLV to use this new sciddir_or_pubkey makes a lot of sense and should be in the BOLTs, don't you? This is an opportunity to create a small spec PR based on master that:

  • extracts the sciddir_or_pubkey from the offers spec PR
  • uses it in the outgoing_node_id TLV field of onion messages / blinded paths
  • adds or modifies a test vector to use this

I expect that this spec PR would be accepted somewhat quickly, and it would make the offers spec PR slightly smaller in scope. It's also important to create that spec PR before people actually start using offers, this way we can just change the TLV without caring about backwards-compat.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can merge that code to both lightning-kmp and eclair though, even before the spec PR gets accepted, because:

  • until we ship bolt 12 support in phoenix, we actually will never create such tlv fields
  • on the eclair side, we're just adding support for reading this new case, not writing it, so it won't impact other nodes and we can update the code without caring about backwards-compat

But we should still create a spec PR to get wide support for this feature!

val publicKey = PublicKey(ByteArray(1) { firstByte.toByte() } + bytes(input, 32))
return EncodedNodeId.Plain(publicKey)
} else {
throw IllegalArgumentException("unexpected first byte: $firstByte")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that you'll need to be careful when decrypting an onion message and decoding its content to catch that exception.

@thomash-acinq thomash-acinq requested a review from t-bast February 14, 2024 17:19
@thomash-acinq thomash-acinq merged commit 34e6834 into master Feb 15, 2024
2 checks passed
@thomash-acinq thomash-acinq deleted the nodeid branch February 15, 2024 09:15
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