-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[Do Not Merge] Adding "light wallet server" that is mymonero compatible #4139
Conversation
Unfortunately, "cryptonote_core", "ringct", and "wallet" | ||
libraries all depend on the device library creating a cyclic | ||
dependency. None of those libraries should be dependent on | ||
the hardware abstraction, it should only be dependent on them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of those libraries should be dependent on the hardware abstraction, it should only be dependent on them.
👍
@vtnerd First off, this is awesome, and I solute you for making the effort to get a lot of the magic running MyMonero out there into the wider Monero community. I wanted to jump in and see if you could give a status update on this PR.
What are you thinking in terms of getting the first partial piece of the changes here into a mergeable PR? Is this something anyone else can help with beyond review? FFS? 😁 |
Aha, I just found monero-project/meta#272, which answers a lot of my own questions. Hopefully that's also useful for onlookers. Edit: Oh, and even more at #4460. Onlookers are totally set now :P |
38681c0
to
aa2da7a
Compare
Force pushed an update:
|
Also, I wanted to mention that the DB is unlikely to change, unless a reviewer notices something serious. So anyone wishing to run/test this server is encouraged to do so, because migration for DB changes will be provided (upon request only pre-merge). |
@dternyak @anonimal What's the current plan to merge this or timeline to add light wallet API support? The specification is almost finished. Light wallet does not work anymore after |
I don't think either are responsible for this being merged. One of the dependent reviews (LMDB util stuff) is currently held up in review. There are likely 1-2 other PRs before I remove the
This PR/patch should work with a recent |
How much work would it be to make this into its own repo, similar to how the GUI does ? It seems very self contained (ie,mostly new files in new directories). |
This uses lots of internal headers/functions. So either use the entire source tree to grab internal libraries (like openmonero) or attempt a library type stub here that can be linked against. Both are pretty rough. Better than a library stub specific for this app might be the ominous "librarize" step that was once on the Monero roadmap, but that is even more work. So I would have to investigate a design for a library stub specific for this task and then decide whether to do this or the internal method that openmonero uses. I'd probably go with the openmonero approach and deal with the constant breaks from the parent repo. Its likely difficult to make a library without being able to use |
FWIW, if the code is in the monero tree, there will be as many constant breaks, except I will get to be the one to fix it. |
Fixing the breaks is hopefully done by the person making the breaking change. Often times the issue isn't that some interface changed and needs to be updated, but that the entire behavior changed and the logic is wrapped so tightly into other parts of the codebase that its difficult to re-use or even copy-and-paste the code. Wallet2 output selection is a great example - much of that code could/should be in a separate functions in a header that is used by Wallet2. I started some re-usability with this patch here; the wallet selection code is not identical (mymonero REST-API doesn't send output ids, so output selection via forking cannot be done), but otherwise the selection algorithm can be re-used by Openmonero without much difficulty. With this patch wallet2/light_wallet_server also share the coinbase filtering/searching logic too for instance. It might be best to rewrite this patch such that there is no dependency on this project at all. The trickiest part would matching the custom serialization format for the transaction hash prefix calculation, but everything else can probably be re-written into a reusable lib based on tweetnacl (to start anyway). |
aa2da7a
to
7e9c905
Compare
Changes that were force pushed:
|
It would be excellent to get this in. So far we haven't seen (alternative to MyMonero) light wallet providers, and having support for a light wallet server in |
Then someone who wants this should review it. |
Totally fair @moneromooo-monero - I would myself if I was familiar enough with the codebase to. Just wanted to drop my two cents in that I think this work would be extremely valuable to include if possible. |
7e9c905
to
3421db2
Compare
@ndorf pointed out the this merge was bad. I hadn't pushed out what was sitting on my laptop somehow. Pushed that out hours ago now. There will be update some after the LMDB PR gets merged. It looks like the db schema will be changing to help fix a bug, let me know if you are using this patch in a meaningful way and I will provide an upgrade program. |
I checkout the PR and compiled. |
Agree that this work is valuable. |
I would love for that code to be considered for PR. But such a PR involves a factoring of wallet2, I think. +1 thank you! |
I mean it would be a partial factoring but the main question is how we converge / collate / coalesce the transaction construction code, and then, it's mostly a matter of wallet2 calling the pure functions I've extracted from it into mymonero-core-cpp |
It was a question, not a request. Mainly because I see it as unwanted, and merging the daemon part might mean you come back later and ask for the wallet part to be merged too :) |
I think you might view most of mymonero-core-cpp as not strictly describable as "that new light wallet code" - since much of is straight lifted from Monero, actually. So if you mean only the lightwallet-specific code in mymonero-core-cpp is not desired, then I'd like to mention that I think I've always advocated for separation of repos somehow…… and maybe what we'd do is have a Monero core of some kind which has all the scanning code and Monero wallet mode code.. including all daemon underlying impl… and this would basically be the union of libmonerocore and the hypothetical libwallet …… then I guess we'd have the daemon and LWS in another repo, probably with simplewallet… then the GUI in a third repo? That way the GUI and simplewallet would code to the same underlying corewallet API. If so, whatever new lightwallet code there is would simply have unit tests added for it in that libmonerocore repo for its additions to the corewallet API. Just some thoughts. I don't mind PRing the removal of the existing dead LW code in wallet2 but am unsure exactly when I'll get to it - probably a week or so |
I've been looking at how to take this code into a separate repo - whether it is possible to have a Cmake file within the repo that other projects can import. This would be useful for this PR, openmonero, mymonero, and @jtgrassie mining pool. These other projects are already having to "hook" into the monero as-if it were designed to be a library, when it has not. Certain parts, like the device code (for ledger, etc.), have now "infected" several chunks of code due to a (what seems to be at quick glance) a poor design. There's also a weirdness between cryptonote_core / cryptonote_basic - some stuff from core should perhaps be moved to basic. You end up having to chop out functions manually to reduce dependencies or just accept the entire All of this makes putting this PR into a separate repo complicated because some re-architecting will be necessary. But multiple projects would likely benefit. The trouble is that I'm not sure how viable this is long-term - it will be easy to muck things up to accelerate some PR getting merged in the future. I can't even give a rough estimate after ~45 mins, but its fairly close to being easier to do something closer to what @paulshapiro has been suggesting for wallet libs - just copy/fork and refactor it to barebones and use that instead and then slowly introduce those new libs into monerod. On the topic of wallet2 + light-wallet code. I'd have to spend time looking at wallet2 again, but my thoughts have typically been that light-wallet code was really hacked into wallet2. The design would probably work better if it were inverted such that wallet2 had a generic interface for requesting possible spends (i.e. conform to the light-wallet interface). If the scanning is local the implementation just returns the results locally immediately, etc. One issue is that I don't recall build breaks or test failures in this area. |
Later this evening I will look at the opposite, see how stuff can be just copied/forked out (including using Paul's existing lib code). |
I'm fine with some factoring, as long as it doesn't become wholesale rewriting (in which case it'd belong as a separate "wallet3" lib, which monero could switch to at some point after it's well tested). |
Though of course this wallet3, if it was made, would risk going the way of the light wallet code or the 0mq RPC, which are bitrotting because noone maintains them, so you'd have to keep it up to date regularly for it to work in practice. |
I don't see how 0mq is bitrotting exactly - it still has the same features/info from before. It just hasn't kept up with the other API for new fields. The LWS server still worked as expected, and other software would likely too. |
That's what I meant, yes. I guess bitrotting was not the best word for it then :) |
There are four high-level ways forward:
FWIW, "librarize" monero was on the roadmap at some point. This PR depends on these components within this repo:
I have no recommendations right now. |
98fc8a7
to
3ad03cf
Compare
Force pushed changes:
|
My suggestions: Move this into a separate repo under monero-project, using cmake-foo similar to openmonero. Hopefully reviewers can/will be encouraged to at least point out that no nefarious code is implemented, so that it doesn't linger for much longer. Start a top-level I have several other things that need to be completed, so I hope to not spread myself thin. But multiple people have asked for something like this, so there is hope of maintenance and feedback support from others. I would expect some kind of feedback/code support from @ndorf @paulshapiro @moneroexamples @woodser @TheCharlatan for this lib. My aplogozies for calling these people out directly, but I want to highlight this post since they may be potential beneficiaries of this effort. I'm not going to make changes to this PR or start any work on a lib without some feedback on the topic (start a separate github issue?). |
@vtnerd total support |
https://github.com/mymonero/monero-core-custom could be a good starting point. This library contains a small subset of upstream Monero code for use in wallet implementations. The same content, but with Monero itself as a git ancestor, is available here: https://github.com/mymonero/monero/tree/core-custom-master (might be worth using Presently, the files themselves differ significantly from their upstream versions, but most or all of those differences are just removing or disabling extra functionality. It should be fairly straightforward to clean that up using one or more preprocessor macros. Anyway, this is already sufficient to use the output selection functionality of this PR. Some additional files would be needed for the entire server (starting with the ZMQ message types, I think). |
FWIW I have started looking into refactoring wallet2 and I expect I will continue, so please consider me available to help. The first thing that jumped out is that the light wallet is mostly an independent wallet within wallet2. Like @vtnerd said, ideally Monero offers a single interface which full and light wallets implement separately. The full wallet should probably offer a base class which minimizes application dependencies (e.g. using an interface to make network requests, not writing to disk, etc) in order to be run in different application environments (e.g. JavaScript for the browser or Go for OpenBazaar). I figured a good way to go about this is to first refactor the underlying wallet2 assets into e.g. wallet2_base, wallet2_full, and wallet2_light, then introduce a wallet3 interface and wrapper implementations which the GUI, RPC, and application developers can use long term. I don't have an opinion on where the light wallet assets should live now, but I do support the light wallet being officially supported by Monero Core. I would expect the light wallet to be alongside the full wallet and I expect it would be easiest to maintain the light wallet server if it is "close" to the other Core code, especially during these refactors. Otherwise realistically if our most active contributors maintain one and not the other because they're in separate repositories, wouldn't they quickly fall out of sync? |
@woodser that was the route I took in factoring wallet2 (think we may have discussed) - wallet3_base, wallet3_light, etc (see https://github.com/mymonero/mymonero-core-cpp--old - which is abandoned) but given the upcoming hybrid mode, having classes separated like that will cause whichever code hangs onto the reference to the wallet object instance to have to rebuild the instance of the wallet if they want to switch to another wallet type (in the soon to come hybrid mode). That's not the worst in the world but it probably makes sense to incorporate that kind of switching logic into a wallet class - and I don't think it's implicitly a good idea to say it belongs in a wallet manager class - not just because that's a route down the same black hole. I think a solution will be to not keep the implementations in those classes at all - but rather in monero core itself in the form of pure functions - then the actual wallet implementation itself could be trivially and potentially preferably implemented by whoever is making their own wallet software - but it would also expand options we have in writing whichever one(s) is/are used by Monero core. Invariably we're going to have some separation of code by repo / library … and I think the only way around that is for Monero to commit to having a particular API at each release and then deprecate that API like other projects. The API can come with unit tests and be negotiated in the same way as existing APIs are added.. based on needs, and such. Keeping them in separate repositories is an example of the definition of an increase in complexity but it provides many benefits. All in all, if the wallet code ended up in core, that doesn't seem like an issue to me, but the more important thing is the stability of the API imo, as @vtnerd also mentions. |
At the very least, we don't need to do anything to wallet2 except extract things from it so that those implementations can be shared... wallet2 can remain exactly the same externally - and its functions would simply end up calling / proxying into an extraction of their implementations. I think that as an initial goal would provide the majority of the benefits everyone's looking for. |
- An epee-based HTTP server that polls in the background for new txes - Admin utility for inspecting state, manipulating some account info, and approving account creation/imports - Optional (off by default) exchange rate retrieval from cryptocompare.com
3ad03cf
to
d738342
Compare
This will still be moved to another repo - in the meantime I pushed a rebase + merge and did some basic tests against it. Anyone running this early release should be good for the fork. |
Bringing this up again. The strategy to get this moving:
Anyway 1-4 is doable and gets people using this. |
This was done in #5207, |
I even commented on that PR. And I updated the lookup table with that entry too. Well, hitting middle age I guess. |
This project has been separated into its own project. Users are encouraged to upgrade, as there was an authentication bug. This has resulted in a breaking DB change - if someone was using this patch with a realnet DB, contact me for migration instructions. If someone wants an updated version of this PR let me know - I have an internal repo with most of the changes in it already. Updates with the new project:
I'd also like to gauge community sentiment on whether this should remain a separate project, or be merged into |
monero-lws is now in a separate repo and functional. |
Another [do not merge] :/ . This is in a solid "beta+" code - people should expect this to work reasonably well in its current state. Anybody testing this branch is encouraged to put bug reports or suggestions into this PR.
This patch is too large to be reviewed on its own. Instead pieces will be broken up so that various parts can be discussed on their own. Eventually the last chunk will be this PR, and the do not merge tag will be removed. This PR will receive force-pushes to keep the eventual log history somewhat pristine (and it will involve several smaller commits via other PRs anyway). A separate repo/branch will be identical to what goes into mainline, except history will never be rewritten. Some additional unit tests + documentation will be written as the PRs are being opened.
This provides a mymonero compatible server that uses LMDB and adds no additional dependencies to the project. For now - there is a server that scans txes in the background while an epee server provides the latest data in LMDB over the mymonero REST api. An optional, off by default, exchange rate retrieval from cryptocompare.com is also provided.
This also provides a separate admin utility. The utility can dump the DB state, change some limited account values, approve pending account creation/import requests, and manually add accounts - all while the server is running. The server will automatically detect changes via LMDB, and continue processing.
My biggest criticism - the ideal situation would be to have this in a separate repo, but I think its difficult until the internal parts of the system can be used effectively as a git submodule.