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

Merge Development branches #81

Merged
merged 21 commits into from
Jan 26, 2016

Conversation

majestrate
Copy link
Contributor

changes include:

  • fixes CryptoPP exceptions that were thrown, those were (probably) caused by using 1 instance of CryptoPP's PRNG across multiple threads.
  • bundling linting tools so others don't need to scavenge the net for them
  • (toy) lua interpreter that is/was useful for finding bugs, disabled in build by default
  • logging now shuts up if disabled in configuration

@anonimal
Copy link
Collaborator

I would like to merge if you are willing + able to make the appropriate adjustments/fixes:

  • Either revert or let's come to a conclusion regarding cli args
  • i2lua came out of nowhere. I think re: testing, a testnet is good, but I wonder if creating two routers like this is the best way to go about doing it. For now, if it helps, we can keep i2lua as long as there is a clear distinction between kovri and any contributing tools:
2016-01-21 19:02:00     &anonimal       I really don't think any of the lua stuff should be in core, it can all go into contrib or src/tests/i2lua or src/contrib/i2lua and linkup there. Also, I'm still reviewing because I see at least a few issues.
  • CPPLINT.cfg and the lint hook are cluttering root, can you move them to contrib/cpplint? (is CPPLINT.cfg really necessary)?
  • This PR is much better than the last one re: style but you did miss files (no cpplint), and functions + pointers + namespaces are not inline with our contributing guide
  • It would help if you could PR more often. Lots of stuff like this that cover various topics are more time-consuming to accurately review.

@majestrate
Copy link
Contributor Author

re: varmap

+psi | if it's really THAT bad we can always add another method that takes a varmap

re: i2lua not in core

+psi | lua really SHOULD be in core as it adds hooks to things to the netdb tunnel building parts

re: linting stuff

good point

re: style

I tried really hard but I don't see anything that is massively off? can you point to an example so I can correct it?

re: pr more often

can do.

@anonimal
Copy link
Collaborator

Awesome, I'm glad you're flexible. I'll be flexible too.

  • Re: an example, I really don't know what to say because I see an entire codebase of them. For starters, re-reading the contributing guide. For more recent examples, see Massive refactor of #72. Update style guidelines. #75 as it directly refactors your recent work. To get an overall sense of the style and rythm, I would look at phase 1 of Quality Assurance #58. Also, nowhere in our base are our functions_styled_like_this(). I have a huge respect for your effort and the fact that your trying to stay inline. Now that you've told me this, I don't feel as strongly about any blatant disregard to the agreed upon contributing guide (not just style but development model and workflow model).
2015-11-30 18:45:58     +psi    no thoughts besides we need a strict code style
2015-11-30 19:03:18     &anonimal       I don't like google but https://google.github.io/styleguide/cppguide.html inherits good points
2015-11-30 19:04:38     +psi    it's strict and specific, like it

Our model is dependent on communication and I think we need more of that before every idea is PR'd. Workflow too + topic branches, especially so we don't have to cherry pick things like this in the future. The absolute last thing I want to do in a review is trip over style, so if it helps that I keep refactoring any PRs (and that in turn is the example), then I can do that (I learn by example too).

