Allow wallet2.h to run in WebAssembly. #6332

woodser · 2020-02-11T17:45:17Z

Add contrib/epee/include/net/abstract_http_client.h which
http_client.h extends.
Replace simple_http_client with abstract_http_client in wallet2,
message_store, message_transporter, and node_rpc_proxy.
Skip code incompatible with webassembly using #if defined __EMSCRIPTEN__ directives.
Add ability to import and export wallet file buffers in wallet2
instead of using file system.

Code and collaboration credits: moneromooo, endogenic, mymonero

Feedback is requested on removing the call to miner::find_nonce_for_given_block in cryptonote_tx_utils.cpp and changing the default http method from GET to POST in wallet2.h and http_abstract_invoke.h (otherwise the browser issues GET requests which fail).

selsta · 2020-02-12T12:42:15Z

@woodser Functional tests failed.

woodser · 2020-02-13T17:35:59Z

Anyone know why test-ubuntu is not passing or how I can find more detail about the error?

Start  5: functional_tests_rpc
5/21 Test  #5: functional_tests_rpc .............***Failed  433.23 sec

selsta · 2020-02-13T22:25:16Z

make
cd build/release/tests/functional_tests
ctest --output-on-failure

src/wallet/wallet2.cpp

src/wallet/wallet2.h

src/wallet/wallet2.cpp

woodser · 2020-02-16T16:44:24Z

With the changes in this PR, the tests are failing with:

AssertionError: {u'jsonrpc': u'2.0', u'id': u'0', u'error': {u'message': u'no connection to daemon', u'code': -38}}

moneromooo-monero

I've only gone quick over it so far, I'll read again once the massive pointer/ref spam is gone.

moneromooo-monero · 2020-02-20T13:44:54Z

contrib/epee/src/mlocker.cpp

@@ -160,18 +181,30 @@ namespace epee

  size_t mlocker::get_num_locked_pages()
  {
+#if defined __EMSCRIPTEN__
+    return 0;
+#endif


Why add all of these when you can just not define HAVE_MLOCK ? Is this somehow not working ?

It is somehow not working (i.e. something is calling one of the mlocker class functions). I will look into it further.

mlocker functions are being called by using the mlocked struct like here in wallet2::setup_keys(), so currently, individual functions in mlocker need guarded.

I don't understand the implication. Can you explain ?

Execution in WebAssembly will fail on CRITICAL_REGION_LOCAL(mutex()) in mlocker.cpp even if HAVE_MLOCK is not defined because the mlocked struct is used externally without checking for this definition. The current checks in question prevent that. Do you prefer another way?

contrib/epee/include/net/http_client.h

moneromooo-monero · 2020-02-20T13:45:59Z

contrib/epee/include/storages/http_abstract_invoke.h

    {
      epee::json_rpc::error error_struct;
      return invoke_http_json_rpc(uri, method_name, out_struct, result_struct, error_struct, transport, timeout, http_method, req_id);
    }

    template<class t_command, class t_transport>
-    bool invoke_http_json_rpc(const boost::string_ref uri, typename t_command::request& out_struct, typename t_command::response& result_struct, t_transport& transport, std::chrono::milliseconds timeout = std::chrono::seconds(15), const boost::string_ref http_method = "GET", const std::string& req_id = "0")
+    bool invoke_http_json_rpc(const boost::string_ref uri, typename t_command::request& out_struct, typename t_command::response& result_struct, t_transport& transport, std::chrono::milliseconds timeout = std::chrono::seconds(15), const boost::string_ref http_method = "POST", const std::string& req_id = "0")


Can you explain why the switch from GET to POST ? It is probably a good idea in theory since they can have side effects, but are there actual reasons beyond this ?

The browser issues GET requests which fail unless these default to POST. I was not able to track how GET works at all or where GET is overriden with POST in monero-project, as the RPC requests require POST.

src/cryptonote_core/cryptonote_tx_utils.cpp

moneromooo-monero · 2020-02-20T13:48:41Z

src/wallet/wallet2.cpp

@@ -1313,7 +1314,7 @@ bool wallet2::set_daemon(std::string daemon_address, boost::optional<epee::net_u
  }

  MINFO("setting daemon to " << get_daemon_address());
-  return m_http_client.set_server(get_daemon_address(), get_daemon_login(), std::move(ssl_options));
+  return m_http_client->set_server(get_daemon_address(), get_daemon_login(), std::move(ssl_options));
 }


All this massive spam all over the file should be avoided. You can probably do so easily by making m_http_client a reference, initialized in the ctor.

