Add auto-confirm feature for Monero

[chimp1984]

I am complete with dev testing so far. Here an overview and hints for testing.

Specification:
This feature is only visible for XMR trades.
BTC buyer gets a new popup at payment sent button click asking for entering the XMR tx ID and XMR tx key. He can either enter valid 32byte hex strings (input validation used) or leave the fields empty. If he leaves one or both empty another popup will ask him if he is sure and give info about the requirements to provide those date in dispute case and that the peer cannot do auto-confirm if he does not provide it. But we do not enforce it. There might be users who sent from a wallet not supporting tx keys but as reciver gets the XMR there is no dispute and no follow-up problems. If we would enforce it we would cause dispute cases for such situations.
The data is sent to the BTC seller inside the trade message. Once received he starts requesting from the service(s) for the tx proof if he has the feature enabled.

There is a news badge and a popup informing about the new feature. By default it is disabled and filled with 1 BTC trade amount limit (auto confirm only used for trades up to 1 BTC), 5 required confirmations and the default service addresses (onion addresses for tor, clearnet for localhost dev testing). Atm there is only 1 service (from @Emzy) but @wiz has started to setup a service as well, so there will be 2 services soon. The user can change the service address (not containing 'http://') but cannot leave it empty, in that case it will be refilled with default values at focus out. The required confirmations are only written at focus out as well to avoid unintended values (those are read just in time at service response). The min number of success results it the number of services defined, so by default 2.

The Seller will see a new text field in the trade screen with the auto-confirm status. Initially the expected state is "Proof requests started". After a response it will show either the number of confirmations or "Transaction not seen in mem-pool yet". Once number of confirmations are >= required the service has completed with success (assuming all data are valid and proof was valid). If anything is incorrect or a connection problem the auto-conf fails (1 error or failure of any servive will terminate all). Failures do not mean that the tx was invalid, the user need to check manually in that case.

As long the tx is not in the mempool or the required confirmations are not reached we are in pending state and show how many services have succeeded and how many confirmations the tx has. It might be that both services report a diff. number of confirmations, in that case the last update is visible to the user. We do not have enough space in the UI to show the data per service but combine all in 1 text field. As long we are in that pending state the services get requested every 90 seconds for max. 12 hours. After 12 hous without success it has failed.
Once all services have succeeded the auto confirmation will get triggered. The trade is closed and in the UI there is an auto-confirmed badge. The state of the auto conf is also visible in the trade details window. The state gets persisted with the trade but in case if PENDING we reset the state when reading the persisted file as it does not make sense to show PENDING in that case. At startup the service will get started automatically in case the trade has not completed in the meantime. If the user clicks manually the confirm button the service will get stopped and the trade completes normally.
If the user updates the required confirmations in settings, this new value will be used at the next response. Also disabling the service in settings cause terminating all services. They will not be restarted if the user enables again. Only after restart the service requests will start again.
All other values in the settings are not applied on running services and changes only have effect at new trades or at a restart.

There is a new entry in filter to block the auto-confirm feature. If that is set, no new service can be started and existing ones get terminated. The settings UI get disabled as well. If filter gets removed or the flag reset we do not restart the pending trades, only at restart they request again.

How to test the feature?

If devMode is enabled dev values for txId txKey, xmr receiver address and amount are fixed set. With that you will always receive a success result from the service. Those dev test data are in XmrTxProofModel and can be changed to different ones.
To test with real values you have to disable devMode.

To test different states is a bit tricky and requires hacking a bit into the code.
I will describe what I did for dev testing:

• Change XmrTxProofRequest.REPEAT_REQUEST_PERIOD from 90 sec to 5 sec
• Comment out the body of the method autoConfirmFiatPaymentReceived in TradeManager. This avoids that the trade gets completed and you don't need to create new trades all the time but can simply restart the app and test another scenario with the same trade.
• Use the DevTestXmrTxProofHttpClient as mock to configure the timing behaviour, the confirmations and the result. To enable that class change the binding in TradeModule from bind(AssetTxProofHttpClient.class).to(XmrTxProofHttpClient.class); to bind(AssetTxProofHttpClient.class).to(DevTestXmrTxProofHttpClient.class);
• One can also set the required confirmations 1 block higher as the confirmations from the dev data (or your own tx) and simple leave the UI running and watch if the confirmation updates at a new XMR block and triggers the auto-confirm if the required confirmations are met.

There is a unit test for the parser. No integration test for the whole feature, would be quite a bit of effort and challenge.

EDIT: Just added support to listen on filter changes and adjusted text.

[chimp1984]

I am complete with dev testing so far. Here an overview and hints for testing.

Specification:
This feature is only visible for XMR trades.
BTC buyer gets a new popup at payment sent button click asking for entering the XMR tx ID and XMR tx key. He can either enter valid 32byte hex strings (input validation used) or leave the fields empty. If he leaves one or both empty another popup will ask him if he is sure and give info about the requirements to provide those date in dispute case and that the peer cannot do auto-confirm if he does not provide it. But we do not enforce it. There might be users who sent from a wallet not supporting tx keys but as reciver gets the XMR there is no dispute and no follow-up problems. If we would enforce it we would cause dispute cases for such situations.
The data is sent to the BTC seller inside the trade message. Once received he starts requesting from the service(s) for the tx proof if he has the feature enabled.

There is a news badge and a popup informing about the new feature. By default it is disabled and filled with 1 BTC trade amount limit (auto confirm only used for trades up to 1 BTC), 5 required confirmations and the default service addresses (onion addresses for tor, clearnet for localhost dev testing). Atm there is only 1 service (from @Emzy) but @wiz has started to setup a service as well, so there will be 2 services soon. The user can change the service address (not containing 'http://') but cannot leave it empty, in that case it will be refilled with default values at focus out. The required confirmations are only written at focus out as well to avoid unintended values (those are read just in time at service response). The min number of success results it the number of services defined, so by default 2.

The Seller will see a new text field in the trade screen with the auto-confirm status. Initially the expected state is "Proof requests started". After a response it will show either the number of confirmations or "Transaction not seen in mem-pool yet". Once number of confirmations are >= required the service has completed with success (assuming all data are valid and proof was valid). If anything is incorrect or a connection problem the auto-conf fails (1 error or failure of any servive will terminate all). Failures do not mean that the tx was invalid, the user need to check manually in that case.

As long the tx is not in the mempool or the required confirmations are not reached we are in pending state and show how many services have succeeded and how many confirmations the tx has. It might be that both services report a diff. number of confirmations, in that case the last update is visible to the user. We do not have enough space in the UI to show the data per service but combine all in 1 text field. As long we are in that pending state the services get requested every 90 seconds for max. 12 hours. After 12 hous without success it has failed.
Once all services have succeeded the auto confirmation will get triggered. The trade is closed and in the UI there is an auto-confirmed badge. The state of the auto conf is also visible in the trade details window. The state gets persisted with the trade but in case if PENDING we reset the state when reading the persisted file as it does not make sense to show PENDING in that case. At startup the service will get started automatically in case the trade has not completed in the meantime. If the user clicks manually the confirm button the service will get stopped and the trade completes normally.
If the user updates the required confirmations in settings, this new value will be used at the next response. Also disabling the service in settings cause terminating all services. They will not be restarted if the user enables again. Only after restart the service requests will start again.
All other values in the settings are not applied on running services and changes only have effect at new trades or at a restart.

There is a new entry in filter to block the auto-confirm feature. If that is set, no new service can be started and existing ones get terminated. The settings UI get disabled as well. If filter gets removed or the flag reset we do not restart the pending trades, only at restart they request again.

How to test the feature?

If devMode is enabled dev values for txId txKey, xmr receiver address and amount are fixed set. With that you will always receive a success result from the service. Those dev test data are in XmrTxProofModel and can be changed to different ones.
To test with real values you have to disable devMode.

To test different states is a bit tricky and requires hacking a bit into the code.
I will describe what I did for dev testing:

• Change XmrTxProofRequest.REPEAT_REQUEST_PERIOD from 90 sec to 5 sec
• Comment out the body of the method autoConfirmFiatPaymentReceived in TradeManager. This avoids that the trade gets completed and you don't need to create new trades all the time but can simply restart the app and test another scenario with the same trade.
• Use the DevTestXmrTxProofHttpClient as mock to configure the timing behaviour, the confirmations and the result. To enable that class change the binding in TradeModule from bind(AssetTxProofHttpClient.class).to(XmrTxProofHttpClient.class); to bind(AssetTxProofHttpClient.class).to(DevTestXmrTxProofHttpClient.class);
• One can also set the required confirmations 1 block higher as the confirmations from the dev data (or your own tx) and simple leave the UI running and watch if the confirmation updates at a new XMR block and triggers the auto-confirm if the required confirmations are met.

There is a unit test for the parser. No integration test for the whole feature, would be quite a bit of effort and challenge.

EDIT: Just added support to listen on filter changes and adjusted text.

[ripcurlx]

utACK/ACK

Tested it roughly on Regtest, but will test it in more detail during release testing.

[chimp1984]

@m52go Could you add the content to the wiki about auto-conf (https://bisq.wiki/Trading_Monero#Auto-confirming_trades)?
#4458 (comment) can be helpful

[m52go]

Updated, feedback welcome:
https://bisq.wiki/Draft:Trading_Monero#Auto-confirming_trades

Will take it out of draft status shortly so it's online when the new release is available.

[ghost]

Code review of onion-monero-blockchain-explorer

The purpose of this review is to determine if using this explorer to validate receipt of XMR funds poses any risk.

The onion-monero-blockchain-explorer provides a web-API to prove that funds have been sent to an address.

tldr;

onion-monero-blockchain-explorer is a thin wrapper over the monero library, providing access over a web API call. There is no modification of the data in or out of the monero library via the single API call used by Bisq (/api/outputs).

---

Background

Two instances of the explorer are hosted by Bisq ops team and queried from around the globe by Bisq clients that have opted-in to the XMR tx auto confirm feature. The feature removes the need for an XMR buyer to manually check that their wallet has received the funds, allowing those trades to complete automatically, paying out the BTC funds from the multisig.

How could an attacker use this tool to defraud? An attacker in this case would be an XMR seller who does not actually send the funds or the correct amount of funds. The explorer could indicate that funds have been sent to the queried address when indeed they have not. But the explorer does not know what amount the user is expecting to receive so if it is to falsify the results it must have a way of knowing the amount to send in the response back to the client. (only an address, txId and viewkey are provided to the API call).

So there would have to be a specific trigger to cause the fake result (expected amount) to be sent.

The amount could be encoded into a fake txId/viewkey. The attacker would generate the txId/viewkey in such a way that they are recognizable as such by the explorer, encoding the correct amount inside. The explorer could extract the amount, returning a success status with amount back to the user.

Or an old transaction could be used which has a matching amount, which the malicious explorer code would modify to look like a new transaction.

Either way, if there was malicious code included in onion-monero-blockchain-explorer, you would expect to see some conditional logic to handle the above, some special handling of the supplied hashes, or special handling of the results looked up in the blockchain.

---

Code walk-through of the API

The onion-monero-blockchain-explorer is built using the monero LMDB library for interfacing to the blockchain. Interface to the monero library is via MicroCore.h which provides methods:
get_core(); get_mempool(); get_block_by_height(); get_tx(); find_output_in_tx(); get_blk_timestamp(); get_block_complete_entry();

The main.cpp entry point checks command line options, starts a mempool monitoring thread, and sets up the URL routes it responds to, along with their respective parameters. The only route that Bisq uses is /api/outputs which corresponds to the method xmrblocks.json_outputs() in page.h.

json_outputs first checks the three parameters tx_hash_str, address_str, and viewkey_str. They must not be empty.

It calls xmreg::parse_str_secret_key with tx_hash_str and does the same with viewkey_str. parse_str_secret_key() calls parse_hash256 which is part of the monero library. It converts a hex string hash to binary data. It calls xmreg::parse_str_address with address_str which in turn calls get_account_address_from_str() in the monero library.

Next it looks for the tx_hash in the mempool or blockchain (find_tx). If not found, the method returns with an error "Cant find tx hash". MicroCore::get_tx() is called to get the transaction info. This in turn calls cryptonote::Blockchain.get_db().get_tx() which is the monero LMDB library.

If not found in the blockchain, the mempool is checked via search_mempool which calls get_mempool_txs and iterates through the results looking for a matching txhash. MempoolStatus.cpp contains the code for maintaining a list of active mempool transactions. It in turn delegates to cryptonote::Blockchain.

Next, it calls get_tx_details. It loads all the information about the transaction, such as number of inputs, outputs, fee, size, version, unlock time, and number of confirmations. Much of this data is obtained by calling summary_of_in_out_rct which is in tools.cpp and just iterates through structs contained in the transaction object.

Next, it iterates over each output from the transaction details (output_pub_keys). For each output, it includes in the json data returned to caller: "output_pubkey","amount","match","output_idx". "match" is set to true if the "output_pubkey" matches the supplied decoded address (i.e. is the user's address)

The "tx_timestamp" is obtained from the transaction's block if present in a block, or the mempool.

tx_hash", "address", "viewkey", and "tx_prove" are pulled from data structures used above and returned to the caller in the json.

"tx_confirmations" is pulled from the transaction details object and returned in the json.

---

Conclusion

No malicious code found. See tldr; above.

[sqrrm]

@jmacxx I had a quick look through onion-monero-blockchain-explorer as well and came to the same conclusion. I think there is minimal risk using it right now. The main risk would be future updates to that code as it's now known how bisq uses it.

Since auto confirm users won't rely on random explorers but rather federated bisq explorers, or better, their own, I see that as a small risk. Any update to the federated explorers would have to be reviewed though, and we should let users know which version is used by the federated node operators. That way they can have some confidence that it's a good version if they choose to install it themselves.

[chimp1984]

Good point regarding updates.
@wiz @devinbileck @Emzy Please always double check with devs before applying an update. Not assuming anything of course but better be cautious than sorry.
To add the version to the response might be a nice feature as well, so we get it in the logs.