-
Notifications
You must be signed in to change notification settings - Fork 364
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
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.
Makes sense
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clippy is dumb. But, also, we could just wait for #1575 (comment)
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I'll leave it open, thank you
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'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).
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 :) |
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.
Thanks, pretty neat now!
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
RecoverableSignature
does not implement Hash but I also opened a PR there so the 'by hand' implementation forInvoiceSignature
could be removed.