Autoconfirm XMR trade if tx check was validated by proof service

[ghost]

This is a follow-up PR to #4378 by @chimp1984

Implements bisq-network/proposals#86

Features:

• XMR seller is prompted to enter txId and viewkey.

On the XMR buyer side,

• looks up the XMR transaction to verify that it has confirmed (using onion-monero-blockchain-explorer)
• user can run their own validating service, or use the ones provided
• 100% agreement of all chosen services is necessary for confirmation
• feature can be configured and turned on/off from settings screen
• feature can be globally turned off via admin filter

Implementation notes

Auto confirmation entails API requests to multiple autoconf service providers. If the responses all show that the required number of confirmations have been reached the trade will be auto confirmed.

The user still has the option to confirm manually just as if auto confirm were not enabled.

Any service failure will abort the attempt at auto confirming.

• if a service is down, we fail with CONNECTION_FAIL
• if a service returns garbage response, we fail with API_INVALID
• if a service returns a different address, we fail with ADDRESS_INVALID
• if a service returns a different txId, we fail with TX_HASH_INVALID
• if a service returns a different viewkey, we fail with TX_KEY_INVALID
• if a service returns a match, but the amount is different we fail with AMOUNT_NOT_MATCHING
• if a service returns transaction not found we wait then re-try
• if a service returns less than the required confirm count, we wait then re-try
• if a service returns >= the required confirm count, TradeManager decrements its count of outstanding service calls. Once the count reaches zero it transitions the trade to confirmed.

In any of these cases the status is shown on screen. It also shows the progress of the confirmation count.

The general approach to implementing this feature was to keep it as simple as possible.

Auto confirmation is a one-shot process triggered by receipt of counterCurrencyExtraData in the CounterCurrencyTransferStartedMessage from the counterparty.

If Bisq is closed while auto confirmation was pending, when it re-starts the trade will show auto conf status disabled.
Auto-confirmed status of a trade is not persisted. Auto-confirmed status is persisted since for trade history it might be important to have some "proof" if it was used or not, as well in dispute cases. [requested by @chimp1984].

Testing

In regtest use a clearnet service address, e.g. 78.47.61.90:8081 (from @Emzy)

In mainnet over tor this would be monero3bec7m26vx6si6qo7q7imlaoz45ot5m2b5z2ppgoooo6jx2rqd.onion which is included as a default setting.

To try out this feature without making a live XMR trade, the following example TX proofs may be used.

Sent 0.12121212 XMR (for sell 0.0012 BTC @ price 0.0099)
to:  4ATyxmFGU7h3EWu5kYR6gy6iCNFCftbsjATfbuBBjsRHJM4KTwEyeiyVNNUmsfpK1kdRxs8QoPLsZanGqe1Mby43LeyWNMF
tx:  0d0d5e74e56c9060095e62b92bce2c7fe8da43a28d3e11a7c51ffe0749fe4dd4
key: 443263d31c8b05f5dd5734128fcb3cac83d0fd3c90b03ec19d5131216a5e3505

Sent 0.1 XMR (for sell 0.001 BTC @ price 0.01)
to:  4ATyxmFGU7h3EWu5kYR6gy6iCNFCftbsjATfbuBBjsRHJM4KTwEyeiyVNNUmsfpK1kdRxs8QoPLsZanGqe1Mby43LeyWNMF
tx:  e8dcd8160aee016d8a0d9c480355d65773dc577313a0af8237c35f9d997b01c0
key: 300fa18ff99b32ff097d75c64d62732bdb486af8c225f558ee48c5f777f9b509

Automated testing routines were created to verify the API JSON response handling.

Checking iteroperability between new and old clients.

• If BTC Seller = new BTC buyer = old, the buyer is not prompted for txId and key and the new proto field is not sent to the BTC seller. Therefore auto-confirmation will not be started and the trade will operate in the old way, manual confirmation.
• If BTC Seller = old BTC buyer = new, the buyer provides txId and key and sends the new proto field to the BTC seller. The BTC seller does not handle the new field and the trade will operate in the old way, manual confirmation.

Test cases

• New user (preferences)
• Auto-confirm success
• Premature manual confirm by user
• XMR Tx Id / key not provided
• XMR Tx not found
• Re-used XMR TxId
• Feature disabled in settings
• Feature disabled by filter
• Transaction limit exceeded
• Invalid amount
• XMR Trade date too old
• XMR Trade date newer than Bisq trade date (ok)
• Bisq shut down during Auto-confirmation procedure
• XMR API service not reachable

Discussion

• Perhaps some XMR sellers may not want (or be able) to provide the txId and viewkey? In interest of flexibility, the UI allows those values to be optional and simply disables autoconf for the trade if the keys are empty or do not pass validation.

[chimp1984]

@jmacxx
Great to see the final PR! I will try to review on the weekend as soon I find time.

Perhaps some XMR sellers may not want (or be able) to provide the txId and viewkey?

It is a requirement for XMR that they use a wallet capable of providing the tx key for the BTC buyer to be able to proof the XMR transfer. In a dispute the mediator would not be able to verify if the XMR transfer was done otherwise. So we just make it more safe in the way that if user ignore the info in the popup and use a not capable wallet they would risk to lose the dispute case. Now they cannot complete the trade, or at least they get aware of the problem very early.

[chimp1984]

@boring-cyborg Wrong labelling!

[chimp1984]

@jmacxx Thanks for the work on the feature! Looks pretty good already.

Some comments:

• I would recommend to use Capitalistion in comments and log messages. Proper sentences makes logs easier to read.
• Could you merge current master into your branch, should not cause any conflicts.
• Translation strings should be lower case inside sentence...
• Hex validation is done in several places. Would be better to move to Hex util class. Regex is not needed as encode/decode throws if input is wrong. Just need additional lenght check.
• Maybe we should consider to move the processCounterCurrencyExtraData method in TradeManager to a new class to not over burden the TradeManager too much with this specialized feature. It is quite large already...
• We mix a bit between generice feature and XMR specific. Even there are no concrete plans to add the autoconf feature to other altcoins we might add it (e.g. if it get sponsored by the altocin), so better to have all prepared to not get issues when we add more. UI can easily be changed, but data in protobuf, trade, messages are more problematic.
• In preferences view the auto conf feature is at the very bottom. We could decrease vertical space for currency selection to make it more prominent or even move it in front of currency selection.
• The addressRegexValidator does not allow http:// only the IP or onion.
• Deleting the service address does not refill it with default only at UI screen change or restart. Its a bit tricky to do that. Usually we used a focusOutHandler on the inputfield and apply it when the user leaves focus.
• The Auto-confirmed badge label is not veritcally centered. Might be a rounding issue with layout...seems to be 1 pixel off.
• Not sure if needed, but might be nice to see the status information at the sellers pending trade view. e.g. how many confirmations from which services. But not sure if UI space allows that. Can be added also later if we see demand for it.
• For dev testing it would be good to deactivate certain checks, like that the key was not used in the past, the date check and the amount check. Otherwise testers need to create a XMR tx for each test run... The DevEnv.devMode property could be used to ignore those checks. Also would be convenient to autofill the buyers tx id and txkey fields with valid data.
• We should consider to allow emty tx ID and tx key input but show a warning if user does not want to send those data. I am not aware of any privacy issue by revealing those data, but it could be that there are some or at least some traders think there are some. Beside that if the users used a wallet which does not provide those data it would lead to a dispute case as he cannot confirm that he sent the xmr. It is mandatory to use a wallet which supports those data, but not sure if we should enforce disputes where otherwise if the seller recieved it there would not be a dispute.
• We should also state in the info popup that it is best for security and privacy that the user runs his own service node and should provide some easy setup instructions (e.g. install script) . beside that we should make clear that there might be reasons why the severice does not provide the result and the user still need to check in the permitted trade time the state of the trade.
• We should add some more detailed info text outside of the app (e.g. blog, wiki, docs). @m52go what do you think? We should make clear the limitations and context of the service. E.g. security is based on assumption that 2 bonded role owners do not collude to defraud traders. There is a privacy loss as the service operators learn about the matching of an onion address and a XMR tx id and amount. All that can be avoided if user runs own service. And of course it is an optional feature if traders dont feel comfortable with it.
• It would be nice to get some statistics how much the feature is used. We could get number of trades using the feature from the node operators (excluding those who run their own service).
• Once we are in a more final state we should think about the edge cases like when wallet is not synced.... also to define test use cases would help ourself for dev testing and later the testers for release testing.

[ripcurlx]

The boring bot unfortunately will always tag it as no-priority as it touches the altcoin repo. Just ignore this for now as it has priority.

[m52go]

We should add some more detailed info text outside of the app (e.g. blog, wiki, docs).

Sure, I can write draft points to cover in the next day or two for feedback to make sure material is covered completely and correctly.

[ghost]

We mix a bit between generice feature and XMR specific. Even there are no concrete plans to add the autoconf feature to other altcoins we might add it (e.g. if it get sponsored by the altocin), so better to have all prepared to not get issues when we add more. UI can easily be changed, but data in protobuf, trade, messages are more problematic.

Changed proto definition to allow a list of AutoConfirmSettings.
Commit 4dde9e1

Maybe we should consider to move the processCounterCurrencyExtraData method in TradeManager to a new class to not over burden the TradeManager too much with this specialized feature. It is quite large already...

Moved the new code out to AutoConfirmManager.java
Commit 13978e3

Hex validation is done in several places. Would be better to move to Hex util class. Regex is not needed as encode/decode throws if input is wrong. Just need additional lenght check.

Is this referring to the Transaction ID and key input fields? I should not restrict them to hex input?

The addressRegexValidator does not allow http:// only the IP or onion.

address:port is all that is needed. Bisq chooses the protocol (http).

Not sure if needed, but might be nice to see the status information at the sellers pending trade view. e.g. how many confirmations from which services. But not sure if UI space allows that. Can be added also later if we see demand for it.

It shows the number/expected confirmations.

The Auto-confirmed badge label is not veritcally centered. Might be a rounding issue with layout...seems to be 1 pixel off.

I intentionally oriented the badge slightly above and to the right of the title to make it appear similar to other badges shown on menu items. I think it looks great that way, but could certainly move it down a bit if that is what you want.

We should also state in the info popup that it is best for security and privacy that the user runs his own service node and should provide some easy setup instructions (e.g. install script) . beside that we should make clear that there might be reasons why the severice does not provide the result and the user still need to check in the permitted trade time the state of the trade.

Done. Commit 9914600
Will post a screenshot so @m52go can review.

We should consider to allow emty tx ID and tx key input but show a warning if user does not want to send those data. I am not aware of any privacy issue by revealing those data, but it could be that there are some or at least some traders think there are some. Beside that if the users used a wallet which does not provide those data it would lead to a dispute case as he cannot confirm that he sent the xmr. It is mandatory to use a wallet which supports those data, but not sure if we should enforce disputes where otherwise if the seller recieved it there would not be a dispute.

It allows empty entry; I've also added the warning in that case. Will post a screenshot so @m52go can review.

For dev testing it would be good to deactivate certain checks, like that the key was not used in the past, the date check and the amount check. Otherwise testers need to create a XMR tx for each test run... The DevEnv.devMode property could be used to ignore those checks. Also would be convenient to autofill the buyers tx id and txkey fields with valid data.

Done.

Deleting the service address does not refill it with default only at UI screen change or restart. Its a bit tricky to do that. Usually we used a focusOutHandler on the inputfield and apply it when the user leaves focus.

Done.

I would recommend to use Capitalistion in comments and log messages. Proper sentences makes logs easier to read.
Translation strings should be lower case inside sentence...

Duly noted; I have attempted to correct all those instances of mis-capitalization.

[ghost]

@m52go

New settings feature text

Warning when XMR trans keys not provided

[chimp1984]

@jmacxx Thanks for your fast response and implementations! I will try to find time for updated review tomorrow.

[chimp1984]

We need at least 2 providers for DEFAULT_XMR_PROOF_PROVIDERS. Can you push wiz and emzy that they have the updated versions running? One need to run with clearnet.

Clearnet handling of provider for non-tor dev setup is missing.

In Trade:
Do not set xmrProofResult in constructor as it is only used in special circumstances. Keeping it null is more clear. (-> remove xmrProofResult = new XmrProofResult(0, 0, XmrProofResult.State.FEATURE_DISABLED);)
Makr xmrProofResult transient and @nullable
setXmrProofResult can be package scope.

// xmrProofResult is not persisted yet
@Getter
@Nullable
private transient XmrProofResult xmrProofResult;

void setXmrProofResult(XmrProofResult xmrProofResult) {
    this.xmrProofResult = xmrProofResult;
    xmrProofResultProperty.setValue(xmrProofResult);
}

We should consider to persist XmrProofResult in trade. for trade history it might be important to have some "proof" if autoconfirm was used or not, as well in dispute cases.
if we use propbug for XmrProofResult the enum should not be used as enum in protobuf as protobuf enums are cumbersome (have global scope). use the name instead. see other enum serialisations...(ProtoUtil.enumFromProto)

Still capitalized:
setting.preferences.autoConfirmXMR=XMR Auto Confirm
setting.preferences.autoConfirmEnabled=Enabled
setting.preferences.autoConfirmRequiredConfirmations=Required Confirmations
setting.preferences.autoConfirmMaxTradeSize=Max Trade Size (BTC)
setting.preferences.autoConfirmServiceAddresses=Service Addresses
setting.info.headline=New XMR Auto confirm feature
filterWindow.disableAutoConf=Disable Auto-confirmation (altcoins)

setXMRTxKeyWindow.txHash=Transaction id -> setXMRTxKeyWindow.txHash=Transaction ID
setXMRTxKeyWindow.txKey=Tx key -> setXMRTxKeyWindow.txKey=Transaction key

The second popup at xmr key input is removing the shaded background. You need to start that second popup with a short delay (UserThread.runAfter with 50 ms or so).
Auto confirm settings should be better visible. Either move it on top or reduce number of entries in currency list.

handleProofResult:
This handling need more thought/work:
if (!walletsSetup.hasSufficientPeersForBroadcast()) {
return failure;
}
if (!walletsSetup.isDownloadComplete()) {
return failure;
}

[chimp1984]

@m52go Can we add a short description and install help to the wiki?

@Emzy @wiz Can you run the updated verison of the services (with tx date delivered)? one service need to provide clearnet as well (i think emzy has that already) - for dev testing in non-tor setup.
Could you provide a installation script so users can easily install the service themself? I got lots of troubles on Debian with compiling the tool...Seems on Ubuntu its easier...

I think it should be handled as bonded role. We could reuse DATA_RELAY_NODE_OPERATOR as role for that.

[Emzy]

@Emzy @wiz Can you run the updated verison of the services (with tx date delivered)? one service need to provide clearnet as well (i think emzy has that already) - for dev testing in non-tor setup.

I just build from master.
If we will use this in production, I need to move the explorer to another machine. It's running on a old pc at home.

[ghost]

Hi @chimp1984 thank you for taking time to review again.

Clearnet handling of provider for non-tor dev setup is missing.

Clearnet in dev mode is by default - no special handling needed. The SOCKS5 proxy provider is null in dev mode so HttpClient uses clearnet. To attest to that I've been using emzy's clearnet service for the past few weeks. The relevant line in HttpClient:

bisq/p2p/src/main/java/bisq/network/http/HttpClient.java

Lines 97 to 100 in f45a26c

	if (ignoreSocks5Proxy || socks5Proxy == null || baseUrl.contains("localhost")) {
	log.debug("Use clear net for HttpClient. socks5Proxy={}, ignoreSocks5Proxy={}, baseUrl={}",
	socks5Proxy, ignoreSocks5Proxy, baseUrl);
	return requestWithGETNoProxy(param, headerKey, headerValue);

If I have misunderstood please correct me.

I am working on all other points you brought up. Then I'll merge in latest from master as requested (after squashing my commits).

Cheers.

[chimp1984]

@jmacxx Made a PR to your branch.... https://github.com/jmacxx/bisq/pull/1

[ghost]

Thanks @chimp1984, I'm AFK for the next few hours, will look at this when I return.

[ghost]

@chimp1984 I attempted to merge your pull request on top of my branch but got stuck in too many merge conflicts. It seems we have been refactoring the code at the same time. I think the best option would be to close this PR and you open a new one with your current snapshot of the code.

[ghost]

I've merged your latest @chimp1984 . Will test it out.

[chimp1984]

Thanks @jmacxx ! I will continue soon as well testing. best to use 2 services for testing and add some artificial delay at the repsonses so u can see how ui behaves... i used same service twice with some small hacks to not ignore it as it has same uid..

I pinged @wiz how his progress is with getting a node. To have 2 nodes is a mandatory requirement for release IMO.

[chimp1984]

@jmacxx I tried to do a pR to your branch but it did not work. So I did a new PR to Bisq. #4458

[ghost]

Closing as replaced by #4458