Making m_http_client a reference will require it to be initialized in the constructor. Since abstract_http_client is abstract, the default value could be an instance of http_simple_client, but that cannot be initialized in emscripten, so I do not have a suitable default value in that environment and execution fails.

I have updated the PR to use a reference which compiles but does not execute in emscripten because initializing http_simple_client in that environment fails.

Please advise. Thanks.

Off the top of my head, you can add a simple object, like:

struct redir {
redir(http_client *c): C(c) {}
operator http_client &() { return *c; }
http_client *C;
};

Then replace http_client *m_http_client with redir m_http_client in wallet2.h.
That should work AFAICT. Add default ctor if needed, and catch NULL if someone uses it before set.

Two thoughts:

Use a std::unique_ptr instead of a raw pointer to clearly designate ownership. Unless std::shared_ptr behavior was necessary (it doesn't appear so at a glance).

Adding * and -> stuff isn't really that hard to read, and we're now adding code just to reduce total line changes ... just why?

To avoid all the diff spam changing lines that don't actually change.

Seems like poor choice of words on my part - I know why you suggested it. But creating a class to prevent simple line changes seems incorrect. Especially since it hides pointer semantics - the value can be NULL but this is hidden at every usage point with an "auto" dereference.

Not sure if you checked as the current diff does not show them anymore, but it changed every single RPC call for example. It was all over the place in wallet2.cpp.

I also think it should be based on what is most correct and natural over these line diffs. I've updated the PR to work with std::unique_ptr until moneromooo-monero's suggestion can be implemented for comparison.

moneromooo-monero · 2020-02-20T13:50:28Z

src/wallet/wallet2.cpp

+    return false;
+  }
+
+  return true;


I've not read this yet, but why is the keys file code changed in this PR ?

The key file code is changed in order to directly import and export wallet data to and from wallet2. Otherwise there is no way to save and re-open a wallet in WebAssembly because the file system is not accessible.

vtnerd

I didn't dig into the wallet2 changes too closely, hopefully I can scan through those later.

contrib/epee/include/net/abstract_http_client.h

vtnerd · 2020-02-22T21:45:55Z

contrib/epee/include/net/http_client.h

 	namespace http
 	{

 		template<typename net_client_type>
-		class http_simple_client_template: public i_target_handler
+    class http_simple_client_template: public i_target_handler, public abstract_http_client


Mark it as class http_simple_client_template final : .... This prevents anyone from overriding this class, and allows the compiler to de-virtualize all this-> virtual method calls within this class.

epee::net_utils::http::http_simple_client is extended by download_client in download.cpp so adding final causes a compile error. Please advise. Thanks.

monero/src/common/download.cpp

Line 94 in 39e1890

class download_client: public epee::net_utils::http::http_simple_client

contrib/epee/include/net/abstract_http_client.h

moneromooo-monero · 2020-03-01T02:02:13Z

contrib/epee/src/abstract_http_client.cpp

+      csRet += "%";
+      csRet += dec_to_hex(val, 16);
+      return  csRet;
+    }


Not related to your change, but this does two allocs per byte... Just noting it so it can be changed later if it turns out to be a bottleneck somewhere.

moneromooo-monero · 2020-03-01T02:03:26Z

contrib/epee/src/mlocker.cpp

@@ -160,18 +181,30 @@ namespace epee

  size_t mlocker::get_num_locked_pages()
  {
+#if defined __EMSCRIPTEN__
+    return 0;
+#endif


I don't understand the implication. Can you explain ?

moneromooo-monero · 2020-03-01T02:12:38Z

src/wallet/wallet2.cpp

+
+  std::stringstream oss;
+  boost::archive::portable_binary_oarchive ar(oss);
+  ar << *this;


That's throwy for a function that returns a bool.

I made get_cache_file_data() return boost::optional<wallet2::cache_file_data> to be consistent with get_keys_file_data(), and an exception is thrown if boost::none is returned.

moneromooo-monero · 2020-03-07T17:57:24Z

I've just noticed one of my comments was on the wrong context.

My comment "I don't understand the implication. Can you explain ?" was intended to point to:

Execution in WebAssembly will fail on CRITICAL_REGION_LOCAL(mutex()) in mlocker.cpp even
if HAVE_MLOCK is not defined because the mlocked struct is used externally without checking
for this definition. The current checks in question prevent that. Do you prefer another way?

woodser · 2020-03-07T18:30:31Z

I believe that comment was my response to your question. The implication is execution of CRITICAL_REGION_LOCAL(mutex()) needs to be prevented in order to run in WebAssembly. It is currently done with the #if defined checks, but I'm open to better suggestions. Maybe I don't understand your question?

moneromooo-monero · 2020-03-07T22:04:01Z

Why does "the mlocked struct is used externally without checking
for this definition" imply "execution of CRITICAL_REGION_LOCAL(mutex()) needs to be prevented" ?

woodser · 2020-03-07T22:46:44Z

The former does not imply the latter. In response to the original question, "Why add all of these when you can just not define HAVE_MLOCK ? Is this somehow not working ?", that does not work because the mlocked struct is used externally without checking the HAVE_MLOCK definition. Therefore, without the added #if defined checks, CRITICAL_REGION_LOCAL(mutex()) will execute and crash in WebAssembly, which needs to be prevented.

moneromooo-monero · 2020-03-08T16:48:01Z

Why does CRITICAL_REGION_LOCAL(mutex()) crash ?

woodser · 2020-03-11T01:39:15Z

It was a consequence of calling boost::this_thread::sleep_for in the CRITICAL_REGION_LOCAL definition ~~when pthreads are not enabled in emscripten (such as for browsers that do not support them)~~ (correction: multithread apis work without pthreads). I made an update to define CRITICAL_REGION_LOCAL with an empty block in this case.

vtnerd · 2020-03-11T03:26:15Z

It was a consequence of calling boost::this_thread::sleep_for in the CRITICAL_REGION_LOCAL definition when pthreads are not enabled in emscripten (such as for browsers that do not support them). I made an update to define CRITICAL_REGION_LOCAL with an empty block in this case.

Is the target always single-thread in that case?

woodser · 2020-03-11T04:47:14Z

Yes.

moneromooo-monero · 2020-03-11T14:21:18Z

Plenty of monero code uses threads. Those don't crash ?

moneromooo-monero · 2020-03-27T15:39:47Z

contrib/epee/include/syncobj.h

+#define  CRITICAL_REGION_LOCAL(x) {} epee::critical_region_t<decltype(x)>   critical_region_var(x) // TODO: calling sleep_for here causes loading wasm module to hang at full utilization
+#else
+#define  CRITICAL_REGION_LOCAL(x) {boost::this_thread::sleep_for(boost::chrono::milliseconds(epee::debug::g_test_dbg_lock_sleep()));} epee::critical_region_t<decltype(x)>   critical_region_var(x)
+#endif


I think you can remove the sleep altogether, I intended to do this for a while.

moneromooo-monero · 2020-03-27T15:42:04Z

contrib/epee/src/net_ssl.cpp

+    SSL_CTX_set_cert_store(ctx, store);
+}
+#endif
+


That's existing code. Are you adding a copy for some reason ?

moneromooo-monero · 2020-03-27T15:43:39Z

src/cryptonote_basic/miner.cpp

@@ -476,7 +476,7 @@ namespace cryptonote
    for(; bl.nonce != std::numeric_limits<uint32_t>::max(); bl.nonce++)
    {
      crypto::hash h;
-      gbh(bl, height, tools::get_max_concurrency(), h);
+      gbh(bl, height, diffic <= 100 ? 0 : tools::get_max_concurrency(), h);


Also existing code. You probably messed up some rebase somewhere.

moneromooo-monero · 2020-03-27T15:45:13Z

src/cryptonote_core/cryptonote_tx_utils.cpp

+
+    #if defined __EMSCRIPTEN__  // miner not accessible in webassembly
+      return false;
+    #endif


If you can't generate the genesis block, you can't even start the wallet2 hash chain. There is a C implementation of Cryptonight. Does it not work ?

The wallet2 instances created through webassembly generally work; they're able to get the balance, send, receive, etc. Is there a problem I can expect to see?

Can you elaborate how to use the C implementation of Cryptonight here?

They'll get a wrong genesis block hash. I guess it might not matter in practice for the wallet only...

To use the C version of Cryptonight, you ensure the C version is compiled in slow-hash.c. It should be the case if you define NO_AES, looking at the code.

That worked. I've removed this change.

xiphon · 2020-04-05T02:59:45Z

src/wallet/wallet2.cpp

@@ -1183,7 +1184,7 @@ wallet2::wallet2(network_type nettype, uint64_t kdf_rounds, bool unattended):
  m_light_wallet_balance(0),
  m_light_wallet_unlocked_balance(0),
  m_original_keys_available(false),
-  m_message_store(),
+  m_message_store(std::unique_ptr<epee::net_utils::http::abstract_http_client>(m_http_client->clone())),


Consider to use std::shared_ptr instead

A shared pointer would introduce a functional change from current master, which uses a separate http client instance for each class. The functional tests fail when changed to a shared pointer.

I don't like virtual abstract_http_client* clone() const = 0; idea.

The message store needs to be initialized with a http client of the same type as wallet2, which the clone() achieves. Do you suggest any different or better approach?

By the way, a shared_ptr was my first choice as well.

I don't understand why switching to a shared_ptr instead of unique_ptr would break (I don't care which one is used otherwise). Can you expand on this a bit ?

Clone implies we are copying some existing instance state. But actually the message store just needs a new http client instead (not a clone of an existing one).

Off the top of my head, you could pass http client factory to wallet2 and message store.

In the simplest case it could be just a callback returning new std::unique_ptr<abstract_http_client> instance.

Ah, i didn't see your guys comments before posting my reply.

Ok I added a factory and removed the clone(). I can make name and styling changes if there are any suggestions.

contrib/epee/include/syncobj.h

contrib/epee/src/abstract_http_client.cpp

src/wallet/message_store.h

src/wallet/wallet2.cpp

xiphon · 2020-04-12T12:50:47Z

src/wallet/wallet2.cpp

+  {
+    epee::net_utils::http::abstract_http_client* abstract_http_client = m_http_client.get();
+    epee::net_utils::http::http_simple_client* http_simple_client = dynamic_cast<epee::net_utils::http::http_simple_client*>(abstract_http_client);
+    CHECK_AND_ASSERT_MES(http_simple_client != nullptr, false, "http_simple_client must be used to set proxy");
+    http_simple_client->set_connector(net::socks::connector{std::move(proxy)});
+  }


I would avoid dynamic_cast. On the other hand, if others are OK with it, i could fix this later in a separate PR.

src/wallet/message_transporter.h

- Add abstract_http_client.h which http_client.h extends. - Replace simple_http_client with abstract_http_client in wallet2, message_store, message_transporter, and node_rpc_proxy. - Import and export wallet data in wallet2. - Use #if defined __EMSCRIPTEN__ directives to skip incompatible code.

esarachan · 2020-04-16T03:16:52Z

contrib/epee/include/net/abstract_http_client.h

+
+namespace epee
+{
+namespace net_utils


NIT: nested namespaces should probably be indented.

See #6332 (comment)

I don’t think this should get changed again.

woodser force-pushed the wasm_pr_4 branch 6 times, most recently from 31e43fb to 624c9cc Compare February 11, 2020 19:37

woodser force-pushed the wasm_pr_4 branch from 624c9cc to ec94ce0 Compare February 13, 2020 03:03

selsta mentioned this pull request Feb 13, 2020

easylogging++: add emscripten support #6209

Merged

xiphon reviewed Feb 16, 2020

View reviewed changes

src/wallet/wallet2.cpp Outdated Show resolved Hide resolved

xiphon reviewed Feb 16, 2020

View reviewed changes

src/wallet/wallet2.h Outdated Show resolved Hide resolved

xiphon reviewed Feb 16, 2020

View reviewed changes

src/wallet/wallet2.cpp Outdated Show resolved Hide resolved

xiphon reviewed Feb 16, 2020

View reviewed changes

src/wallet/wallet2.cpp Outdated Show resolved Hide resolved

moneromooo-monero reviewed Feb 20, 2020

View reviewed changes

vtnerd reviewed Feb 22, 2020

View reviewed changes

vtnerd reviewed Feb 23, 2020

View reviewed changes

contrib/epee/include/net/abstract_http_client.h Show resolved Hide resolved

moneromooo-monero reviewed Mar 1, 2020

View reviewed changes

sumogr mentioned this pull request Mar 4, 2020

[easylogging++] add emscripten support sumoprojects/sumokoin#736

Merged

woodser force-pushed the wasm_pr_4 branch 4 times, most recently from b8801e4 to 7d01980 Compare March 27, 2020 03:08

moneromooo-monero reviewed Mar 27, 2020

View reviewed changes

woodser force-pushed the wasm_pr_4 branch 3 times, most recently from c255029 to 4bedc21 Compare April 1, 2020 19:38

xiphon reviewed Apr 5, 2020

View reviewed changes

woodser force-pushed the wasm_pr_4 branch 3 times, most recently from f633c8b to 7a77f01 Compare April 6, 2020 21:30

moneromooo-monero approved these changes Apr 10, 2020

View reviewed changes