Fix leading zeroes bug + add tests from geth and parity

eth/p2p/rlpx_protocols/whisper_protocol.nim

@@ -155,12 +155,12 @@ proc leadingZeroBits(hash: MDigest): int =
       result += countLeadingZeroBits(h)
       break
 
-proc calcPow(size, ttl: uint64, hash: Hash): float64 =
+proc calcPow*(size, ttl: uint64, hash: Hash): float64 =
   ## Whisper proof-of-work is defined as the best bit of a hash divided by
   ## encoded size and time-to-live, such that large and long-lived messages get
   ## penalized
 
-  let bits = leadingZeroBits(hash) + 1
+  let bits = leadingZeroBits(hash)
   return pow(2.0, bits.float64) / (size.float64 * ttl.float64)
 
 proc topicBloom*(topic: Topic): Bloom =
@@ -431,7 +431,7 @@ proc valid*(self: Envelope, now = epochTime()): bool =
 
 proc len(self: Envelope): int = 20 + self.data.len
 
-proc toShortRlp(self: Envelope): Bytes =
+proc toShortRlp*(self: Envelope): Bytes =
   ## RLP-encoded message without nonce is used during proof-of-work calculations
   rlp.encodeList(self.expiry, self.ttl, self.topic, self.data)
 
@@ -458,7 +458,7 @@ proc minePow*(self: Envelope, seconds: float, bestBitTarget: int = 0): (uint64,
     tmp.update(i.toBE())
     # XXX:a random nonce here would not leak number of iters
     let hash = tmp.finish()
-    let zeroBits = leadingZeroBits(hash) + 1
+    let zeroBits = leadingZeroBits(hash)
     if zeroBits > bestBit: # XXX: could also compare hashes as numbers instead
       bestBit = zeroBits
       result = (i, hash)
@@ -494,7 +494,7 @@ proc initMessage*(env: Envelope, powCalc = true): Message =
   if powCalc:
     result.hash = env.calcPowHash()
     result.pow = calcPow(result.env.len.uint32, result.env.ttl, result.hash)
-    trace "Message PoW", pow = result.pow
+    trace "Message PoW", pow = result.pow.formatFloat(ffScientific)
 
 proc hash*(msg: Message): hashes.Hash = hash(msg.hash.data)
 
@@ -515,16 +515,18 @@ proc allowed*(msg: Message, config: WhisperConfig): bool =
 
   return true
 
-# NOTE: PoW calculations are different from go-ethereum implementation,
-# which is not conform EIP-627.
-# See here: https://github.com/ethereum/go-ethereum/issues/18070
-# However, this implementation is also not conform EIP-627 as we do not use the
-# size of the RLP-encoded envelope, but the size of the envelope object itself.
+# NOTE: Hashing and leading zeroes calculation is now the same between geth,
+# parity and this implementation.
+# However, there is still a difference in the size calculation.
+# See also here: https://github.com/ethereum/go-ethereum/pull/19753
+# This implementation is not conform EIP-627 as we do not use the size of the
+# RLP-encoded envelope, but the size of the envelope object itself.
 # This is done to be able to correctly calculate the bestBitTarget.
 # Other options would be:
 # - work directly with powTarget in minePow, but this requires recalculation of
 #   rlp size + calcPow
 # - Use worst case size of envelope nonce
+# - Mine PoW for x interval, calcPow of best result, if target not met .. repeat
 proc sealEnvelope(msg: var Message, powTime: float, powTarget: float): bool =
   let size = msg.env.len
   if powTarget > 0:

tests/p2p/test_shh.nim

@@ -186,12 +186,44 @@ let
     data: repeat(byte 9, 256), nonce: 1010102)
 
 suite "Whisper envelope":
-  test "should use correct fields for pow hash":
-    # XXX checked with parity, should check with geth too - found a potential bug
-    #     in parity while playing with it:
-    #     https://github.com/paritytech/parity-ethereum/issues/9625
-    check $calcPowHash(env0) ==
-      "A13B48480AEB3123CD2358516E2E8EE9FCB0F4CB37E68CD09FDF7F9A7E14767C"
+
+  proc hashAndPow(env: Envelope): (string, float64) =
+    # This is the current implementation of go-ethereum
+    let size = env.toShortRlp().len().uint32
+    # This is our current implementation in `whisper_protocol.nim`
+    # let size = env.len().uint32
+    # This is the EIP-627 specification
+    # let size = env.toRlp().len().uint32
+    let hash = env.calcPowHash()
+    ($hash, calcPow(size, env.ttl, hash))
+
+  test "PoW calculation leading zeroes tests":
+    # Test values from Parity, in message.rs
+    let testHashes = [
+      # 256 leading zeroes
+      "0x0000000000000000000000000000000000000000000000000000000000000000",
+      # 255 leading zeroes
+      "0x0000000000000000000000000000000000000000000000000000000000000001",
+      # no leading zeroes
+      "0xff00000000000000000000000000000000000000000000000000000000000000"
+    ]
+    check:
+      calcPow(1, 1, Hash.fromHex(testHashes[0])) ==
+        115792089237316200000000000000000000000000000000000000000000000000000000000000.0
+      calcPow(1, 1, Hash.fromHex(testHashes[1])) ==
+        57896044618658100000000000000000000000000000000000000000000000000000000000000.0
+      calcPow(1, 1, Hash.fromHex(testHashes[2])) == 1.0
+
+    # Test values from go-ethereum whisperv6 in envelope_test
+    var env = Envelope(ttl: 1, data: @[byte 0xde, 0xad, 0xbe, 0xef])
+    # PoW calculation with no leading zeroes
+    env.nonce = 100000
+    check hashAndPoW(env) == ("A788E02A95BFC673709E97CA81E39CA903BAD5638D3388964C51EB64952172D6",
+                              0.07692307692307693)
+    # PoW calculation with 8 leading zeroes
+    env.nonce = 276
+    check hashAndPoW(env) == ("00E2374C6353C243E4073E209A7F2ACB2506522AF318B3B78CF9A88310A2A11C",
+                              19.692307692307693)
 
   test "should validate and allow envelope according to config":
     let ttl = 1'u32
@@ -349,15 +381,15 @@ suite "Whisper filter":
     # this message has a PoW of 0.02962962962962963, number should be updated
     # in case PoW algorithm changes or contents of padding, payload, topic, etc.
     # update: now with NON rlp encoded envelope size the PoW of this message is
-    # 0.02898550724637681
+    # 0.014492753623188406
     let msg = prepFilterTestMsg(topic = topic, padding = padding)
 
     var filters = initTable[string, Filter]()
     let
       filterId1 = filters.subscribeFilter(
-                    newFilter(topics = @[topic], powReq = 0.02898550724637681))
+                    newFilter(topics = @[topic], powReq = 0.014492753623188406))
       filterId2 = filters.subscribeFilter(
-                    newFilter(topics = @[topic], powReq = 0.02898550724637682))
+                    newFilter(topics = @[topic], powReq = 0.014492753623188407))
 
     notify(filters, msg)