[bisq-network/bisq] Provide ASIC resistant PoW scheme for BSQ swaps (PR #5858)

[chimp1984]

@chimp1984 requested changes on this pull request.



> @@ -882,7 +882,7 @@ message ProofOfWork {
     bytes payload = 1;
     int64 counter = 2;
     bytes challenge = 3;
-    bytes difficulty = 4;
+    double difficulty = 4;

This cause an error popup for users running master. The `new BigInteger(difficulty)` at `getNumLeadingZeros` with a empty byte array fails.
Only happens with published BSQ offers (I was only one who had a BSQ offer and I just removed it). So we could ignore it.... Or we deprecate that existing fields and add a new one for the double type? But I guess its not worth to pollute the protofile with that short term tiny risk.... Would also only affect users running current master before that PR is merged.


> @@ -770,6 +770,7 @@ message Filter {
     bool disable_mempool_validation = 28;
     bool disable_pow_message = 29;
     int32 pow_difficulty = 30;
+    repeated int32 enabled_pow_versions = 35;

I understand that it makes sense to keep it contextually close to the other pow field but I think we follow the convention that we keep the fields ordered by the field index. So I think it would be better to add it at the end of the fields. But no strong opinion either.... 

> @@ -493,13 +494,23 @@ public boolean isProofOfWorkValid(Offer offer) {
         }
         checkArgument(offer.getBsqSwapOfferPayload().isPresent(),
                 "Offer payload must be BsqSwapOfferPayload");
-        return HashCashService.verify(offer.getBsqSwapOfferPayload().get().getProofOfWork(),
-                HashCashService.getBytes(offer.getId() + offer.getOwnerNodeAddress().toString()),
+        ProofOfWork pow = offer.getBsqSwapOfferPayload().get().getProofOfWork();
+        var service = ProofOfWorkService.forVersion(pow.getVersion());
+        if (!service.isPresent() || !getEnabledPowVersions().contains(pow.getVersion())) {
+            return false;
+        }
+        return service.get().verify(offer.getBsqSwapOfferPayload().get().getProofOfWork(),

We could use the `pow` variable from line 497....

> +        return CompletableFuture.supplyAsync(() -> {
+            long ts = System.currentTimeMillis();
+            byte[] solution = new Equihash(90, 5, difficulty).puzzle(challenge).findSolution().serialize();
+            long counter = Longs.fromByteArray(Arrays.copyOf(solution, 8));
+            var proofOfWork = new ProofOfWork(solution, counter, challenge, log2Difficulty,
+                    System.currentTimeMillis() - ts, getVersion());
+            log.info("Completed minting proofOfWork: {}", proofOfWork);
+            return proofOfWork;
+        });
+    }
+
+    @Override
+    public byte[] getChallenge(String itemId, String ownerId) {
+        checkArgument(!StringUtils.contains(itemId, '\0'));
+        checkArgument(!StringUtils.contains(ownerId, '\0'));
+        return Hash.getSha256Hash(checkNotNull(itemId) + "\0" + checkNotNull(ownerId));

Why are the 2 strings concatenated with a null char?

> +        });
+    }
+
+    @Override
+    public byte[] getChallenge(String itemId, String ownerId) {
+        checkArgument(!StringUtils.contains(itemId, '\0'));
+        checkArgument(!StringUtils.contains(ownerId, '\0'));
+        return Hash.getSha256Hash(checkNotNull(itemId) + "\0" + checkNotNull(ownerId));
+    }
+
+    @Override
+    boolean verify(ProofOfWork proofOfWork) {
+        double difficulty = adjustedDifficulty(proofOfWork.getNumLeadingZeros());
+
+        var puzzle = new Equihash(90, 5, difficulty).puzzle(proofOfWork.getChallenge());
+        return puzzle.deserializeSolution(proofOfWork.getPayload()).verify();

I think it would be better to extends the `ProofOfWork` fields to support the storage of the pow solution instead using the payload.
For the use case of the offers it might not matter as the payload is kind of irrelevant as the challenge contains already the offerID as well, but for other potential use cases (e.g. p2p network dos protection) it would be more useful to have a field for the payload and one for the challenge (which could be an interactive challenge nonce) and then have once field for the pow solution (counter in hashcash). Maybe to keep counter as informational field for the number of iterations but not clear yet to me if that makes sense for Equihash...

> @@ -769,7 +769,7 @@ message Filter {
     bool disable_api = 27;
     bool disable_mempool_validation = 28;
     bool disable_pow_message = 29;
-    int32 pow_difficulty = 30;
+    double pow_difficulty = 30;

The values are rather large (e.g. 500 000). I understand that in Equihash a double is used, but wouldn't it be sufficiet to convert an integer value to double and leave the proto field type as int32? 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/5858#pullrequestreview-816912239
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20211203/844e7f63/attachment.htm>