Skip to content

Conversation

@tankyleo
Copy link
Contributor

 Use a 12-byte nonce as an input to Chacha20-Poly1305 Previously, we were using the Chacha20-Poly1305 implementation at `rust-lightning/lightning/src/crypto/chacha20poly1305rfc.rs`. That implementation required us to use an 8-byte nonce. Since we made the switch to the `rust-bitcoin/chacha20_poly1305` implementation, we can now use a full 12-byte nonce as specified in the RFC. 
@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Oct 30, 2025

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@tankyleo tankyleo requested review from tnull and removed request for joostjager October 30, 2025 21:51
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Thanks good catch!

&self, input: Vec<u8>, version: i64, data_encryption_key: &[u8; 32], aad: &[u8],
) -> Storable {
let mut nonce = [0u8; NONCE_LENGTH];
self.entropy_source.fill_bytes(&mut nonce[4..]);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems we have another instance of this in KeyObfuscator's generate_synthetic_nonce. But there I doubt we can easily change it without breaking backwards compatibility. Maybe we should at least leave a comment there for future context why we only copy 8 bytes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe more generally, should we add an upgrade test here? I.e., serialize a v0.3.1 Storable and make sure we can still deserialize it with our current code and aad = []?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe more generally, should we add an upgrade test here? I.e., serialize a v0.3.1 Storable and make sure we can still deserialize it with our current code and aad = []?

Now had Claude do it and opened #45.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems we have another instance of this in KeyObfuscator's generate_synthetic_nonce. But there I doubt we can easily change it without breaking backwards compatibility. Maybe we should at least leave a comment there for future context why we only copy 8 bytes?

Convinced myself this is the case, and added a TODO, let me know how that sounds. Thinking about how we would upgrade this thing without putting some hacky version byte somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the upgrade test ! will review today.

@tnull
Copy link
Contributor

tnull commented Oct 31, 2025

CI failure will be fixed by #44

@tankyleo tankyleo force-pushed the 25-10-use-12-byte-nonce branch from e196e3c to 13ab628 Compare November 3, 2025 14:41
Previously, we were using the Chacha20-Poly1305 implementation at `rust-lightning/lightning/src/crypto/chacha20poly1305rfc.rs`. That implementation required us to use an 8-byte nonce. Since we made the switch to the `rust-bitcoin/chacha20_poly1305` implementation, we can now use a full 12-byte nonce as specified in the RFC.
@tankyleo tankyleo force-pushed the 25-10-use-12-byte-nonce branch from 13ab628 to 4128ce8 Compare November 3, 2025 14:51
@tankyleo tankyleo requested a review from tnull November 3, 2025 14:54
@tankyleo
Copy link
Contributor Author

tankyleo commented Nov 3, 2025

ACK'ed the upgrade test, I let you merge the upgrade test when you are ready.

@tankyleo
Copy link
Contributor Author

tankyleo commented Nov 3, 2025

Cherry picked the two upgrade tests on top of this commit, both still pass, along with the full test suite.

@tnull tnull merged commit a126555 into lightningdevkit:main Nov 3, 2025
0 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants