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] Initial version of MMS (Monero Messaging System) #4134

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@rbrunner7
Copy link
Contributor

commented Jul 14, 2018

This is the code of the MMS (Monero Messaging System) that I am currently working on to make Monero multisig easier.

It's clearly work in heavy progress, maybe 3/4 completed, 1/4 still to go. You probably won't have a chance getting this running already on your own to test it, mostly because of the fragility of the current code and missing documentation.

General background info about my MMS project is on Monero Taiga website here.

I make this WIP / DO NOT YET MERGE PR so that:

  • It's clear I am still on the way and making progress
  • Interested people can see where I propose to put the MMS code in the Monero source code tree and see the structure of the code
  • People can see my proposed naming
  • People can get an overall impression of the code size of the MMS
  • People can comment on my C++ / Monero code base beginner's programming style
@rbrunner7

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2018

Some noteworthy and interesting points about the implementation thay may help you understand the code and judge the consequences it may have if/when put into use:

  • A wallet gets one more file with an extension of .mms where the MMS stores the messages for that wallet. wallet2 is extended to know about that new file.
  • The new class message_store manages the messages in those files.
  • A wallet2 object owns a message_store object now.
  • The wallet files do not need a new structure version for the introduction of the MMS.
  • The message store must be able to get info about the current state of the wallet. To avoid cyclic or at least problematic imports wallet2 implements a new interface i_wallet_state_provider so that a message_store can call its owner to get the info without a need to use the wallet2 type.
  • The MMS uses viewkeys to encryt and sign messages, and Monero addresses for addressing, as described here.
  • The "original" address and viewkey of a wallet are used which it gets at creation, not the multisig address and any associated keys that it gets after turning it into a multisig wallet.
  • When "going multisig" wallet2 now stores that original address and viewkey as part of the JSON that goes into the keyfile, in order not to loose them and to continue using them for the MMS.
  • The MMS command set for the CLI wallet is the most mature part of this preview code and almost fully functioning. You can see it in action in this YouTube video.
  • Monero gets a new executable monero-messaging-daemon that implements a "reference" or "default" messaging daemon.
  • It's implemented in C++ and uses the Monero daemon framework classes just like the wallet RPC daemon.
  • That daemon uses pyBitmessage for actual message transport.
  • I apply for the next free port 18083 to use as the default port for messaging daemons.
@vtnerd
Copy link
Contributor

left a comment

I did a quick sweep. My biggest criticism is on the new RPC to a forwarding bridge. I would drop it. Just have the wallet write directly to BitMessage RPC.

