Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Whisper: wrong proof-of-work calculated #9625

Closed
arnetheduck opened this issue Sep 21, 2018 · 1 comment
Closed

Whisper: wrong proof-of-work calculated #9625

arnetheduck opened this issue Sep 21, 2018 · 1 comment
Labels
F2-bug 🐞 The client fails to follow expected behavior.
Milestone

Comments

@arnetheduck
Copy link

arnetheduck commented Sep 21, 2018

2 suspected issues with proof-of-work calculation:

  • When calculating leading zeroes, one byte is skipped leading to a bogus leading_zeroes calc
    • consider a hash that has no leading zeroes / starts with 0xff..
    • the calculation will do hash.get(0 + 1).map_or... - off by one
  • the u64 is prone to overflow, for high proof-of-works (a float would handle this)

https://github.com/paritytech/parity-ethereum/blob/93e1040d07e385d1219d00af71c46c720b0a1acf/whisper/src/message.rs#L36

@Tbaut Tbaut added F2-bug 🐞 The client fails to follow expected behavior. Z0-unconfirmed 🤔 Issue might be valid, but it’s not yet known. labels Sep 24, 2018
@5chdn 5chdn added this to the 2.3 milestone Sep 27, 2018
@5chdn 5chdn modified the milestones: 2.3, 2.4 Oct 29, 2018
@5chdn 5chdn modified the milestones: 2.4, 2.5 Jan 10, 2019
@niklasad1
Copy link
Collaborator

niklasad1 commented Jan 10, 2019

Hey @arnetheduck

When calculating leading zeroes, one byte is skipped leading to a bogus leading_zeroes calc
- consider a hash that has no leading zeroes / starts with 0xff..
- the calculation will do hash.get(0 + 1).map_or... - off by one

Yes, you are right it should be just hash.get(leading_zeros) since index starts from zero and it will handle the case of 0xff too

the u64 is prone to overflow, for high proof-of-works (a float would handle this)

Yeah, I agree because the max number of zeros is 256 and any shift left bigger than 63 will end-up in an overflow but I think it will force us to use floating point operations thus no shifting.
We need to do something like 2.0_f64.powi(n as i32) instead (I don't think f64 supports ldexpf) unfortunately it is way slower

@niklasad1 niklasad1 removed the Z0-unconfirmed 🤔 Issue might be valid, but it’s not yet known. label Jan 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
F2-bug 🐞 The client fails to follow expected behavior.
Projects
None yet
Development

No branches or pull requests

4 participants