Bisq: Bisq doesn't validate XMR addresses

[vp1111]

I'm using Bisq on Linux (Debian), version 0.8.0.
When creating an "Altcoin account" and selecting Monero (XMR), I can write whatever in the field "Altcoin address". It doesn't validate anything. I can write an actual XMR address or I can write "banana" and it will go through, letting me start a trade on it.

[ManfredKarrer]

We can offer a 100 BSQ bounty for any dev who implements an XMR address validation. Feel free to promote that in the Monero communities!

[Gingeropolous]

@ManfredKarrer , please include, as part of your bounty criteria, that the validation includes the new subaddress types that start with an 8. I don't know if bisq uses integrated payment IDs, but subaddresses should be favored over integrated PIDs.

[Technohacker]

Done :sweat_smile:
Standard addresses and Subaddresses are accepted :D

[ManfredKarrer]

@Gingeropolous Thanks for the information. Bisq does not use payment IDs, just regular addresses. In case of a dispute the XMR sender needs to provide the tx key to the arbitrator.

[ManfredKarrer]

@Technohacker That was fast! ;-)

@Gingeropolous Could you help to review the PR?

[Technohacker]

@ManfredKarrer If you don't mind the meme, _"The fastest PR in the west"_ :sweat_smile:

[Gingeropolous]

unfortunately im just a hacker groupy

[ManfredKarrer]

@Technohacker Haha ;-)

Don't forget to file a compensation request end of the month (PR at https://github.com/bisq-network/compensation)

[Technohacker]

@ManfredKarrer I'll have a look, thanks!
So should I do it as soon as the PR is merged?

[ManfredKarrer]

Yes once its merged you can file the request. I will be not much online next days, so might take a bit for review and testing...

[fireice-uk]

@Technohacker Well done :+1:

[sneurlax]

Weak validation, doesn't ensure that addresses are actually valid, just that they're prefixed with a 4 or an 8 and the correct number of correct characters--not that the checksum validates or anything like that.

Does anybody know of a Java implementation of cryptonote address validation?

[Technohacker]

@sneurlax Yes it's being discussed in the linked PR. There are no Java implementations for Cryptonote address validation, hence we need to roll our own. We're searching for a Keccak-256 library due to Bisq dropping Bouncy Castle for Java 10.

[Gingeropolous]

none of the things at the bottom of http://llcoins.net/ do address validation? i find that hard to believe.

[Gingeropolous]

mebbe this one

https://xmr.llcoins.net/addresstests.html

[Technohacker]

@Gingeropolous Yes we know about that one. I mentioned Java implementations don't exist. That's in JS.

[sneurlax]

@Gingeropolous that's what I was thinking. I was just looking for the hash function in Java. I'll roll it up if Technohacker can't

[Technohacker]

@sneurlax I sure would be kinda busy for a while, so if I can't get one ready, you can go ahead. If possible, you can use my class in the PR for a generic CryptoNote address validator :)

[ManfredKarrer]

@sneurlax @Technohacker We plan a new big release around mid of October. Would be great if we can get that PR ready until then? Otherwise we can add it as a partial validation if the checksum part makes too much troubles.

[Technohacker]

@ManfredKarrer The PR's actually ready for general Cryptonote addresses (sans checksum). @sneurlax should be able to get the checksum code ready :)

[ManfredKarrer]

Basic validation is merged.
For the checksum validation we delegate it to a new PR.

[blur-network]

@ManfredKarrer After discussing a bit with @Technohacker, and tooling around with the Address Tests on xmr.llcoins.net . . . It turns out that the public address for any cryptonote coin with a "prefix length" of 2 characters will fail the tests provided by CryptonoteAddressValidator in #1683.

When a netbyte for a given currency (b201 in Aeon, or cd3c for Blur) is four characters, it seems that the base58 encoding from cryptonote standard will result in a 97-character public address.

I'm going to add a conditional statement to the CryptonoteAddressValidator, and force push the changes to the already opened PR #1723 for listing of BLUR.

[blur-network]

PR #1723 has been updated to validate two-character prefixes (Blur, Aeon, etc...) in general Cryptonote public addresses and subaddresses. Does not validate against checksum, though. Merely expands upon validation method from @Technohacker.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[ripcurlx]

Closing as complete.

[ManfredKarrer]

The validation is only partly complete. As XMR is our most traded asset I think we should ry to get a more complete validation some day. Nothing high prio as XMR traders are smart enought to not make copy paste errors with adding the address :-) but would be still good to get that completed.

[xiphon]

would be still good to get that completed

on it, will implement complete and correct address validation.

[ManfredKarrer]

@xiphon Great! Please try to not introduce a new library dependency (for crypto stuff).

[xiphon]

Sure, it will be zero-dependency code, including own Keccak1600 hashing class and Monero Base58 decoder.

What way would you prefer:

1. Place Keccak1600 and MoneroBase58 private classes right in the CryptonoteAddressValidator.java source.
   Given that both won't be used elsewhere in the bitsq codebase, looks good to me.
   Though might be somewhat less appropriate from Bisq code organization point of view.
2. Place Keccak1600 and MoneroBase58 classes in bisq.common.crypto

[ManfredKarrer]

@xiphon
Great!
I perfer to keep it in the CryptonoteAddressValidator.