Skip to content
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

Fix leading zeroes bug + add tests from geth and parity #73

Merged
merged 1 commit into from Jun 27, 2019

Conversation

@kdeme
Copy link
Contributor

commented Jun 26, 2019

Triggered by ethereum/go-ethereum#19330 and ethereum/go-ethereum#19753, I tested and noticed a difference in the best bits calculation of our implementation.

Not sure where the + 1 came from, but with this change that part of the PoW calculation should be the same for geth, parity and this implementation (and also in line with EIP-627).

The size calculation is still different though (and not in line with EIP-627), see comments in code & test.
We can decide here to follow spec or not, currently geth does not either (albeit in a different way than us, we have the old version of geth basically).

Should be noted also that the last changes in geth will have an impact in old versus new whisper nodes compatibility. More specifically, in possible many dropped whisper messages due to a too low PoW (as the hash will be different between old and new clients).

@stefantalpalaru

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

possible many dropped whisper messages due to a too low PoW

I'm seeing that already:

WRN 2019-06-26 14:42:02+02:00 Message PoW too low                        topics="whisper" tid=6754 file=../vendor/nim-eth/eth/p2p/rlpx_protocols/whisper_protocol.nim:509 minPow=0.2 pow=0.0003427592116538132
WRN 2019-06-26 14:42:02+02:00 Message PoW too low                        topics="whisper" tid=6754 file=../vendor/nim-eth/eth/p2p/rlpx_protocols/whisper_protocol.nim:509 minPow=0.2 pow=0.0006578947368421052
WRN 2019-06-26 14:42:02+02:00 Message PoW too low                        topics="whisper" tid=6754 file=../vendor/nim-eth/eth/p2p/rlpx_protocols/whisper_protocol.nim:509 minPow=0.2 pow=0.01052631578947368
WRN 2019-06-26 14:42:02+02:00 Message PoW too low                        topics="whisper" tid=6754 file=../vendor/nim-eth/eth/p2p/rlpx_protocols/whisper_protocol.nim:509 minPow=0.2 pow=0.0003427592116538132
WRN 2019-06-26 14:42:02+02:00 Message PoW too low                        topics="whisper" tid=6754 file=../vendor/nim-eth/eth/p2p/rlpx_protocols/whisper_protocol.nim:509 minPow=0.2 pow=0.0006578947368421052
WRN 2019-06-26 14:42:02+02:00 Message PoW too low                        topics="whisper" tid=6754 file=../vendor/nim-eth/eth/p2p/rlpx_protocols/whisper_protocol.nim:509 minPow=0.2 pow=0.01052631578947368
@kdeme

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

Yes, exactly, because our implementation already uses the same hashing algo as in the EIP-627 (and as Parity). So these messages are the proof of the incompatibility of geth whisper versus nimbus whisper.

Now, geth will get this same issue with old versus new nodes. This will also impact the Status client I'd guess.

@stefantalpalaru
Copy link
Contributor

left a comment

LGTM

@zah

zah approved these changes Jun 27, 2019

@kdeme kdeme merged commit 13a3281 into master Jun 27, 2019

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@delete-merged-branch delete-merged-branch bot deleted the bug/pow-calc-whisper branch Jun 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.