Separate tlv decoding from content validation #2414
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When decoding a tlv stream, we previously also validated the stream's content at decoding time. This was a layer violation, as checking that specific tlvs are present in a stream is not an encoding concern. This was somewhat fine when we only had very basic validation (presence or absence of specific tlvs), but blinded paths substantially changed that because one of the tlvs must be decrypted to yield another tlv stream that also needs to have its content validated. This forced us to have an overly complex trait hierarchy in PaymentOnion.scala and expose a blinding key in classes that shouldn't care about whether blinding is used or not. We now decouple that into two distinct steps: * codecs simply return tlv streams and verify that tlvs are correctly encoded * business logic case classes (such as ChannelRelayPayload) should be instantiated with a new `validate` method that takes tlv streams and verifies mandatory/forbidden tlvs This lets us greatly simplify the trait hierarchy and deal with case class that only contain fully decrypted and valid data.
There was redundancy in the wrong places: route blinding codec tests were testing route blinding decryption and were missing content validation. We also change the behavior of the route blinding decode method to return the blinding override when present, instead of letting higher level components duplicate that logic.
t-bast
force-pushed
the
separate-tlv-decode-validation
branch
from
98cac21
to
214b020
Compare
pm47
reviewed
Sep 9, 2022
eclair-core/src/main/scala/fr/acinq/eclair/wire/protocol/PaymentOnion.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentPacket.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentPacket.scala
Outdated
Show resolved
Hide resolved
thomash-acinq
approved these changes
Sep 12, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When decoding a tlv stream, we previously also validated the stream's content at decoding time. This was a layer violation, as checking that specific tlvs are present in a stream is not an encoding concern.
This was somewhat fine when we only had very basic validation (presence or absence of specific tlvs), but blinded paths substantially changed that because one of the tlvs must be decrypted to yield another tlv stream that also needs to have its content validated. Our validation thus became incomplete, since it didn't decrypt the
encrypted_recipient_datatlv to validate its own tlv contents.This forced us to have an overly complex trait hierarchy in
PaymentOnion.scalaand expose a blinding key in classes that shouldn't care about whether blinding is used or not.We now decouple that into two distinct steps:
ChannelRelayPayload) should be instantiated with a newvalidatemethod that takes tlv streams and verifies mandatory/forbidden tlvsThis lets us greatly simplify the trait hierarchy and deal with case class that only contain fully decrypted and valid data.
ℹ️ note to reviewers: I recommend starting with the changes in
PaymentOnion.scalaandPaymentPacket.scala, as they contain the gist of the change. The other changes are simply replicating this same strategy to other codecs and fixing the build and tests.