The second criticism is that message_store needs better separation from wallet2. That callback can likely be removed, and the implementation will be better as a result.

}
else
{
const std::string filename = args[n];

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 17, 2018

Contributor

const std::string&

This comment has been minimized.

Copy link
@rbrunner7

rbrunner7 Jul 20, 2018

Author Contributor

Corrected.


m_wallet_state_provider = wallet_state_provider;
wallet_state state;
m_wallet_state_provider->get_wallet_state(state);

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 17, 2018

Contributor

This indicates strong coupling from message_store to wallet2, which shouldn't be necessary. wallet2 should be sending everything in the function calls into this class. In other words, drop this callback + virtual method interface.

This comment has been minimized.

Copy link
@rbrunner7

rbrunner7 Jul 20, 2018

Author Contributor

Still making up my mind regarding wallet2 dependency in general. See also my long-ish separate post about that subject below.

Note that it's not wallet2 calling into message_store, but the wallets do, in my case the CLI wallet. So if the message store cannot get info from the wallet directly and things have to happen over method parameters, simplewallet has to fetch it first from the wallet in order to feed it to the message store methods, which I found really tedious.

uint32_t message_store::member_index_by_transport_address(const std::string &transport_address) const
{
for (size_t i = 1; i < m_members.size(); ++i) {
const coalition_member &m = m_members[i];

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 17, 2018

Contributor

What happens if there are multiple uses of the transport address here? Just returns the first one? Is that correct?

This comment has been minimized.

Copy link
@rbrunner7

rbrunner7 Jul 20, 2018

Author Contributor

You are of course correct. That call is nonsense, as transport addresses are not unique, on the contrary, the system must be able to use a single transport address for any number of Monero addresses, e.g. for a public remote messaging daemon where you certainly don't want to use a separate transport address for each client that walks along, and for message transport systems where it's difficult or expensive to configure new addresses, unlike Bitmessage.


uint32_t message_store::get_message_index_by_id(uint32_t id)
{
for (size_t i = 0; i < m_messages.size(); ++i)

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 17, 2018

Contributor

How many messages is this expecting? Should this be sorted by ::id and then binary searched via std::lower_bound? The one downside is the moving of the struct during the initial insert call, but if there are lots of searching going on, its best to identify one search field, and then sort once by that field and binary search against it.

This comment has been minimized.

Copy link
@rbrunner7

rbrunner7 Jul 20, 2018

Author Contributor

Good question that I pondered myself also. IMO hard to say how many messages to expect in an average multisig wallet. Maybe a good idea to not optimize prematurely and see how that develops. I think internal binary search for message ids could be added at any time without breaking anything.

crypto::chacha20(ciphertext.data(), ciphertext.size(), chacha_key, iv, &plaintext[0]);
}

void message_store::send_message(uint32_t id)

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 17, 2018

Contributor

The send/add should be done in one call. It prevents the need to lookup a message that should already known.

This comment has been minimized.

Copy link
@rbrunner7

rbrunner7 Jul 20, 2018

Author Contributor

Well, "higher up" i.e. in simplewallet both cases are supported: Not immediately sending a message that was generated, and on the other hand re-sending an existing message because e.g. it was not transported properly. For those use cases separate "add" and "send" methods are needed.

@@ -1119,11 +1162,48 @@ bool simple_wallet::sign_multisig(const std::vector<std::string> &args)
uint32_t signers = 0;
try
{
bool r = m_wallet->sign_multisig_tx_from_file(filename, txids, [&](const tools::wallet2::multisig_tx_set &tx){ signers = tx.m_signers.size(); return accept_loaded_tx(tx); });
if (!r)
if (by_mms)

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 17, 2018

Contributor

I think this is matched by the } on line 1199. But it kind of hard to read with 2-space identations and a long if bracket. I think inverting the check - if (!by_mms) { ...stuff ... return true; } will make it more readable - I don't need an editor to bounce me to the other bracket.

}

std::string line = input_line(tr("Choice: "));
if (std::cin.eof() || line.empty())

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 17, 2018

Contributor

Why the eof() check here?

if (line.length() == 1)
{
char c = line[0];
if (isdigit(c) && (c != '0'))

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 17, 2018

Contributor

Just use atoi. The string is exactly one byte, so it cannot overflow an int on any platform. Also line 1477 will not work with some locales/character encodings.

for (size_t i = 0; i < messages.size(); ++i)
{
const tools::message &m = messages[i];
if (m.state == tools::message_state_ready_to_send)

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 17, 2018

Contributor

Is there some way to sort these, in case the queue gets large?

bool mn_ok = (mn.length() == 3) && (mn[1] == '/'); // do not support numbers > 9 for now
if (mn_ok)
{
char c = mn[0];

This comment has been minimized.

Copy link
@vtnerd

vtnerd Jul 17, 2018

Contributor

some more string conversions that can be done with other utilities.

@rbrunner7

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2018

Many thanks, @vtnerd, for this review of the code! I see there indeed a lot of valuable advice and hints. It will take time to go through all of it, which will probably happen over the course of several weeks, so there won't be immediate feedback or comments to all the points you rose. Anyway, I am aware that I operate at the very edge of my knowledge about C++ and Monero, and that I have much to learn still.

I am not sure whether this PR is a good place to discuss your 2 probably most important general points regarding the "architecture" of the whole system, the messaging daemon as a separate program and the separation of the MMS from the wallet2 class, but let us start and see where it leads. See the following 2 separate comments.

@rbrunner7

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2018

My biggest criticism is on the new RPC to a forwarding bridge. I would drop it. Just have the wallet write directly to BitMessage RPC.

The idea behind a "messaging daemon" as a separate program responsible for actual message transport is two-fold:

The first issue is indeed an attempt to offer a standard interface towards all possible clients that may want to send and receive messages plus achieving independence from the particular messaging system used.

One the one hand I think you may be right, maybe pyBitmessage will be everything that will ever be used, with a separate messaging daemon overkill then compared to direct calls into the Bitmessage client from message_store.

On the other hand one question that popped up immediately and several times in the Reddit threads that I made about the MMS, from people that did not look at the architecture in detail, was: "Can other messaging system be used as well?" I am frankly a little afraid to start an attempt to force the Monero community to use pyBitmessage, without a clean interface that allows replacement for people that don't "like" that program for one reason or another, even if that interface will only be there as a "pill" to calm minds.

There is however a second issue that I personally see as more important right now than the first one: I was thinking about smartphone wallets using the MMS, and people with little IT knowledge, people that now are gratefully using remote nodes because they see the task of operating their own node as too daunting.

Running pyBitmessage on a smartphone is probably difficult, and using pyBittmessage over the Internet isn't ideal neither: It's a nice small program designed to run locally, as I understand it.

So I imagined the solution: Remote messaging daemons, in the same style as remote nodes. Smartphones and "noobs" in this scenario would rely on remote messaging daemons that some helpful people operate.

@vespco opined that maybe over time the rather minimalistic pyBitmessage client would not suffice anymore, and that a messaging daemon should implement the Bitmessage protocol "natively". Again something easier with a messaging daemon as a separate program.

@rbrunner7

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2018

The second criticism is that message_store needs better separation from wallet2. That callback can likely be removed, and the implementation will be better as a result.

I was in fact starting at that point, without a callback interface from message_store "upward" into wallet2, doing everything over dead-simple parameters into the message_store methods.

But over time the parameter lists grew longer and longer, and I had to revise them frequently as the need for additional info became apparent over the course of implementing more functionality, until I reached the point where I said "Enough of that nonsense" ...

But more importantly, the thought to see independence from wallet2 as something positive and worthwhile to try to achieve never crossed my mind: I had seen myself how Monero's own CLI and GUI wallets revolve almost completely around wallet2, and I had contacted the teams behind all 3 published Monero smartphone wallets (Monerujo, Cake Wallet, X Wallet) that confirmed me: Yes, as the heart of the wallet, we also have wallet2 running on the smartphone.

A few days ago @naughtyfox made a PR for generalized N/M multisig. Where did they put the code necessary for that? Well, I thought when I saw the PR, "into wallet2 of course - where else?"

Maybe it would help greatly for this discussion if you describe where you come from, how "your" software transacts and calculates balances, and why you see wallet2 and message_store intertwined as problematic or at least as less than ideal. I think I have to really understand your choice of the word "need" in your statement "message_store needs better separation from wallet2".

char c = mn[0];
if (isdigit(c))
{
threshold = c - '0';

This comment has been minimized.

Copy link
@naughtyfox

naughtyfox Jul 18, 2018

Contributor

I wouldn't place low-level parsing code in high-level business logic function

uint32_t coalition_size;
const std::string &mn = args[1];
bool mn_ok = (mn.length() == 3) && (mn[1] == '/'); // do not support numbers > 9 for now
if (mn_ok)

This comment has been minimized.

Copy link
@naughtyfox

naughtyfox Jul 18, 2018

Contributor

It would be simpler to read and understand it if you placed negative scenario with exit before

@rbrunner7

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2018

@naughtyfox , thanks for the feedback. Can I trick you into commenting a little about the general architecture of the MMS somehow? Maybe interesting to compare with your wallet.


void handle_rpc_exception(const std::exception_ptr& e, epee::json_rpc::error& er, int default_error_code);

const boost::program_options::variables_map *m_vm;

This comment has been minimized.

Copy link
@naughtyfox

naughtyfox Jul 23, 2018

Contributor

I think there is no need to make it class member because it's used only in one function

std::string msgid;
std::string message;
std::string fromAddress;
std::string receivedTime;

This comment has been minimized.

Copy link
@naughtyfox

naughtyfox Jul 23, 2018

Contributor

I think it's better to make it time_t type if possible


uint32_t message_store::member_index_by_monero_address(const cryptonote::account_public_address &monero_address) const
{
for (size_t i = 1; i < m_members.size(); ++i) {

This comment has been minimized.

Copy link
@naughtyfox

naughtyfox Jul 23, 2018

Contributor

I would rather use std::find


bool message_store::any_message_of_type(message_type type, message_direction direction) const
{
for (size_t i = 0; i < m_messages.size(); ++i)

This comment has been minimized.

Copy link
@naughtyfox

naughtyfox Jul 23, 2018

Contributor

I would use std::find or std::find_if here. Don't think it's needed to be reimplemented

m_messages.clear();
}

void message_store::write_to_file(const std::string &filename) {

This comment has been minimized.

Copy link
@naughtyfox

naughtyfox Jul 23, 2018

Contributor

What about to not binding function interfaces to file system? It would give a lot of flexibility and would let cover much more code with unit tests. The same is true for wallet2 class...
For example, if we passed streams instead of file names we'd cover almost all of monero code with tests:

void message_store::write_to_file(const std::string& filename) {
  std::ostream file = . . .; // make ostream
  write_to_file(file);
}

void message_store::write(std::ostream& file) {
. . . 
}
@@ -162,7 +162,7 @@ struct options {
};
};

void do_prepare_file_names(const std::string& file_path, std::string& keys_file, std::string& wallet_file)
void do_prepare_file_names(const std::string& file_path, std::string& keys_file, std::string& wallet_file, std::string &mms_file)

This comment has been minimized.

Copy link
@naughtyfox

naughtyfox Jul 23, 2018

Contributor

I don't think it's good idea to make such intrusive dependency into wallet2 module

This comment has been minimized.

Copy link
@rbrunner7

rbrunner7 Jul 23, 2018

Author Contributor

Can you please elaborate on that? Is this a narrow and specific comment about this particular method and about changing its signature, or a quite general comment about the relationship between a message_store and a wallet2 object?

It has been quite difficult to get high-level feedback about this, but this is at the heart of my proposal: I propose to be the MMS to be a new integral and standard part of Monero and its wallets. After the introduction of the MMS, a Monero wallet has a key file, a wallet file, and optionally a message store file, and at least as I see it, it's allowed to be "open" about that, no need to hide something.

This comment has been minimized.

Copy link
@naughtyfox

naughtyfox Jul 23, 2018

Contributor

Is this a narrow and specific comment about this particular method and about changing its signature, or a quite general comment about the relationship between a message_store and a wallet2 object?

I think it's better to save some MMS related data by MMS module itself and make wallet2 know nothing about it. I read you had some issues with making the module as much independent from wallet2 as possible (that's why you introduced new interface). I think it's worth to think about making some methods in wallet2 to export data needed for MMS module instead of inheriting from the interface (because it's not kind-of relation). I haven't had a chance to dive in the architecture at the moment. Probably later I'll propose some more valuable suggestions.

This comment has been minimized.

Copy link
@naughtyfox

naughtyfox Jul 23, 2018

Contributor

I propose to be the MMS to be a new integral and standard part of Monero and its wallets.

It may be standard part of monero, but it's better to keep it separate from wallet2 class because, in my opinion, wallet2 has a lot of responsibilities already and it's not easy to test and keep free of bugs.

This comment has been minimized.

Copy link
@rbrunner7

rbrunner7 Jul 23, 2018

Author Contributor

The architecture currently is like that:

The functionality added to wallet2 is quite simple. The most important new job of a wallet2 object is managing a new third file beside the key file and the wallet file: the message store file. A wallet2 now "owns" a message store (file).

Which is, IMHO, useful: Because wallet2 manages the file all the programs that may use the MMS don't have to, no danger that the message store file "gets lost" or does not get renamed when a wallet is renamed etc.

Why should it have advantages if all wallet programs using wallet2 (I know about 5 such programs) have to babysit a message store beside it themselves when wallet2 could easily do that job once and for all its clients?

Most of the new functionality is 1) in the message_store class and 2) in any wallet program like in my case the CLI wallet that want to support the MMS.

If you don't use the MMS no message store file will be created - you won't notice anything, wallet2 will work as it ever has, with two files. Only by initializing the MMS a message store file is created. Technically, if you delete the file, nothing bad happens, the MMS is just not active anymore for that wallet.

@vtnerd

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2018

My biggest criticism is on the new RPC to a forwarding bridge. I would drop it. Just have the wallet write directly to BitMessage RPC.

The idea behind a "messaging daemon" as a separate program responsible for actual message transport is two-fold:

The first issue is indeed an attempt to offer a standard interface towards all possible clients that may want to send and receive messages plus achieving independence from the particular messaging system used.

Then the forwarding daemon should not be merged into this repo because it shouldn't have any dependencies on core code. Instead it would be put into a separate repo within the monero project.

One the one hand I think you may be right, maybe pyBitmessage will be everything that will ever be used, with a separate messaging daemon overkill then compared to direct calls into the Bitmessage client from message_store.

On the other hand one question that popped up immediately and several times in the Reddit threads that I made about the MMS, from people that did not look at the architecture in detail, was: "Can other messaging system be used as well?" I am frankly a little afraid to start an attempt to force the Monero community to use pyBitmessage, without a clean interface that allows replacement for people that don't "like" that program for one reason or another, even if that interface will only be there as a "pill" to calm minds.

I don't see a clear argument in this paragraph. Alternative messaging systems could be selected at runtime through a C++ virtual method interface without an additional RPC system in-between. Or a dynamic lib that wallets can link against that implements multiple schemes. Keeping in a separate process that doesn't require linking may have some advantage - but then the various wallets have to implement a proper client for the RPC anyway.

There is however a second issue that I personally see as more important right now than the first one: I was thinking about smartphone wallets using the MMS, and people with little IT knowledge, people that now are gratefully using remote nodes because they see the task of operating their own node as too daunting.

Running pyBitmessage on a smartphone is probably difficult, and using pyBittmessage over the Internet isn't ideal neither: It's a nice small program designed to run locally, as I understand it.

So I imagined the solution: Remote messaging daemons, in the same style as remote nodes. Smartphones and "noobs" in this scenario would rely on remote messaging daemons that some helpful people operate.

This is the most persuasive argument. And different wallets don't need specifics on multiple messaging layers. There is still something funky about this "mode" that I can't specify exactly right now. Need more time to think about it.

@vespco opined that maybe over time the rather minimalistic pyBitmessage client would not suffice anymore, and that a messaging daemon should implement the Bitmessage protocol "natively". Again something easier with a messaging daemon as a separate program.

Why couldn't this new BitMessage implementation use the same RPC scheme as pyBitmessage? Are you making a generic messaging RPC or an RPC specific for multi-signature messages? Because you haven't proposed the latter AFAIK. The message store is in wallet2, not in this daemon.

@vtnerd

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2018

I was in fact starting at that point, without a callback interface from message_store "upward" into wallet2, doing everything over dead-simple parameters into the message_store methods.

But over time the parameter lists grew longer and longer, and I had to revise them frequently as the need for additional info became apparent over the course of implementing more functionality, until I reached the point where I said "Enough of that nonsense" ...

This suggests that some state information in wallet2 may be better off in another object that wallet2 and message store use via std::shared_ptr. If possible, the message store would have a const instance, letting wallet2 (or any other implementation) change the state as necessary.

I will say that the current wallet2 does not make modifications easy, so I am sympathetic.

A few days ago @naughtyfox made a PR for generalized N/M multisig. Where did they put the code necessary for that? Well, I thought when I saw the PR, "into wallet2 of course - where else?"

This does not mean that either design is the best approach.

Maybe it would help greatly for this discussion if you describe where you come from, how "your" software transacts and calculates balances, and why you see wallet2 and message_store intertwined as problematic or at least as less than ideal. I think I have to really understand your choice of the word "need" in your statement "message_store needs better separation from wallet2".

I would tell you to redesign the interactions between wallet2 and the message store even if 100% of the wallet implementations used wallet2. This design is a defacto circular dependency between the two, and at a glance this does not appear to be necessary. The biggest benefit to breaking down the abstractions into these parts is that the individual parts can be unit tested. Currently, if it goes into wallet2 directly, it cannot be easily tested. Tests for the message store might be possible, but you have to provide a "fake" wallet2 implementation for the callback. An even better approach would be to use the "real" stateful object that both share, and manipulate the shared state with the same functions that wallet2 would use. This reduces the risk of different behavior in the "reimplementation" of the wallet2 interface.

As far as non-wallet2 implementations go - mymonero has always used a different implementation since it was historically web-based. It simply isn't possible to cross-compile that into Javascript. The more wallet code that is purely functional (or small stateful objects) will allow for more re-use in different environments. The big one that comes to mind is the default Trezor wallet, which is likely to be problematic for that system (which arguable leaks tons of metadata due to its design - but its hard to convince privacy over convenience sometimes).

@rbrunner7

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2018

I would tell you to redesign the interactions between wallet2 and the message store even if 100% of the wallet implementations used wallet2. This design is a defacto circular dependency between the two, and at a glance this does not appear to be necessary.

Convinced. I will go back to the "pass wallet state info through method parameters" approach - tedious, as I argued, but I agree with you now the "lesser evil".

An even better approach would be to use the "real" stateful object that both share, and manipulate the shared state with the same functions that wallet2 would use.

In theory a good idea, but one that I see running into cumbersome practical difficulties as soon as you look at the details. Take the number of current transfers that the wallet holds and that the message store uses to coordinate wallet re-syncs: That's simply the current size of an array that the wallet holds. To ask the wallet to update a separate value everytime it changes in some additional object that the wallet and the message store share looks like a pain to me ...

@rbrunner7

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2018

There is still something funky about this "mode" that I can't specify exactly right now. Need more time to think about it.

For me it's quite interesting, somewhat fascinating even, how you don't seem to buy the "daemon approach", whereas for me that approach is so natural, gives so much flexibility with little effort and achieves a clean separation and division of tasks in a simple way. Maybe with your experience at my disposition I would immediately share your concerns, but now I am mostly at a loss.

If we try to go to the bottom of it, what "type" of argument dominates here for you?

  • Unnecessary complexity, bad cost/benefit ratio
  • Wrong/unsuitable architecture to solve the job at hand
  • Doesn't harmonize with the architecture of the already existing Monero software
  • Security considerations
  • A mix of the above
  • Something else altogether

I wish more Monero dev's would chime in with their opinions and viewpoints in a similar way like you, but I only have to watch myself merely glancing over PR's without commenting, to know why this might not happen. That's why I do appreciate your comments here.

@rbrunner7

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2018

The first issue is indeed an attempt to offer a standard interface towards all possible clients that may want to send and receive messages plus achieving independence from the particular messaging system used.

Then the forwarding daemon should not be merged into this repo because it shouldn't have any dependencies on core code. Instead it would be put into a separate repo within the monero project.

I don't fully understand what you mean here.

I currently implement the messaging daemon by extensively using what you probably mean with "core code", because I use the epee / Monero daemon classes. In this respect it's rather similar to the monerod and monero-wallet-rpc programs. And why not, remember, it's my aim to blend in, to become an integral, standard part of the Monero software.

And about that separate repo within the Monero project: That was my very first thought when I started with implementation, but then I feared that this could get complicated, because of needing all those Monero classes. I also saw how small this all will get - the messaging daemon is only three source files.

And finally I mused: If I do my job well, and people accept my work, why should "my" messaging daemon not be allowed in as the "baseline" messaging daemon, and part of Monero. After all, simplewallet also has its place there right in the middle of the Monero repo, being something like the "baseline" wallet.

I know, I know, maybe overly ambitious, especially for a newcomer like me, but on the other hand I am really convinced that people should have a default messaging daemon ready to use as soon as they installed Monero.

@vtnerd

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2018

There is still something funky about this "mode" that I can't specify exactly right now. Need more time to think about it.

For me it's quite interesting, somewhat fascinating even, how you don't seem to buy the "daemon approach", whereas for me that approach is so natural, gives so much flexibility with little effort and achieves a clean separation and division of tasks in a simple way. Maybe with your experience at my disposition I would immediately share your concerns, but now I am mostly at a loss.

There is enough work specifying how a potential Monero communication system will work on a single messaging system. How a Monero address (view key prsumably) can be used as the identity on another network. Or should there be no association? A specification/algorithm for the multi-sig messaging. Thinking about communication for BTC scriptless-script atomic swaps (a more complex two chain multisg). Probably other stuff too if I thought about it some more.

Right now the message store could use the BitMessage API directly, and be modified for a unified API (if determined useful) at some later point with minimal work. Or if not BitMessage, some other system.

If we try to go to the bottom of it, what "type" of argument dominates here for you?

  • Unnecessary complexity, bad cost/benefit ratio
  • Wrong/unsuitable architecture to solve the job at hand
  • Doesn't harmonize with the architecture of the already existing Monero software
  • Security considerations
  • A mix of the above
  • Something else altogether

If the primary objective is to provide a mechanism for community members to run open messaging daemons, what happens when a particular messaging daemon only supports 1 / 3 messaging systems? Just keeps trying other daemons? Also, coordination between the parties is easier if they don't have to negotiate / know in advance the messaging layer that will be in use. My first thought would be that Monero should support a specific messaging system (thereby making the messaging daemon unnecessary).

Also, just random thoughts:

  • The monero blockchain can be used as a messaging system, with some possible privacy concerns (output history). There might be ways to mitigate this. This system would be a convenient method because it doesn't require a second p2p system, and Monero addresses work natively.
  • Monero addresses could be used as an identity for async messages or real-time chat over Kovri. BitMessage only supports the one mode.
@vtnerd

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2018

The first issue is indeed an attempt to offer a standard interface towards all possible clients that may want to send and receive messages plus achieving independence from the particular messaging system used.

Then the forwarding daemon should not be merged into this repo because it shouldn't have any dependencies on core code. Instead it would be put into a separate repo within the monero project.

I don't fully understand what you mean here.

I currently implement the messaging daemon by extensively using what you probably mean with "core code", because I use the epee / Monero daemon classes. In this respect it's rather similar to the monerod and monero-wallet-rpc programs. And why not, remember, it's my aim to blend in, to become an integral, standard part of the Monero software.

The messaging daemon uses HTTP, JSON, and XML. None of this is a hard-requirement on Monero internals. The epee code is the closest to being fully "exportable" for use in other projects if that HTTP server is the most useful, although I'm not sure on the difficulty of exporting that code. This code could also be written in some other language, but may not be well received by Monero community.

And about that separate repo within the Monero project: That was my very first thought when I started with implementation, but then I feared that this could get complicated, because of needing all those Monero classes. I also saw how small this all will get - the messaging daemon is only three source files.

And finally I mused: If I do my job well, and people accept my work, why should "my" messaging daemon not be allowed in as the "baseline" messaging daemon, and part of Monero. After all, simplewallet also has its place there right in the middle of the Monero repo, being something like the "baseline" wallet.

This is about logistics, and about the messaging daemon attempting to solve a problem separate from the core Monero code. The problem the daemon is addressing is even separate from the messaging core - which should arguably go into Monero since multi-sig or atomic swap stuff is useful when sending/receiving Monero. This messaging daemon, in theory, would be useful to anyone needing to communicate asychronously over varied messaging systems.

As an example, I would have preferred the light wallet server go into a separate repo, but exporting cryptonote_basic and cryptonote_core will be fairly difficult right now. And there is no other alternative tools to replace them (they aren't standards like HTTP, JSON, and XML).

I know, I know, maybe overly ambitious, especially for a newcomer like me, but on the other hand I am really convinced that people should have a default messaging daemon ready to use as soon as they installed Monero.

The repo the project is placed in, is separate from whether it gets installed by default. The GUI can and is downloaded in a single download, even though it is not in the primary repo.

@rbrunner7

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2018

Thanks, @vtnerd , you have given me much food for thought. I will comment on some specific points of your two new posts later.

Now I am first and foremost just curious about something: You in my shoes, if you had taken on this project and arrived at the opinion that a direct interface from the message store to BitMessage is the way to go, how would you proceed from the point that I am now:

Would you a) just implement it that way, make a PR and see what the people comment, or b) seek discussion elsewhere first, e.g. by opening a dedicated "discussion PR" or by bringing the subject up at a developer meeting?

@vtnerd

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2018

Now I am first and foremost just curious about something: You in my shoes, if you had taken on this project and arrived at the opinion that a direct interface from the message store to BitMessage is the way to go, how would you proceed from the point that I am now:

Would you a) just implement it that way, make a PR and see what the people comment, or b) seek discussion elsewhere first, e.g. by opening a dedicated "discussion PR" or by bringing the subject up at a developer meeting?

EDIT: Some clarification - the messages between Monero addresses seems like it should be a RPC type system. For instance, if someone wants to reject a request for multisig, I think they have to ignore the message. The requesting person just sees nothing happen - that seems odd. Or did I miss the error path from these requests? So markdown document 1 : how does the communication between Monero addresses work? Markdown document 2 : How do the messages/rpc between addresses get sent?

I would start with a markdown file to describe the RPC schema / format between Monero addresses, and listing the rationale for any non-obvious decisions. At least I know intended behavior to compare against the code. Some thoughts that should go into that file:

  • Do not use the portable binary archive. That is likely to be more frustrating long-term. Describe this in terms of JSON - it can be message pack in the end version for performance. This will be more sane.
  • Define a high-level format for the requests/responses. I recommend JSON-RPC
    • How does recipient determine the action for the message (the "method" field in JSON-RPC).
    • How are errors returned? "Not implemented" or "not possible" type errors - because if it doesn't even understand the message type, what should it do? (the "error" field in JSON-RPC).
      • Error responses is the unclear thing to me in the current format defined by the code
    • How does a request and response get paired up? It is somewhat implicit I think in the current system, but I'm not sure if always works with the wallet2 multisig format (correct me if wrong here).
  • Specify the RPC requests/responses for multisig. Is it whatever wallet2 does? What the heck does that do anyway? What is that encoding scheme? Maybe we are screwed here "cryptonote special" or "portable binary archive", but should support something more sane if possible.
  • Taiga suggested it was encrypted to the monero-address. How? saltpack could be useful, NaCL is used for encryption so spend pub -> curve25519 is possible. This also allows broadcasting one request to a group (i.e. I went to sent this one chat message to 3 people). The small advantage of saltpack is that we don't have to define our own format for multi-party messages, and other tools could decrypt the data if necessary. Or give justification to another scheme.

Then the messaging layer. Why not Kovri? Did I miss this on Taiga? There are going to be several benefits: it can do real-time calls to a Monero address, monerod and the light wallet servers are likely already going to be communicating over this network, and the messages can be broadcast-to-all or directed for efficiency.

Then the messaging layer keys - the primary justification seems to be that the mutlsig changes the address used. I don't see this as a justification for a new key - the wallet could/should track that monero address X is associated with multisig address Y. Am I missing something?

Lastly, the messaging daemon. It does make sense for moneroju and mymonero type of wallets (although slightly different). I think what consistently bothered me is why support multiple message layers? If Kovri were used, the API could be merged into some combination of monerod, monero-light-wallet-server/openmonero, and kovrid. In other words, the messaging daemon would just be Kovri. That project is already looking to define a communication mechanism from monerod, and including "route this message to this Monero user" seems like the kind of API call that Kovri should support.

@rbrunner7

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

Also, just random thoughts:

The monero blockchain can be used as a messaging system, with some possible privacy concerns (output history). There might be ways to mitigate this. This system would be a convenient method because it doesn't require a second p2p system, and Monero addresses work natively.

That was the starting point of my long journey towards a working MMS half a year ago or so, and I have written extensively about this approach on my project Taiga wiki.

The long story short: While it's of course trivial to add reasonably-sized messages as extra tx data, using normal Monero transctions for multisig messaging quickly gets uncomfortably expensive - even for a single multisig transaction several messages have to be passed around. And if you avoid that and introduce messages as something new that Monero daemons transport, in addition to blocks and transactions, you have a problem of potential spam on your hand that is very difficult and complicated to solve.

@rbrunner7

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

Then the messaging layer. Why not Kovri? Did I miss this on Taiga?

I just quote this sentence out of much that you wrote that brought a smile on my face when I finished reading and let me think of a saying in German: Lieber einen Spatz in der Hand als eine Taube auf dem Dach. (Linguee gives me as the corresponding English saying: A bird in the hand is worth two in the bush.)

What I mean: You aim very, very high.

Basically you sketch a technical solution to the problem that I tackled (easy Monero multisig) that seems solid to me - but also a about magnitude more difficult to realize, at least for me, and over a much longer term that I aspire to at the moment. Even after thinking much about it, I am convinced there could be room for something much simpler, much faster, that is reasonably sound technologically and still useful. I believe that with a little bit of luck I can get a first simple version of the MMS into the spring hardfork.

Kovri, I mean, look, that just has its first alpha version out now. It might take a year for first final release version to appear and then on top of that another half year until it gets fully accepted by the Monero community and sees widespread use.

But nevertheless from what you wrote I have slowly come around to your point of view: In a world with widespread Kovri use my "messaging daemon" as designed now quickly becomes legacy, not something to carry along longterm and develop further. With Kovri for "easy multisig" something has to be done to avoid a hard "must be online" requirement, but that problem is not that hard to solve.

That's why I will heed your advice and build the code to interface with Bitmessage directly into the message store. Much simpler, much more direct, no more messaging daemon, but works.

On a second track I will see what can be done with that pesky "smartphone wallet problem". I already checked that public PyBitmessage instances directly facing the Internet without any additional security measures are indeed, as I feared already, very dangerous - everybody and their dog can listen a moment on the cleartext HTTP going to it to sniff username and password and then just for fun delete all messages every 10 seconds.

But first it may be possible to set up a web server as a proxy to protect it, second PyBitmessage is open source so API access could be hardened, third who says that any of the smartphone wallets is really hungry for multisig?

@rbrunner7

This comment has been minimized.

Copy link
Contributor Author

commented Nov 4, 2018

I close this PR as it's outdated and will soon be superseeded by a new PR for a much further developed version of the MMS, influenced in many ways by the comments here in this PR. Thanks again for reviewing and commenting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.