On that note, these other factors effect my opinion re: codebase decisions:

  • I'm seeing persistent inability to run cpplint, even after putting the linter into the codebase. This has huge repercussions for large PRs of which I have to review. I don't have the time to catch the more difficult bugs because I'm looking at the glaring project management issues. I want that to be resolved so I can dive into your kickass new code with you because that would help the project and help me get a better perspective of your ideas. Re: keeping the linter in our codebase (which I don't think we should do), if future devs refuse to read the contributing guide and subsequently $ pip install cpplint on their own, then I seriously question the quality of any C++ contribution they can make.
  • I'm also seeing inattention to important security details and subsequent mistakes. This can happen when someone hasn't lived with the code long enough to gain a sense of oversight; e.g., observe and remember important points that I clearly commented in the code. Things like this would not have been missed had the code been given better review before attempting to fix something that wasn't broken (and thus breaking it).
  • Re: more breakage,
    2016-01-20 15:22:24 +psi the reason why it broke was because i didn't see that we had benchmarks
    I know there is a lot to cover, so I understand, but benchmarks that have been there since the start of the project. We all make mistakes and errors, but this is what I'm talking about re: needing to live with the code longer to remember it and to gain a knowledgeable perspective before acting on it.
  • Re: varMap: it's trivial at this point. Patchwork like this only causes more problems that it solves. Breaking CLI functionality is a moot point. If it must be done, it should be taken care of swiftly and in one swoop as to not impede development or it should be dealt with in another branch all-together. If this is a real issue, then that's what /issues are for and an new issue should be opened. I'm am soooo for tearing everything down to make it better, but we agreed to more communication and to focusing on two sepearate entities: me: design and functionality, you: crypto. We can/should/will continue to do both but that can only work as long as we open communication before PR'ing (preferably before committing). I am not opposed to adding another method, but we should talk about it before acting on it. I would ask the same of myself if I started working on crypto.
  • Re: i2lua, I feel that the scripting interface is too young to include in the codebase at this time. Hacking kovri to work with the new router is, IMO, not inline with Kovri's mission and I don't see much point in having new or existing devs deal with Lua (let alone a separate router) as a means to creating a testnet. More research and discussion is needed or else the codebase will become erratic and unmanageable and work/time is wasted. A testnet is a freaking great idea, I want it badly, but there has to be better options. If not, then I'll support pouring more community effort into i2lua. I really want cool new things like this, and if this weren't an anonymity project (and if we had more devs) I would say "balls-to-the-wall free-for-all!" but I don't feel comfortable doing that at this point. Since I think you'll continue to work on i2lua, maybe keeping it another branch for now would be best and, also, keeping it out of core (regardless of what it hooks) as that would be for the better in terms of our project. When i2lua matures (and if we decide to keep it with kovri and not a separate program), we can always merge into development. In the mean time, we really should discuss our options, preferably at our next meeting.
  • Re: client, core: it's simple: anything that interfaces with core is either client or api. core is the kovri router and only the essentials that keep the I2P network intact and online. What is currently there is essentially all that needs to be there. I've slaved all my free non-work time over the past 3 months living and learning this codebase and I'll need a very good argument as to why any of the current design logic should change, but I am open to change. Right now, I don't see any substantial arguments to supplement the existing design with any additional code outside of what is currently missing and that needs to be implemented (Quality Assurance #58, TunnelEndpoint: implement out-of-order message fragment reassembly #74, Enforce message expiration #77, etc.).
  • Re: communication; IMHO, responding to questions, asking questions, bringing up ideas for discussion and not just doing something and expecting/assuming that it will/should be merged; all needs more communication. This is why I'm adamant about implementing Agile; because it ensures open communication so that we know what the other is doing on a daily (or semi-daily) basis. I've screwed up in the past re: lack of communication when starting Quality Assurance #58, these things happen and I'm sorry for that, but as a result, I think we've learned that communication can issues be fixed. Since this is FOSS, I know I can't ask for anything from anyone but if I didn't think that everyone involved with this project can do great things, then I wouldn't be here to begin with.

So, I think we should get the fixes out of this PR and into development a.s.a.p. If your tree is clean (after merging into a personal dev branch perhaps?), maybe a git revert of everything here but the fixes and then a new PR will do the trick. I'm open to ideas. I don't want to squash anyone's workflow but I also want to be on the same page.

* add i2p::Buffer for pre filled buffers
* various changes to unfux crypto parts
@majestrate
Copy link
Contributor Author

nevermind then, this is way too much bullshit for me to go through.

@majestrate majestrate closed this Jan 23, 2016
@anonimal
Copy link
Collaborator

If by "bullshit" you mean failing to follow through with your agreement and the basics of our contributing guide, then I agree. If you mean "bullshit" as in the facts-of-life behind running a real organization, then I disagree: basic standards must be set and adhered to.

I don't know who you work for, but I can't imagine that they tolerate that kind of immaturity and response. Like I've said before, I'm flexible and open to discussion - but rage quitting is not the solution, and you're always welcome to finish what you've started.

@majestrate
Copy link
Contributor Author

If by "bullshit" you mean failing to follow through with your agreement and the basics of our contributing guide, then I agree. If you mean "bullshit" as in the facts-of-life behind running a real organization, then I disagree: basic standards must be set and adhered to.

There is too much red tape for a project with 2 (maybe 1 soon?) contributors.

I don't know who you work for, but I can't imagine that they tolerate that kind of immaturity and response.

Such a lovely and mature (totally not patronizing or condescending) way to frame your response.

Like I've said before, I'm flexible and open to discussion

Your actions so far seem to contradict this statement.

rage quitting is not the solution, and you're always welcome to finish what you've started.

Allow me to contribute code if you're serious about that.

@majestrate majestrate reopened this Jan 25, 2016
@anonimal
Copy link
Collaborator

Allow me to contribute code if you're serious about that.

This is your 10th pull request to Kovri since Nov. 17th, 2015. All previous 9 have been merged without question. This is the only one that had issues which came under scrutiny before merging.

@fluffypony will merge this in the morning, SAST time. Once that's done, we'll open the appropriate tickets for the issues that will arise.

@fluffypony fluffypony merged commit c45f521 into monero-project:development Jan 26, 2016
fluffypony added a commit that referenced this pull request Jan 26, 2016
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)
5889c60 if not logging still start daemon (Jeff Becker)
502d187 fix up PRNG bits (Jeff Becker)
d4e577b add linting helper tools (Jeff Becker)
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)
@anonimal
Copy link
Collaborator

Re: #83, @fluffypony and I have agreed to revert the appropriate commits instead of opening a slew of tickets because:

A) Neither of us have the time to do so
B) The reasoning for reversion was clearly stated above

However, I will open tickets for an introduced bug, an i2plua feature request, and a review of RouterContext.

Topic branches

Our contributor workflow is inline with Bitcoin's so commits like 8605937 have a better chance of causing less friction for any development team if appropriate topic branches are used for each relevant commit/PR.

From now on, we will allow a wiki-git philosophy where we will merge regardless of quality, but all contributors should realize that this is a community project and everyone may have to (at some point) self-sacrifice for the good of the software. All contributors are free to do whatever they want within their own forks but, like any pull request, a "request" is just that: a request to be considered - not a demand to be satisfied.

Thank you 🙇

anonimal added a commit that referenced this pull request Jan 27, 2016
ab6fae4 Cherry-pick remaining doc/crypto commits from #81 (anonimal)
47223f8 Cherry-pick commits from #81. (anonimal)
661b002 Revert #81 before cherry-picking. (anonimal)
anonimal added a commit that referenced this pull request Jan 29, 2016
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)
a8035b0 Update Coverity configuration (not yet enabled). (anonimal)
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 that referenced this pull request 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)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants