Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Autoconfirm XMR trade if tx check was validated by proof service (using tx private key) #4378

Closed

Conversation

chimp1984
Copy link
Contributor

Implements bisq-network/proposals#86

Main concept:

  • User has option in preferences to automatically confirm XMR receipt if a request to the XMR tx proof service succeeded.
  • Tx proof service need to be run by trusted Bisq contributors as a bonded Role. I suggest to reuse the DATA_RELAY_NODE_OPERATOR for that role. It runs over Tor by default so we avoid exit nodes.
  • Currently its a polling service. As the source code is in C++ I doubt that a Bisq dev wants to get into that to add support for a subscriber model. An alternative approach could be a small proxy in Java which runs on the same machine and polls on a fast interval to the service. The Bisq nodes can subscribe at the proxy for the txs they are interested in and get called once the tx is confirmed. Alternatively we could reach out to @moneroexamples who wrote it to see if there is some support already included or if he would be interested to add support.
  • We also should add an option to use one's own service, either localhost or remote.

Main part missing is the XMR proof service request processing. I did not get the service compiled and running yet, so could not test response data and error conditions.

Further it is missing a "news badge" and popup to guide the user to the new feature.
Currently there is no indication at the last trade step the the trade was "auto confirmed". I guess that would be a good addition to show some icon or text info.

We also should log the result from the service for security reasons. Multiple services with parallel requests which must match would be good to reduce risk that a service get compromised.

Another idea is to add a trade amount threshold so that the autocomfirm is only used for trades up to that limit. That would make it easier to users to navigate risk exposure.

Anyone welcome to take over the project from here as I might not have time soon to continue.

Main part missing is the XMR proof service request processing. I did not
get the service compiled yet, so could not test response data and error
conditions.

Further it is missing a "news badge" and popup to guide the user to the
new feature.

Only basic dev tested so far.

Anyone welcome to pick the project up from here as I might not have
time soon to continue.
@chimp1984
Copy link
Contributor Author

The Codacy/PR Quality Review complaints are about code which is not finished as the service is not available yet.

@Emzy
Copy link
Contributor

Emzy commented Jul 31, 2020

@ghost
Copy link

ghost commented Aug 2, 2020

I started work in a copy of your branch. The plan is to address the TODO items in the code then look at some of the extra features described above.

@chimp1984
Copy link
Contributor Author

@jmacxx Thanks for picking the PR up!

One more (small) feature we should add:

  • Add a new filter entry to disable automatic XMR confirmations. This can be activated by the developer who has the filter key and would help to stop that this feature is used in case of a hack or severe bug.

I think we should have at least 2 services running (@Emzy has it already, @wiz signaled that he is interested to run as well one) and require the request to both and that both results are matching.

@ghost
Copy link

ghost commented Aug 6, 2020

I've been testing, attempting to force the "unconfirmed" scenario where we wait to ping again the API. But the tx proof service we're using reports a match instantly for the transaction, even before 1 block confirmation. I'd imagine we would want to wait the standard 10 blocks before announcing that the tx is confirmed.

  • https://www.exploremonero.com/receipt waits 10 blocks before indicating the transaction is confirmed.
  • MyMonero wallet waits at least 10 blocks before marking the transaction as confirmed.

So this may be an issue. Maybe we need to use the other api from @moneroexamples: api/outputsblocks. I will try that & report back.

[edit:]

  • api/outputsblocks does not allow us to verify a transaction confirmation status.
  • api/outputs would be great for us if it included the number of block confirmations so we could decide at how many blocks we consider the tx confirmed.

[edit2:]
The following change to api/outputs should provide the number of block confirmations for the tx:

diff --git a/src/page.h b/src/page.h
index e170d14..7f9a5ce 100644
--- a/src/page.h
+++ b/src/page.h
@@ -5413,6 +5413,7 @@ json_outputs(string tx_hash_str,
     j_data["address"]  = pod_to_hex(address_info.address);
     j_data["viewkey"]  = pod_to_hex(prv_view_key);
     j_data["tx_prove"] = tx_prove;
+    j_data["tx_confirmations"] = txd.no_confirmations;

     j_response["status"] = "success";

I will test it out once monerod has synced. ⏳

[edit3:]
Above patch works great for checking number of confirmations, submitted to @moneroexamples
PR: moneroexamples/onion-monero-blockchain-explorer#212

@chimp1984 chimp1984 changed the title [WIP] Autoconfirm XMR trade if tx check was validaated by proof service (using tx private key) [WIP] Autoconfirm XMR trade if tx check was validated by proof service (using tx private key) Aug 6, 2020
@ghost
Copy link

ghost commented Aug 9, 2020

@chimp1984 How does this look for settings? (GUI & protobuf representation)


image


///////////////////////////////////////////////////////////////////////////////////////////
// Preferences
///////////////////////////////////////////////////////////////////////////////////////////

message PreferencesPayload {
[...]
    repeated AutoConfirmSettings auto_confirm_settings = 56;
}


message AutoConfirmSettings {
    TradeCurrency coin = 1;
    bool enabled = 2;
    int32 nConfirmBlocks = 3;
    int32 nServicesMustConfirm = 4;
    int64 amountMaxLimit = 5;
    int64 amountUsed = 6;
    repeated string serviceUrl = 7;
}

@chimp1984
Copy link
Contributor Author

@chimp1984 How does this look for settings? (GUI & protobuf representation)

image

///////////////////////////////////////////////////////////////////////////////////////////
// Preferences
///////////////////////////////////////////////////////////////////////////////////////////

message PreferencesPayload {
[...]
    repeated AutoConfirmSettings auto_confirm_settings = 56;
}


message AutoConfirmSettings {
    TradeCurrency coin = 1;
    bool enabled = 2;
    int32 nConfirmBlocks = 3;
    int32 nServicesMustConfirm = 4;
    int64 amountMaxLimit = 5;
    int64 amountUsed = 6;
    repeated string serviceUrl = 7;
}

Thanks for the info.

This is quite more advances as I had in mind. I guess that will require a small sub UI to not blow up the settings too much. Maybe an new tab? Specially if its designed for extending to other altcoins. The coin selection would create new setting entries for each coin. It would have a similar structure like the account UI (list of entries with details view on selection and create/delete buttons).

The amountUsed and reset button are not totally clear to me. Reset that currently used amount? But what time interval or is it counting up and reset is only way to clear it? Then its a bit annoying as user need to maintain that manually...
Maybe its enough to set a limit per trade (e.g. only auto confirm for trades < 1 BTC).

Regaring other altcoins:
With other altcoins its a block explorer look up but we have a small open problem there: How do identify a tx in case of 2 trades with exactly the same trade amount? The BTC buyer has to send the TX ID but as he knows the receiver address he could look up and if he finds a recent tx he could use that one, and then the seller does not know which one has paid. With manual payout its not a big deal as he can simply wait until both are transferred or ask via the chat both traders. With auto confirm though that could lead to the wrong BTC payout.
I guess there is no easy solution but as it is a rare edge case I would suggest to deactivate the autoconfirm in case the user has multiple trades with same altcoin amounts.

@ghost
Copy link

ghost commented Aug 12, 2020

Thanks for the feedback - the complexities you mentioned were my mis-interpreting the requirements. I have changed them to simplify, proto now looks like this:

message AutoConfirmSettings {
    bool enabled = 1;
    int32 confirm_blocks = 2;
    int64 trade_limit = 3;
    repeated string service_addresses = 4;
}

Settings GUI looks like this:
image

Not sure how worth-while it will be to implement other alt-coins since they are barely traded and the overhead in addition to coding would be for ops to run trusted explorer APIs & blockchains for each coin. For now I opted to keep it simple especially with the GUI: just autoconf for XMR.

Thanks to @moneroexamples the API now includes the confirmation count so Bisq app waits until the relevant number have been received. @Emzy if you have time, could you update to the latest master branch of onion-monero-blockchain-explorer? The test API service has been incredibly useful, I've been using it to check the scenario of multiple API providers.

Code checked into my branch is functional but still needs cleaning up:

  • strings need to be put into displayStrings.properties
  • indication on final trade step screen needs improving
  • need some way to hardcode known providers addresses similar to what is done with price and fees APIs

I will continue to address the above, just wanted to post the progress update. At this point it is functionally ready to be tried out or reviewed if anyone wishes to do so.

@chimp1984
Copy link
Contributor Author

I agree regarding Altcoins. Running the explorers can easily become expensive and is not justified by trade volumes.

UI looks good! I think that way we can squeeze it into existing settings. Maybe a separtor and a headline would be good to keep it distinct from other options. The comma separated list is a good idea, no need for more complex UI for now. We can fill it up with the existing nodes and the user can edit it with own node if wanted.

Ah great that @moneroexamples was so fast!!!

I would love to try it out but will be probably busy with non Bisq work over the next days/weeks. But will see when I find time.
Did a quick review of your branch... will review more in depth once its in a PR. Feel free to make a new PR, I will close then this draft.

@ghost
Copy link

ghost commented Aug 15, 2020

Thanks for the code review, I made the suggested changes, and the todo list is complete.
Its looking quite good, I'll do a day or two more testing then create a PR.

Step 3 waiting for XMR confirmation

image

Step 4 auto confirmed!

image

@wiz
Copy link
Contributor

wiz commented Aug 15, 2020

I think we should have at least 2 services running (@Emzy has it already, @wiz signaled that he is interested to run as well one) and require the request to both and that both results are matching.

Yes, I'm very happy to host any nodes you want, but the idea of automatically releasing funds based on what TTPs say is a bit against the security model of Bisq, isn't it?

@ghost
Copy link

ghost commented Aug 15, 2020

Optionality exists:

  • User can enable/disable the feature, it is disabled by default.
  • User has the option to connect Bisq to their own monero explorer.

@ghost
Copy link

ghost commented Aug 16, 2020

@Emzy any chance you can upgrade your testing service to latest master? moneroexamples/onion-monero-blockchain-explorer@395d8f4

@Emzy
Copy link
Contributor

Emzy commented Aug 17, 2020

@Emzy any chance you can upgrade your testing service to latest master? moneroexamples/onion-monero-blockchain-explorer@395d8f4

It's updated to master.

@chimp1984
Copy link
Contributor Author

Close as superseded by #4421

@chimp1984 chimp1984 closed this Aug 20, 2020
@chimp1984 chimp1984 deleted the add-xmr-tx-key-service branch August 24, 2020 03:30
@ghost ghost mentioned this pull request Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants