derive Hash for Invoice #1575
derive Hash for Invoice #1575
Conversation
lightning-invoice/src/lib.rs
Outdated
| state.write(&sig); | ||
| state.write_i32(recovery_id); |
There was a problem hiding this comment.
I'm not sure if it is ok to do it like this. In secp256k1 they already have a Hash impl for the contained type in ffi pub struct RecoverableSignature(ffi::RecoverableSignature); but they don't derive Hash on that. If ffi was public the correct way would be (&self.0.0[..]).hash(state) but it is not possible
There was a problem hiding this comment.
Should we just wait for rust-bitcoin/rust-secp256k1#462 then? Otherwise let's at least leave a TODO to note we should drop the manual form here, but this is correct.
Codecov Report
@@ Coverage Diff @@
## main #1575 +/- ##
==========================================
- Coverage 91.04% 91.00% -0.04%
==========================================
Files 80 80
Lines 44072 44072
Branches 44072 44072
==========================================
- Hits 40126 40109 -17
- Misses 3946 3963 +17
Continue to review full report at Codecov.
|
lightning-invoice/src/lib.rs
Outdated
| @@ -1415,6 +1415,15 @@ impl Deref for SignedRawInvoice { | |||
| } | |||
| } | |||
|
|
|||
| impl core::hash::Hash for InvoiceSignature { | |||
There was a problem hiding this comment.
Looks like clippy doesn't like this
error: you are implementing `Hash` explicitly but have derived `PartialEq`
--> lightning-invoice/src/lib.rs:1418:1
|
1418 | / impl core::hash::Hash for InvoiceSignature {
1419 | | fn hash<H: core::hash::Hasher>(&self, state: &mut H) {
1420 | | let (recovery_id, sig) = self.0.serialize_compact();
1421 | | let recovery_id = recovery_id.to_i32();
... |
1424 | | }
1425 | | }
| |_^
|
= note: `#[deny(clippy::derive_hash_xor_eq)]` on by default
note: `PartialEq` implemented here
--> lightning-invoice/src/lib.rs:456:28
|
456 | #[derive(Clone, Debug, Eq, PartialEq)]
| ^^^^^^^^^
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#derive_hash_xor_eq
= note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)
There was a problem hiding this comment.
Clippy is dumb. But, also, we could just wait for #1575 (comment)
There was a problem hiding this comment.
Yes it's no hurry so do I close the PR and open another if this is ready or should I leave it open ?
There was a problem hiding this comment.
Up to you, you're welcome to leave it open and I'll just add a "waiting on secp bump" tag
There was a problem hiding this comment.
Ok I'll leave it open, thank you
There was a problem hiding this comment.
I'd argue clippy isn't dumb here. The contract of Hash states that hashes have to be equal if Eq returns equal for the hashed objects. With this implementation we'd rely on libsecp256k1 to not use the higher bits of the recovery id byte for other things for that contract to hold. Related to rust-bitcoin/rust-secp256k1#463. The Hash impl from secp256k1 would be guaranteed to be at least internally consistent (just not between versions I guess).
TheBlueMatt
added
the
blocked on rust-bitcoin
|
I think it's ready now @TheBlueMatt |
Oh wait it's until ldk updates to the version, not only when it's released so nvm |
|
Indeed, now blocked on rust-bitcoin/rust-bitcoin#1066 |
|
@NicolaLS, the upstream PRs are in and I see rust-bitcoin 0.29 does include rust-bitcoin/rust-bitcoin#1066. So looks like we're unblocked completely now :) |
dunxen
removed
the
blocked on rust-bitcoin
Is it a problem that some CI failed ? |
Those are unrelated to the project. It’s a crates registry issue (I think). |
It would be nice to have Hash derived for Invoice. The problem was that
RecoverableSignaturedoes not implement Hash but I also opened a PR there so the 'by hand' implementation forInvoiceSignaturecould be removed.