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.

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.

@tnull
Copy link
Contributor

tnull commented Oct 31, 2025

CI failure will be fixed by #44

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.

3 participants