This repository has been archived by the owner on Sep 1, 2022. It is now read-only.
AddressBook + HTTP: major refactoring, enhancements, fixes. Refs #305 #336
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Move filesystem code into new files
- Refactor layout to linearly reflect design
- Doxygen document all classes and members
- Document and comment-out unused code
- Move GetB32Address() into core/Identity.h
- GetB32Address() is a data identity function
that's not specific to address book
- Add many TODO's for ensuing commits
- Refactor constants into scoped enum - Fully qualify integer types - C++11 refactor
- Rename most members + variables + filenames to accurately describe their purpose and relationship - Create an unordered map of default constants - Create new getter functions for default constants - Add documentation + clarify existing documentation - Resolve various TODO's + add more TODO's - Comment-out more unused code - Minor design refactor - Minor style refactor References monero-project#305
- Implement needed sanity tests for hardening - Implement written narrative for address book + subscriber process - Add default publisher URI to subscriber if publishers file not found - Catch error code message for subscriber's update timer - Remove unneeded check for local destination - Add documentation + refactor to clarify member purpose References monero-project#305
Will be needed for address book subscription
Major: - Implementation improvements * HTTP: download impl * HTTP: URI handling impl * HTTP: response header handling for clearnet * AddressBook: unhook in-net HTTP impl (now in class HTTP) * Add documentation Medium: - If default subscription file does not exist, create/save after downloading from publisher Minor: - Adjust related reseed and tunnel code for class HTTP - Update variables names to accurately reflect purpose - Update log messages for accuracy - Update address book flow narrative - Update/improve HTTP unit-test References: - monero-project#305 (parent ticket) - monero-project#168 and monero-project#155 (both in terms of HTTP/S) - monero-project#154 and monero-project#48 (we now have a multi-purpose HTTP/S download impl) Notes: - My apologies for not keeping this commit atomic. Bigger design work can be harder to keep atomic (especially if one wants new features to work before committing) - This commit has been successfully tested and all unit-tests pass
- Remove unordered_map of default constants - Create struct AddressBookDefaults for said constants - Rename related functions for accuracy
- Replaces solitary DST Root CA X3 for manas.ca - Fixes download for CloudFlare'd getmonero.org
- Run sort on file
…n doesn't exist Also minor refactor/documentation References monero-project#305
- Add bounds checking for subscribers downloading * Also add note about why it looks confusing - Remove copypasta TODO's for bounds checking * Impl does not apply to std::map - Add notes about identity sanity tests in AddressBookFilesystemStorage - Resolve CSV TODO
| std::ofstream::in); | ||
| if (file.is_open()) { | ||
| std::string publisher; | ||
| while (!file.eof()) { |
There was a problem hiding this comment.
@anonimal Can we use while (!getline(file, publisher))?
There was a problem hiding this comment.
After removing getline() from within the block, I think we'd want while (getline(file, publisher)) or else we'll skip the block entirely.
| const std::map<std::string, i2p::data::IdentHash>& addresses) { | ||
| std::size_t num = 0; | ||
| auto filename = GetAddressBookPath() / GetDefaultAddressesFilename(); | ||
| std::ofstream file(filename.string(), std::ofstream::out); |
There was a problem hiding this comment.
@anonimal Don't need std::ofstream::out.
|
JFTR: nearly all the previous comments refer to old code that I chose not to touch because A) it worked (enough) and B) fixing/cleaning up would equate to more work for me 😆 but yes, this all should be fixed before this is merged. Thanks for the insight and for cracking the whip (and full inbox) @EinMByte 😉 Also, what I'm going to do is open two new tickets: 1 for spec discussion proposed above and 1 for finishing HTTP/cpp-netlib (its now out of scope for address book). After that, we should mark #305 as resolved. |
- Minor design refactor to avoid redundant publishers check - Remove useless interface AddressBookFilesystemStorage * Storage code now entirely class AddressBookStorage - More and better sanity tests/hardening * bool test after ifstream construction * Validate input lines that have whitespace * Instantiate URI sanity test *outside* of while loop - Use baseline HTTP request timeout in all request code - Add constants for HTTP response codes - Add another .i2p TLD check to HTTP unit-test - Resolve/remove TODO's + add TODO's - Add more documentation
|
There are a few more TODO's I'd like to see resolved before closing #305. Other than that, I'll proceed as planned with opening new related tickets. |
This was referenced Sep 8, 2016
anonimal
added a commit
that referenced
this pull request
Sep 8, 2016
6eddd9c AddressBook + HTTP: remove unnecessary else statements (anonimal) 418edac AddressBook + HTTP: resolve #336 comments and more (anonimal) 1ac2d80 AddressBook: use std::thread::join when downloading (anonimal) 37f0508 AddressBook: add bounds checking + resolve TODO's (anonimal) 216a2a1 AddressBook: fix writing new subscription to disk only if subscription doesn't exist (anonimal) 03019c0 Package: add monero{price,nodes,world}.i2p to hosts.txt (anonimal) d186de6 Package: add common CA certs for manas.ca and getmonero.org (anonimal) 7ffb5b6 AddressBook: refactor defaults into AddressBookDefaults (anonimal) add429b HTTP/AddressBook: major refactoring + download impl (anonimal) 7458653 Package: add downloads.getmonero.org SSL cert (anonimal) c93407a Package: create publishers.txt for address book (anonimal) bf141be AddressBook: sanity tests + narrative + refactor (anonimal) 0498624 AddressBook: refactoring, documentation, and more (anonimal) 2f9839d AddresBook: datatype refactor. Refs #305 (anonimal) f943ffc AddressBook: documenting + refactoring. Refs #305 (anonimal)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By submitting this pull-request, I confirm the following:
Place an X inside the bracket (or click the box in preview) to confirm
This addresses most of #305 but the work is not yet complete. Most notably unfinished (as noted in TODO's) is the cpp-netlib refactor in HTTP.
Design-wise,
AddressBookis still a bit hairy because of the update-timer - but the design is functional (enough). I want to get this into master so I can take a step back & gain some perspective and so someone else can easily contribute if they'd like.Also, though we can have multiple publishers, we currently throw everything into one subscription file (overwritten, not appended) and only use one subscription file instead multiple (e.g., default, custom, private). This area of the spec is up for debate @EinMByte @fluffypony. This feature could be nice (low priority) but I think it could fit well in its own ticket instead of throwing it with #305 (but I'm also antsy to close #305 and move on, so I may be biased).
With that said, this PR is in a stable state: clearnet and in-net subscription handling is functional, tests pass, etc. More details are given in commit log messages.