Skip to content
This repository has been archived by the owner on Sep 1, 2022. It is now read-only.

Improvements to the library organization #98

Closed
EinMByte opened this issue Jan 29, 2016 · 13 comments
Closed

Improvements to the library organization #98

EinMByte opened this issue Jan 29, 2016 · 13 comments

Comments

@EinMByte
Copy link
Contributor

Fragment from IRC:

<EinMByte> 1) Add an (actual) API to the core library.
<EinMByte> 2) Put I2PControl, Streaming etc in the client library.
<EinMByte> 3) Make the main application only use the client library and the API
<EinMByte> So the client library would use *only* the API to do core stuff
<EinMByte> (so the API is used instead of I2CP, since we don't have I2CP)

Comment from @fluffypony on the matter:
<fluffypon> I agree with that approach - core shouldn't be able to provide every-interface- ever when that can be done with stubby clients

This would mean that the application will consist of 3 rather than 4 components:

  • The core library (libkovri-core). The only thing that changes here is the addition of an API that allows loose coupling with the client library, and possibly other applications that want to use the core directly.
  • The client library (libkovri-client). This will also contain what is currently in libkovri-api. This library depends on libkovri-core, and its API to do whatever is necessary.
  • The main application, which should contain the daemon and (command line) interface code. This is the front-end only,. My suggestion is that this repository should only contain a minimal user interface, as is the case right now. There can be separate repositories that use the client and core libraries to provide (various) graphical user interfaces.

Please discuss.

@anonimal
Copy link
Collaborator

Welcome back @EinMByte. A very warm welcome to you.

My decision for our current organization was based on present I2P specifications. The idea was to parallel the specifications because present APIs are not yet necessary to Kovri nor are they necessary to the Kovri client at this point (JFTR: I'm not opposed to reorganization).

@EinMByte I see where you are going with the joining of client and api, but that is under the assumption that we will even keep those APIs. I've given thought to a "kovri-lite" that works specifically with bitmonero and our present "kovri" which could be a more functional i2p client (that is, if we wish to keep everything and put in the work). With a current spec-appropriate organization, I imagine that maintaining the two repos would be easy as "kovri-lite" would only need core (with proposed API) and possibly a custom client bitmonero side.

@EinMByte @fluffypony I'd like to hear more about your ideas on this. @EinMByte can you provide more support for your proposal? What API do you propose for core? How many more API's do we really need? One of our objectives is to provide I2CP; do you disagree with that direction?

As I've said before, I feel that I2P has too many moving parts so I'd like to see some more benefits to this reorganization. None-the-less, I'm glad we're talking about this and I think we're moving in the right direction 😄

@fluffypony
Copy link
Contributor

In discussions with zzz about the RNG it occurred to me that there were many design decisions (in I2P) that were good for the time, but in hindsight and given the landscape in 2016 they might be done differently. I don't think we should be afraid to push something out onto an interoperability layer, and implement our own standards internally.

Building on that, and on what you've said: let's think of it as kovri-core instead of kovri-lite, as that better describes its role. So kovri-core is effectively just the router, and that has some sort of API, be it 0MQ IPC or direct library hooks into functions.

Monero would use kovri-core, but so would Kovri (the full-featured router). Ideally kovri-core would be able to self-update, and also be able to service any applications that access it (so if I run Kovri and Monero on the same system there is only one instance of the running router).

Full featured Kovri is able to expose all sorts of interfaces: BOB, SAM, Lua, JSON RPC, whatever, and it's just transliterating that to kovri-core's API / IPC. If someone wants to provide native Python hooks into routing through the router and controlling it, they simply use kovri-core functions, no need to touch anything else.

Also this means there's no need to have two repos we keep in sync - Kovri just bundles kovri-core anyway.

@fluffypony
Copy link
Contributor

Also to add: Kovri can add interfaces as plugins, which means we don't even have to develop SAM or BOB support; someone else could (ostensibly) create a kovri-sam plugin and we bundle it in to the release (or git submodule it)

@majestrate
Copy link
Contributor

So kovri-core is effectively just the router, and that has some sort of API, be it 0MQ IPC or direct library hooks into functions.

I2PControl comes to mind but some things, like custom tunnel building algorithms, make more sense to be done via lua instead of I2PControl, IMO we should pick 1 and go with that instead of fragmenting everything up into a dozen different API components that end up highly coupled in the end.

Full featured Kovri is able to expose all sorts of interfaces: BOB, SAM, Lua, JSON RPC, whatever, and it's just transliterating that to kovri-core's API / IPC.

SAM and BOB are for clients to transport data while, lua / jsonrpc are for controlling the router. I think it would be best to explicitly separate these two types of API. A problem I see in the future is where I2CP fits in. I2CP is both data transport and a little bit of controlling the router, the control only applies to the destination belonging to the I2CP session but it's not simply data transport like SAM or BOB.

If someone wants to provide native Python hooks into routing through the router and controlling it, they simply use kovri-core functions, no need to touch anything else.

If we do that we're going to need to expose core to the world which means a lot of cleanup, too much to bite off for now, probably best to not think about that quite yet.

/end 2cents

@majestrate
Copy link
Contributor

One of our objectives is to provide I2CP; do you disagree with that direction?

IMO I2CP is a requirement, it allows everything that exists made for i2p to "just work" (no really everything, SAM and BOB go over I2CP in the lower levels)

How many more API's do we really need?

Ideally 1, but realistically 2 (maybe 3 if we can get away with it).

  • I2CP (exists+required, allows for all existing i2p applications to work out of the box with kovri, i2pd does not have this)
  • I2PControl (exists+not required, works with java i2p, is the proposed Grand Unified Router Controller thing, i2pd and java i2p has this, kovri has this )
  • lua ( not exists+not required, would be powerful tool for researchers, power users, hackers, tinkerers, no one has this)
  • SAM (exists+not required, i2pd has it, java i2p has it, we don't anymore )
  • BOB (exists+not required, i2pd has it, java i2p has it, we don't anymore )
  • 0MQ (exists+not required, no one has this but it would allow people outside the i2p ecosystem to easily develop i2p applications without bullshit )

@EinMByte
Copy link
Contributor Author

@anonimal I am not opposed to providing I2CP support, but I wish to be realistic: it's going to take a while to implement the full protocol.
As a first priority, I want to make sure people can start using the core as a library. Monero is good example.
However, in response to @fluffypony, the core is unlikely to be able to self-update (since it's not a stand-alone application). Then again, replacing the core should be a trivial matter and the API should facilitate this.
I2CP itself could be put into the client library. This is a fundamental difference with the Java router, but I see little reason to follow that design. We only need I2CP to allow easy coupling between Java and C++ components. The core does not take the form of an application, this is in contrast with the Java router (which is not intended to be bundled).

In response to "How many APIs?": for the time being we need only two APIs, but the client library/API requires no additional code. The interface to the core library is the new part. The components from the client library must be modified to use this API, and they can be used in a stand-alone manner.

When @majestrate is talking about plugins that allow custom tunnel building algorithms etc., you'll need direct access to the core in some cases. Some of these things cannot be done in the Java router today, and it would be nice to offer that amount of extensibility. To me this boils down to making sure that the API provided by the core is very flexible.

Of course I'm willing to write relevant code for all of the suggestions I have made above.

@majestrate
Copy link
Contributor

(ftr my github handle is @majestrate , @psi is someone else )

@fluffypony
Copy link
Contributor

@EinMByte we could publish "best practice" implementation that includes auto-updating to spec, or provide a small auto-updater library that can be called on startup + routinely or whatever. In terms of the actual auto-update, more discussion on #48, but effectively it's just stopping the router, replacing the .so / .dll for kovri-core, and starting it up again, right?

@EinMByte
Copy link
Contributor Author

@fluffypony I agree, auto-update can be something in the client library or if that becomes too large it can be a separate library with some other basic stuff.

Something more specific:

As a first step, I propose to move the following things from client to the main kovri application:

  • Daemon, DaemonLinux, DaemonWin32
  • Config

This mainly requires ClientContext to be initialized in a different way (it shouldn't use the config directly). A builder pattern is needed there anyway.

@anonimal
Copy link
Collaborator

In terms of the actual auto-update, more discussion on #48, but effectively it's just stopping the router, replacing the .so / .dll for kovri-core, and starting it up again, right?

@fluffypony yes, but the trick is how to do that self-sufficiently without an external container or with as little co-dependency as possible - and all while reusing libcore functions to download/validate/install said update without moving libcore code around or redundantly copying libcore code into other libs. So, while it's been stated that yes libcore shouldn't nor can it (at the moment) auto-update, we'll have to think about how to reuse our code instead of reinvent - and that may take reorganization.

There are ways we can auto-update and our library layout will definitely effect those decisions. Yay #48.

How many more API's do we really need?

This was actually a rhetorical question. Available API's are well documented so the question was more of a response to this previous question:

What API do you propose for core?

@EinMByte, I agree with the reality of I2CP but what do you propose for an API (or is that TBD)?

I'm still not seeing enough reason to move libs around but I'm also not seeing enough reason to keep our current design. It also seems like we could throw out libapi all together instead of merging with libclient - or not. Daemon* + Config could move into libcore - or not (though, I can see the motive behind doing so (see below)). My point to @fluffypony is for reorganization - but there are other ways to approach the problem.

So, I see this ticket becoming a moot point. Can anyone provide more concrete relationships between the need to reorganize (staying on-topic to the ticket) and the need to implement a core API + everything else proposed? Regardless, we should open a new ticket for at least the core API proposal.

As a first priority, I want to make sure people can start using the core as a library.

I agree, and I see this as a good motive for reorganization. More motives would be appreciated though.

@anonimal
Copy link
Collaborator

It also seems like we could throw out libapi all together instead of merging with libclient - or not.

Edit: I didn't mean all of libapi, just the non-essentials after reorganizing.

@EinMByte EinMByte self-assigned this Feb 2, 2016
@anonimal
Copy link
Collaborator

anonimal commented Feb 2, 2016

From Sunday's meeting:

2016-01-31 19:58:42     +EinMByte       anonimal: You were OK with merging libapi and libclient right? Forgot to ask since that was part of my short-term TODO for #98
2016-01-31 20:00:55     &anonimal        Yes, as long as the reasoning is: A) part of an overarching *essential* design decision and not just aesthetics (which we are good on because we've talked about it) and B) we use everything within t
he two libs and anything else is pruned or moved elsewhere.
2016-01-31 20:01:38     +EinMByte       Well "we" use it or someone else does, since it's a library
2016-01-31 20:02:30     &anonimal        By "we", e.g., if SAM/BOB was still there, B) would ensure that it's not until it is needed.

In addition, overall great discussion and things are moving along as planned. Related comments in #107.

anonimal added a commit that referenced this issue Feb 7, 2016
0bff9a2 Cleanup (following cpplint advice) (EinMByte)
4bbe89d Resolve #107. (EinMByte)
43f14e1 Move config file constants to Config.h. (EinMByte)
2f39034 Separated libcore, libclient and kovri application (issue #98) (EinMByte)
6efa143 Move Daemon to kovri-main. (EinMByte)
e0f9c73 Merge libclient and libapi. (EinMByte)
anonimal added a commit that referenced this issue Feb 7, 2016
8999af5 Reseed: improve non-standard ports patch (581dd9e) (anonimal)
581dd9e Reseed: fix bug for servers with non-standard ports. (anonimal)
8eb187f CMake: update OSX OpenSSL. (anonimal)
0958052 Minor ammendments to reflect new lib organization. (anonimal)
cf48ea0 Fix build, forgot to include crypto/Rand.h (EinMByte)
a5de2d6 Fix bitmask used to select SSU fragment size (thanks orignal). (EinMByte)
1dcbe27 Fill padding with random bytes in NTCP (thanks psi). (EinMByte)
d23138d Add bounds check to SU3 fileNameLength (thanks psi). (EinMByte)
0bff9a2 Cleanup (following cpplint advice) (EinMByte)
4bbe89d Resolve #107. (EinMByte)
43f14e1 Move config file constants to Config.h. (EinMByte)
2f39034 Separated libcore, libclient and kovri application (issue #98) (EinMByte)
6efa143 Move Daemon to kovri-main. (EinMByte)
e0f9c73 Merge libclient and libapi. (EinMByte)
anonimal added a commit that referenced this issue Feb 17, 2016
51dcb23 License/Docs: implement 0MQ 22/C4.1 + revise docs. (anonimal)
5dc950f License/Docs: minor revisions to license. (anonimal)
fd859a7 Crypto: update RandInRange() backend + unit-test. (anonimal)
9c330cb Crypto: create true RandInRange(). Fixes #130. (anonimal)
de59b56 Config: implement single-switch opts, refactor. (anonimal)
06cd79b Reseed: update server list + certificates. (anonimal)
e6e8b15 Refactor: resolve all using-directive TODO's. (anonimal)
5153a94 I2NP/NetDb: move NETWORK_ID to Version.h (anonimal)
037e4f6 I2NP: fix tunnel build request raw memory leak. (anonimal)
456a14d NTCPSession: fix Phase2 raw memory leak. Refs #65. (anonimal)
eb6d2e6 NTCPSession: refactor datatypes + identifiers. (anonimal)
5d64e62 NTCPSession: resolve a refactor TODO. (anonimal)
8999af5 Reseed: improve non-standard ports patch (581dd9e) (anonimal)
581dd9e Reseed: fix bug for servers with non-standard ports. (anonimal)
8eb187f CMake: update OSX OpenSSL. (anonimal)
0958052 Minor ammendments to reflect new lib organization. (anonimal)
cf48ea0 Fix build, forgot to include crypto/Rand.h (EinMByte)
a5de2d6 Fix bitmask used to select SSU fragment size (thanks orignal). (EinMByte)
1dcbe27 Fill padding with random bytes in NTCP (thanks psi). (EinMByte)
d23138d Add bounds check to SU3 fileNameLength (thanks psi). (EinMByte)
2dc4d74 Fix typo in ElGamal.cpp. (EinMByte)
20859e6 Review PR #112 (resolves #114). (EinMByte)
0bff9a2 Cleanup (following cpplint advice) (EinMByte)
aeca942 Clean up comments in DSA and ElGamal unit tests. (EinMByte)
1e457f3 Rename crypto unit test files for consistency. (EinMByte)
7c5315f add copyright (Jeff Becker)
9e70f59 fix up elg tests (Jeff Becker)
3c0bca4 use main prng in Elgamal Key Generation (Jeff Becker)
d4ffd85 add elg tests and fix up dsa tests (Jeff Becker)
748ec5b fix up elgamal, add bounds check and zero pad check (Jeff Becker)
43f582f add dsa tests (Jeff Becker)
6298dfb make it compile (Jeff Becker)
79620dc reorganize existing crypto unit tests into their own files (Jeff Becker)
5792af6 Fix #109. (EinMByte)
193c3f4 remove OpenSSL link instructions from building.md (Riccardo Spagni)
1f8d3f0 find installed OpenSSL on OS X (Riccardo Spagni)
835cfea Update MacOSX build requirements. Resolves #37. (anonimal)
4bbe89d Resolve #107. (EinMByte)
43f14e1 Move config file constants to Config.h. (EinMByte)
2f39034 Separated libcore, libclient and kovri application (issue #98) (EinMByte)
6efa143 Move Daemon to kovri-main. (EinMByte)
e0f9c73 Merge libclient and libapi. (EinMByte)
d2c0908 Remove coreVersion + stat_uptime. Bump to 0.9.24. (anonimal)
9a14807 Remove redundant #pragma once in Daemon.h. (EinMByte)
76c1cf2 Fix race condition in Transports.cpp. (EinMByte)
9afe4bd Add core/util/Log.h to Daemon.cpp. Fixes #85. (anonimal)
e330dcd don't send ssu packets if they are too big (also covers underflow of size_t which in this case made kovri segfault last night) (Jeff Becker)
24e9b71 fix #85 (Jeff Becker)
53c9867 Huge cleanup of logging implementation. (anonimal)
ab6fae4 Cherry-pick remaining doc/crypto commits from #81 (anonimal)
47223f8 Cherry-pick commits from #81. (anonimal)
661b002 Revert #81 before cherry-picking. (anonimal)
c45f521 fix up i2lua to conform to style guide better (Jeff Becker)
b4f94eb check for dsax >= dsaq (Jeff Becker)
8e77042 (probably) fixed issue with CryptoPP exceptions (Jeff Becker)
e768333 * add documentation strings * add i2p::Buffer for pre filled buffers * various changes to unfux crypto parts (Jeff Becker)
803c799 fix up lua parts, implement i2lua.GetRouterByHash (Jeff Becker)
ede65b2 remove linting related things (Jeff Becker)
88da250 more indentation fixups (Jeff Becker)
14cf9aa more style fixups (Jeff Becker)
3829173 linting (Jeff Becker)
15594e4 Update .codedocs + .gitignore. (anonimal)
b567bba Add .codedocs file and CodeDocs badge to README.md (Paul Novotny)
5889c60 if not logging still start daemon (Jeff Becker)
502d187 fix up PRNG bits (Jeff Becker)
d4e577b add linting helper tools (Jeff Becker)
60173bf Massive refactor of #72. Update style guidelines. (anonimal)
9eae20b we don't need a dummy rng for eddsa anymore (Jeff Becker)
5f473be * use function pointers and NOT std::function in benchmarks as doing so crashes benchmarks occasionally (Jeff Becker)
94ea36f add i2lua documentation (Jeff Becker)
88a511e fix boost::log link error (Jeff Becker)
8605937 * make lua interpreter actually work (Jeff Becker)
243c898 add initial boilerplate for lua interpreter (jeff)
51d9a21 fix benchmarks (jeff)
a8b2b44 fix typo (jeff)
26a4482 compiles and links but untested refactor of all parts using CryptoPP's PRNG to use i2p::crypto::Rand* functions which wrap CryptoPP's PRNG such that it can be swapped out with relative ease if needed. (jeff)
39b513a fix i2pcontrol crash (jeff)
2429f3a add tunnels.cfg reload (jeff)
anonimal added a commit to anonimal/kovri that referenced this issue Oct 26, 2016
- All client code only requires kovri::client namespace
- Moved code to more appropriate libclient directories
- Specified header locations in relation to libraries

Note: this work will prepare libclient for further work with larger
refactoring and library improvements.

This commit is malleable and the work will most likely be subject to
further revision.

References monero-project#339 monero-project#98
anonimal added a commit to anonimal/kovri that referenced this issue Oct 27, 2016
- All core code only requires kovri::core namespace
- Moved code to more appropriate libcore directories
- Renamed files where approriate (also done in 47e7dcb)
- Specified header locations in relation to libraries
- Removed unused cryptopp headers (outside of crypto)

Note: this work will prepare libcore for further work with larger
refactoring and library improvements.

This commit is malleable and the work will most likely be subject to
further revision.

References monero-project#339 monero-project#98
anonimal added a commit to anonimal/kovri that referenced this issue Oct 27, 2016
anonimal added a commit to anonimal/kovri that referenced this issue Oct 27, 2016
anonimal added a commit to anonimal/kovri that referenced this issue Oct 27, 2016
anonimal added a commit to anonimal/kovri that referenced this issue Oct 27, 2016
anonimal added a commit to anonimal/kovri that referenced this issue Oct 27, 2016
- Based on previous commits
- Also updates ed25519 (custom sha512 header)

References monero-project#339 monero-project#98
anonimal added a commit to anonimal/kovri that referenced this issue Nov 26, 2016
Introduced in 2f39034, we must initialize contexts in child process (if
in daemon mode) or else we'll SIGSEGV on startup when in daemon mode.

Also adds some exception handling, functions, docs, and TODO's.

Closes monero-project#214 (if file logging is enabled, we can log when in daemon mode)

References monero-project#98
@anonimal anonimal added this to the 0.1.0-alpha milestone Jan 25, 2018
@anonimal anonimal assigned anonimal and unassigned EinMByte Jan 25, 2018
@anonimal anonimal removed this from the 0.1.0-alpha milestone Jun 28, 2018
@anonimal
Copy link
Collaborator

anonimal commented Sep 7, 2018

NOTICE: THIS ISSUE HAS BEEN MOVED TO GitLab. Please continue the discussion there. See #1013 for details.

@anonimal anonimal closed this as completed Sep 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants