-
Notifications
You must be signed in to change notification settings - Fork 266
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
Use length-delimited, byte-aligned codecs #1442
Conversation
eclair-core/src/main/scala/fr/acinq/eclair/wire/ChannelCodecs.scala
Outdated
Show resolved
Hide resolved
("commitInput" | inputInfoCodec) :: | ||
("remotePerCommitmentSecrets" | ShaChain.shaChainCodec) :: | ||
("remotePerCommitmentSecrets" | byteAligned(ShaChain.shaChainCodec)) :: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shaChainCodec
isn't byte aligned, and there may be room for optimization there. cc @sstone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should make it byte-aligned, but the question is whether to do it "outside" (what you just did), or inside the shachain codec (use bool8
instead of bool
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently we can be more optimal, the codec should naturally be byte-aligned. Since this is sensitive code I'd rather leave that optimisation to another PR.
.typecase(0x02, relayedCodec) | ||
.typecase(0x03, localCodec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the compatibility codec since the new codec will never use it.
My plan for tests would be to generate a bunch of channel data before this patch (maybe with scalacheck generators @t-bast?) and their java object checksum. I would then apply this change, read-then-write-then-read-again the data, and make sure the three checksums are equal. I think that would bring solid assurance that we don't have a regression. |
So I validated the change by following the procedure decribed in my previous comment. I'm using an sqlite table with two columns: scodec-serialized and java-serialized, generated on a pre-migration branch. Then on the post-migration branch, I make sure that I ended up using three data sets:
test("nonreg length-delimited reference data generation") {
val sqlite = DriverManager.getConnection(s"jdbc:sqlite:${new File("endurance_before.sqlite")}")
sqlite.createStatement().executeUpdate("CREATE TABLE data (scodec BLOB NOT NULL, java BLOB NOT NULL)")
val read_sqlite = DriverManager.getConnection("jdbc:sqlite::resource:eclair.sqlite.endurance.bak")
val rs = read_sqlite.createStatement().executeQuery("SELECT data FROM local_channels")
var count = 0
while (rs.next()) {
val scodec = BitVector(rs.getBytes("data"))
val d = ChannelCodecs.stateDataCodec.decode(scodec).require.value
val bos = new ByteArrayOutputStream()
val oos = new ObjectOutputStream(bos)
oos.writeObject(d)
oos.close()
val java = bos.toByteArray
val statement = sqlite.prepareStatement("INSERT INTO data VALUES (?, ?)")
statement.setBytes(1, scodec.toByteArray)
statement.setBytes(2, java)
statement.execute()
count = count + 1
}
println(s"processed $count channels")
} This is the actual test: test("nonreg length-delimited migration") {
val sqlite = DriverManager.getConnection(s"jdbc:sqlite:${new File("endurance_before.sqlite")}")
val rs = sqlite.createStatement().executeQuery("SELECT * FROM data")
var count = 0
while (rs.next()) {
val scodec = ByteVector(rs.getBytes(1)).bits
val java = rs.getBytes(2)
val d_scodec = ChannelCodecs.stateDataCodec.decode(scodec).require.value match {
case d: DATA_WAIT_FOR_FUNDING_CONFIRMED => d.copy(waitingSince = 42000)
case d: DATA_CLOSING => d.copy(waitingSince = 123456789)
case d => d
}
val bis = new ByteArrayInputStream(java)
val ois = new ObjectInputStream(bis)
val d_java = ois.readObject().asInstanceOf[HasCommitments] match {
case d: DATA_WAIT_FOR_FUNDING_CONFIRMED => d.copy(waitingSince = 42000)
case d: DATA_CLOSING => d.copy(waitingSince = 123456789)
case d => d
}
assert(d_scodec === d_java)
val scodec2 = ChannelCodecs.stateDataCodec.encode(d_scodec).require
assert(scodec != scodec2)
assert(scodec2.size % 8 == 0)
val d_scodec2 = ChannelCodecs.stateDataCodec.decode(scodec2).require.value
assert(d_scodec2 == d_java)
count = count + 1
}
println(s"processed $count channels")
} It makes little sense committing the migration tests since they occur on two different versions of the code and they only apply at the time of the migration. I prefer documenting them here. |
// We used to ignore unknown trailing fields, and assume that channel_update size was known. This is not true anymore, | ||
// so we need to tell the codec where to stop, otherwise all the remaining part of the data will be decoded as unknown | ||
// fields. Fortunately, we can easily tell what size the channel_update will be. | ||
val noUnknownFieldsChannelUpdateSizeCodec: Codec[Int] = peek( // we need to take a peek at a specific byte to know what size the message will be, and then rollback to read the full message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got rid of this compatibility code.
@@ -184,18 +182,14 @@ object ChannelCodecs extends Logging { | |||
("amountIn" | millisatoshi) :: | |||
("amountOut" | millisatoshi)).as[Origin.Relayed] | |||
|
|||
// this is for backward compatibility to handle legacy payments that didn't have identifiers | |||
val UNKNOWN_UUID = UUID.fromString("00000000-0000-0000-0000-000000000000") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got rid of this compatibility code.
At least it doesn't hurt to keep them. The way I see it the goal is to completely remove the code of these legacy codecs (for example, X releases after the one where we release the new codecs). We can remove these tests at that point? If that's the goal, maybe we can put a comment in the code to indicate that strategy. |
I think the testing strategy is sound, and at a high-level the code looks good. Could you squash/rebase and then we can review thoroughly? |
There is a price though: we have several accessible codecs for the same object types, which is a bit error-prone. But we don't use those codecs that often and the risk of using a legacy one by mistake is quite low. |
6a31dcd
to
06207d1
Compare
Done. I need to re-run the tests. It just occured to me that #1141 had touched the codecs, so a bit more testing is probably not a bad thing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's surprisingly less painful to review than I had anticipated :)
I hope I didn't miss anything, the migration tests look solid.
eclair-core/src/test/scala/fr/acinq/eclair/wire/ChannelCodecsSpec.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/wire/ChannelCodecs.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/wire/ChannelCodecsSpec.scala
Outdated
Show resolved
Hide resolved
6e3414b was less straightforward than I expected. Being in an inner object of the normal codecs, the legacy codecs have transparent access to all codecs which leads to hidden dependencies. I realize now that copy-pasting the old file would have been the safest approach. I started like that but reverted because it prevented me from sharing some codecs... which I now see wasn't a good idea. WDYT? I've regenerated the random data to support static remote key and successfully re-ran the migration tests. |
Tried a different isolation with d052666, and re-ran the migrations tests. I think that design is better. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once you've re-done the E2E tests!
eclair-core/src/main/scala/fr/acinq/eclair/wire/ChannelCodecs.scala
Outdated
Show resolved
Hide resolved
d052666
to
b90a9e0
Compare
Isolated legacy codec in separate file, and restricted visibility to "package" in order to reduce the risk of using those codecs. Also restricted the codecs to `decodeOnly` for the same reason.
b90a9e0
to
7a67349
Compare
All LN protocol message must be stored as length-delimited, because they may have arbitrary trailing data.
Took the opportunity to restore byte alignment. It was mainly due to the use of single-bit
bool
codec.The goal is to play safe and have simple codecs, so the existing codecs have been kept and moved to a
Legacy
inner object.This is an alternative to #1239.