Identity: refactor routing key creation #882

coneiric · 2018-05-30T03:28:52Z

By submitting this pull-request, I confirm the following:

I have read and understood the contributor guide in kovri-docs.
I have checked that another pull-request for this purpose does not exist.
I have considered and confirmed that this submission will be valuable to others.
I accept that this submission may not be used and that this pull-request may be closed by the will of the maintainer.
I give this submission freely under the BSD 3-clause license.

Resolves TODO replacing sprintf with calls to standard library functions, and fixes gcc8 format truncation warning.

anonimal

Identity: use snprintf to create routing key

lol, I never want to read that sentence again.

Let's not use any C functions for routing key generation / in this function. I'd like to see a smarter / safer implementation of this function.

I know that's beyond the purview of this PR but if you're interested in doing more that (work on this function) then I can hold on merging - just let me know. We could also resolve the windows build (that this PR breaks) in the process.

and "hu" specifiers to silence format truncation warnings.

What warnings? I can't reproduce.

coneiric · 2018-05-30T16:51:22Z

lol, I never want to read that sentence again.

😄 felt gross writing it, too.

Let's not use any C functions for routing key generation / in this function. I'd like to see a smarter / safer implementation of this function.

I'm using boost input/output facets for stream formatting in another branch I'm working on. Would this count as smarter/safer? For this PR, I was just following the TODO, and trying to minimally change the code. I'm willing to implement the facets, if you think this is a reasonable approach. If not, I'm open to suggestions.

What warnings? I can't reproduce.

Using gcc8 on your fix-build branch with make all-options:

In file included from /kovri/build/src/core/cotire/kovri-core_CXX_unity.cxx:8:
/kovri/src/core/router/identity.cc: In function 'kovri::core::IdentHash kovri::core::CreateRoutingKey(const IdentHash&)':
/kovri/src/core/router/identity.cc:750:7: warning: '%04i' directive writing between 4 and 11 bytes into a region of size 9 [-Wformat-overflow=]
       "%04i%02i%02i",
       ^~~~~~~~~~~~~~
/kovri/src/core/router/identity.cc:750:7: note: directive argument in the range [-2147481748, 2147483647]
/kovri/src/core/router/identity.cc:750:7: note: directive argument in the range [-2147483647, 2147483647]
In file included from /usr/include/stdio.h:862,
                 from /usr/include/c++/8.1.0/cstdio:42,
                 from /usr/include/c++/8.1.0/ext/string_conversions.h:43,
                 from /usr/include/c++/8.1.0/bits/basic_string.h:6361,
                 from /usr/include/c++/8.1.0/string:52,
                 from /kovri/src/core/../core/instance.h:34,
                 from /kovri/src/core/instance.cc:33,
                 from /kovri/build/src/core/cotire/kovri-core_CXX_unity.cxx:4:
/usr/include/bits/stdio2.h:33:34: note: '__builtin___sprintf_chk' output between 9 and 34 bytes into a destination of size 9
   return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
          ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       __bos (__s), __fmt, __va_arg_pack ());

anonimal · 2018-05-30T19:28:28Z

Using gcc8 on your fix-build branch with make all-options:

Ah, I see now. But the format is for unsigned and the type is signed:

In file included from /home/anonimal/kovri/build/src/core/cotire/kovri-core_CXX_unity.cxx:8:
/home/anonimal/kovri/src/core/router/identity.cc:751:7: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
      tm.tm_year + 1900,
      ^~~~~~~~~~~~~~~~~
/home/anonimal/kovri/src/core/router/identity.cc:752:7: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
      tm.tm_mon + 1,
      ^~~~~~~~~~~~~
/home/anonimal/kovri/src/core/router/identity.cc:753:7: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
      tm.tm_mday);

So, gcc says one thing and clang says another - but both are correct 🤷‍♂️. I don't suspect overflow given the parameters but obviously this should be resolved during the refactor.

I'm using boost input/output facets for stream formatting in another branch I'm working on. Would this count as smarter/safer?

Probably not smarter since the standard library would work fine with this function.

For this PR, I was just following the TODO, and trying to minimally change the code.

Ok, then please just patch up this PR to work with windows, add a TODO to the function, and do the usual referencing of PR number in gitlog. Thanks.

Create routing key using standard library functions for memory safety. Referencing monero-project#882

coneiric · 2018-05-30T23:17:07Z

So, gcc says one thing and clang says another - but both are correct man_shrugging.

Right, I don't know either. Side-stepping the issue by using std::put_time.

Probably not smarter since the standard library would work fine with this function.

Agreed, I found a better way. Thanks for the hint.

Ok, then please just patch up this PR to work with windows, add a TODO to the function

I didn't mean that I wouldn't fully patch the issue, only explaining my reasoning for using snprintf. I'm still using the gmtime and struct tm from C, mostly because of the note about thread safety for std::gmtime in the reference material. If that's not a concern, I can replace gmtime and struct tm with the standard library counterparts.

For some reason the documentation on cppreference is incorrect for argument order with gmtime_s (causing the failing build). If you don't have any objections, I'm going to make the standard library replacements with std::gmtime and std::tm. Also has the side-benefit of getting rid of the preprocessor defines.

anonimal · 2018-05-30T23:30:31Z

Whatever you end up doing, a unit-test will be extremely helpful. Also please consider rebasing against master so you can eliminate unrelated build warnings.

coneiric · 2018-05-30T23:48:48Z

Whatever you end up doing, a unit-test will be extremely helpful. Also please consider rebasing against master so you can eliminate unrelated build warnings.

Agreed, will add a unit-test, and rebase against master.

edit: I made the changes, but will wait until fix-mingw-build is merged. The test for creating an invalid key causes a memory error without the check in Tag, and I don't want to bug the build bots.

Create routing key using standard library functions for memory safety. Move responsibility for error handling to callers. Referencing monero-project#882

rakhimov · 2018-06-03T07:04:35Z

src/core/router/identity.cc

+  tm.tm_year += 1900;
+  tm.tm_mon += 1;
+  ss << std::put_time(&tm, "%Y%m%d");
+  std::memcpy(reinterpret_cast<char *>((buf + 32)), ss.str().data(), 9);


I don't get 1. why this is not done with std::strftime
2. why platform specific gmtime_r/s ?

What is wrong with:

std::time_t current_time = std::time(nullptr); char time_string[9] = {}; auto ret = std::strftime(time_string, sizeof(time_string), "%Y%m%d", std::gmtime(&current_time));

This should work for both platforms.

I see that std::gmtime may not be thread-safe.
gmtime_s is C11 standard alternative for POSIX gmtime_r.

why this is not done with std::strftime

Good question :). I wasn't familiar with the function, and must have missed it when looking at the reference material.

I see that std::gmtime may not be thread-safe.

Originally, I had left those alternatives in for this reason, and will add them back. Thanks for the review @rakhimov.

After looking at the documentation, std::put_time makes more sense for use with streams. If arrays and memcpy are better, then I'll use your suggestion.

Ideally, I would like to get rid of the gmtime_* calls as well. Boost.DateTime seems the only viable thread-safe alternative, but looks like a lot of overhead for such a simple use-case. Maybe it's worth it...

Please consider doing whatever it takes to present a clean, streamlined, simple, efficient implementation of this function. A simple cross-platform thread-safe solution is welcome.

Will do, I'll try Boost.DateTime, and see if it comes out cleaner.

anonimal · 2018-06-05T04:11:24Z

src/core/router/identity.cc

  time_t t = time(nullptr);
  struct tm tm;
-  // TODO(unassigned): never use sprintf, use snprintf instead.
+  std::stringstream ss;
+  ss.imbue(std::locale(std::locale::classic()));


ss.imbue(std::locale(std::locale::classic()));

Why do you want this? I understand conceptually why but this doesn't appear necessary (and the router works fine without it in this branch).

this doesn't appear necessary

I realize now that this is the default, but wanted to ensure UTC with no localization leaks before.

anonimal · 2018-06-05T04:18:05Z

src/core/router/identity.cc

@@ -730,27 +730,23 @@ Keys CreateRandomKeys() {
 IdentHash CreateRoutingKey(
    const IdentHash& ident) {
  std::uint8_t buf[41];  // ident + yyyymmdd
-  memcpy(buf, (const std::uint8_t *)ident, 32);
+  std::memcpy(buf, (const std::uint8_t *)ident, 32);


This cast is unnecessary because of Tags functor.

Ok, I'll remove it in the next refactor, if I keep the memcpys.

anonimal · 2018-06-05T04:29:17Z

src/core/router/identity.cc

+  tm.tm_year += 1900;
+  tm.tm_mon += 1;
+  ss << std::put_time(&tm, "%Y%m%d");
+  std::memcpy(reinterpret_cast<char *>((buf + 32)), ss.str().data(), 9);


If you're going to keep the memcpy's then you should setup the empty buffer with ByteStream and refactor accordingly.

As for keeping a stringstream for time, that's TBD depending on what else you plan on doing.

If you're going to keep the memcpy's then you should setup the empty buffer with ByteStream and refactor accordingly.

Will do, I'll look at setting this up with ByteStream. Will try both refactors (memcpy and Boost.DateTime), and see which comes out cleaner.

Thanks for the review, and the pointers on where to go next 👍

coneiric · 2018-06-06T21:18:07Z

The latest set of changes includes Boost DateTime refactors, and refactors to CreateRoutingKey callers. I thought handling the errors internally, and returning a zeroed key made more sense.

Left off the reference to the PR, if throwing (or something else) is preferred.

anonimal

It's coming along. Two points in addition to the others:

Can you please drop all netdb additions. They should go in a separate PR.
Please update your submodule (rebase against master and, when switching branches, update the submodule) before committing

anonimal · 2018-06-06T21:58:49Z

src/core/router/identity.cc

+      return key;
+    }
+  std::stringstream ss;
+  ss.exceptions(std::ios::failbit | std::ios::badbit);


Why do you want this? This isn't necessary because we want to catch / process all exceptions.

I got confused from the Boost examples, and this excerpt from LWG 23.9.4, that I would need to also mask for badbit to enable the stringstream to rethrow all exceptions.

I also incorrectly assumed I would need to set exceptions for output, I'll remove the line.

anonimal · 2018-06-06T22:08:15Z

src/core/router/identity.cc

+      kovri::core::Exception ex(__func__);
+      ex.Dispatch();
+      IdentHash fail_key;
+      return fail_key;


IdentHash fail_key;
return fail_key;

We don't want to create an object simply to satisfy a return type. We should rather re-throw and deal with consequences in any callers.

Ok, that makes sense. I'll make the changes.

anonimal · 2018-06-06T22:13:11Z

tests/unit_tests/core/router/identity.cc

+  BOOST_CHECK_THROW(core::CreateRoutingKey(nullptr), std::invalid_argument);
+  BOOST_CHECK(core::CreateRoutingKey(zeroed_ident).IsZero());
+}
+


This test-case needs to ensure proper / spec-defined date formatting.

How can we inspect the date formatting without re-implementing the function, or is that what you mean? I'm confused...

How can we inspect the date formatting without re-implementing the function, or is that what you mean?

That would be expected if you wish to adequately test the data before it is hashed. Perhaps split into multiple functions (formatting versus key creation / hashing)?

anonimal · 2018-06-06T22:33:42Z

src/core/router/identity.cc

+    {
+      LOG(error) << __func__
+                 << ": cannot create routing key from zero-initialized ident";
+      return key;


We don't want to return an object simply to satisfy a return type. IsZero() was a hack to work with the union and should be eliminated / not built upon. We can keep IsZero for now but we should throw and deal with consequences in any callers.

Alright, that makes sense. I'll try to think of alternatives too (for another PR).

Create routing key using Boost DateTime for memory and thread safety. Callers are responsibile for error handling. Referencing monero-project#882

Separate routing key formatting from creation. Useful for testing proper formatting according to spec. Referencing monero-project#882

Create routing key using Boost DateTime for memory and thread safety. Callers are responsibile for error handling. Referencing monero-project#882

Separate routing key formatting from creation. Useful for testing proper formatting according to spec. Referencing monero-project#882

anonimal · 2018-06-11T21:09:25Z

src/core/router/identity.h

+/// @param ident Identity hash used as the basis for the routing key
+/// @throw std::invalid_argument if supplied a zero-initialized ident
+/// @throw std::ios_base::failure on date formatting failure
+/// @throw CryptoPP::Exception on hashing failure


/// @throw std::ios_base::failure on date formatting failure
/// @throw CryptoPP::Exception on hashing failure

We don't need this. We shouldn't keep track of every throw point that isn't directly related to the function's return or interface. Doing so only clutters the code and documentation, and is a pain to maintain should an implementation change.

Wasn't sure about the level of detail to include. I agree with you though, I'll remove it.

anonimal · 2018-06-11T21:10:11Z

src/core/router/identity.h

+/// @brief Format routing key with mod date according to spec
+/// @param ident Identity hash used as the basis for the routing key
+/// @throw std::invalid_argument if supplied a zero-initialized ident
+/// @throw std::ios_base::failure on date formatting failure


/// @throw std::ios_base::failure on date formatting failure

We don't need this. We shouldn't keep track of every throw point that isn't directly related to the function's return or interface. Doing so only clutters the code and documentation, and is a pain to maintain should an implementation change.

Wasn't sure about the level of detail to include. I agree with you though, I'll remove it.

anonimal · 2018-06-11T21:10:48Z

src/core/router/identity.h

+/// @throw std::invalid_argument if supplied a zero-initialized ident
+/// @throw std::ios_base::failure on date formatting failure
+std::string FormatRoutingKey(
+    const IdentHash& ident);


Is this really greater than 80 chars (it's 53, clang-format shouldn't do this)?

No, I'm not sure why the line broke here, I'll fix it.

anonimal · 2018-06-11T21:13:01Z

tests/unit_tests/core/router/identity.cc

+  BOOST_CHECK_NO_THROW(core::FormatRoutingKey(ident.GetIdentHash()));
+  const std::string formatted_key = core::FormatRoutingKey(ident.GetIdentHash());
+  const core::IdentHash formatted_ident(reinterpret_cast<const std::uint8_t*>(
+      formatted_key.substr(0, ident.GetIdentHash().size()).data()));


const std::string formatted_key = core::FormatRoutingKey(ident.GetIdentHash());
const core::IdentHash formatted_ident(reinterpret_cast<const std::uint8_t*>(
formatted_key.substr(0, ident.GetIdentHash().size()).data()));

This block is logically related to the comment below, not the check above. For sake of readability, you should put this block below the line // Ensure formatted routing key contains the original ident hash.

Ok, I must have missed that when making changes to the tests. I'll fix it.

anonimal · 2018-06-11T21:21:38Z

src/core/router/identity.cc

-    // TODO(anonimal): review if we need to safely break control, ensure exception handling by callers
-    throw;
-  }
+  try


Extend this try block to:

diff --git a/src/core/router/identity.cc b/src/core/router/identity.cc index ccd6c862..ed793bc2 100644 --- a/src/core/router/identity.cc +++ b/src/core/router/identity.cc @@ -725,16 +725,17 @@ Keys CreateRandomKeys() { return keys; } -IdentHash CreateRoutingKey( - const IdentHash& ident) { - if (ident.IsZero()) - { - throw std::invalid_argument( - "Identity: cannot create routing key from zero-initialized ident"); - } +IdentHash CreateRoutingKey(const IdentHash& ident) +{ IdentHash key; try { + if (ident.IsZero()) + { + throw std::invalid_argument( + "Identity: cannot create routing key from zero-initialized " + "ident"); + } const std::string formatted_key = FormatRoutingKey(ident); kovri::core::SHA256().CalculateDigest( key(),

I extended the try-block, but Github is still picking it up as unchanged.

anonimal · 2018-06-11T21:23:53Z

tests/unit_tests/core/router/identity.cc

+      ident.FromBuffer(m_AliceIdentity.data(), m_AliceIdentity.size()));
+
+  BOOST_CHECK_NO_THROW(core::FormatRoutingKey(ident.GetIdentHash()));
+  const std::string formatted_key = core::FormatRoutingKey(ident.GetIdentHash());


This case does not test for a valid format. You will have to supply an extra argument to the formatting function (a ptime object), and then process accordingly (ensuring that time given is properly formatted).

I used boost::gregorian::from_undelimited_string() below, which I thought would throw when supplied an invalid ISO date string. If you prefer an explicit ptime argument, I can try to implement that in the next set of refactors.

anonimal · 2018-06-11T22:26:56Z

src/core/router/identity.cc

  return key;
 }

+std::string FormatRoutingKey(const IdentHash& ident)


JFTR: our key is more akin to a vector of bytes and not a string. Changing this would then not justify all the stringstream use within the function. A note for future optimizations.

our key is more akin to a vector of bytes and not a string

I understand, and thought that using stringstream was the most straight-forward way to use the Boost.DateTime functions. Will try to use a vector of bytes instead on this next set of refactors. Thanks for the tips.

coneiric · 2018-06-12T17:54:26Z

@anonimal I made the latest recommended changes, and replaced uses of stringstream. If these changes away from stringstream use are unwanted, I can revert back.

Left off the commit references to this PR, just in case you do want me to revert.

coneiric · 2018-06-12T17:58:06Z

src/core/router/identity.cc

  return key;
 }

+std::vector<std::uint8_t> FormatRoutingKey(const IdentHash& ident)


I did not add the boost::gregorian::date object as a second parameter, since I thought passing it by non-const reference may lead to thread-unsafe usage. Instead, I extract the date and ident from the result in tests.

See revert comment.

anonimal · 2018-06-12T20:57:02Z

src/core/router/identity.cc

+  // Add date string to the formatted buffer
+  for (const auto& c : date_string)
+    formatted_buf.emplace_back(static_cast<std::uint8_t>(c));
+  return formatted_buf;


If these changes away from stringstream use are unwanted, I can revert back.

This is incredibly inefficient, worse than before. Please revert (and put some newlines in here so other people can more easily read the code).

Didn't intend to make it hard to read. Will think about other future refactors, and revert to stringstream for now.

anonimal

Please apply this patch:

diff --git a/src/core/router/identity.cc b/src/core/router/identity.cc
index 858b9ae5..cf7a01a6 100644
--- a/src/core/router/identity.cc
+++ b/src/core/router/identity.cc
@@ -725,25 +725,31 @@ Keys CreateRandomKeys() {
   return keys;
 }
 
-IdentHash CreateRoutingKey(
-    const IdentHash& ident) {
+// TODO(unassigned): should not be a free function
+IdentHash CreateRoutingKey(const IdentHash& ident)
+{
   IdentHash key;
   try
     {
       if (ident.IsZero())
-        {
-          throw std::invalid_argument(
-              "Identity: cannot create routing key from zero-initialized "
-              "ident");
-        }
+        throw std::invalid_argument("Identity: zero-initialized ident");
+
+      // Get spec-defined formatted date
+      std::string const date(GetFormattedDate());
+
+      // Create buffer for key
+      std::vector<std::uint8_t> buf(
+          ident(), ident() + (ident.size() + date.size()));
 
-      // Format the routing key
-      const std::vector<std::uint8_t> buf = FormatRoutingKey(ident);
-      // Hash the formatted buffer to create the routing key
+      // Append the ISO date
+      buf.insert(buf.end(), date.begin(), date.end());
+
+      // Hash the key
+      assert(key.size() <= buf.size());
       kovri::core::SHA256().CalculateDigest(key(), buf.data(), buf.size());
     }
   catch (...)
-    {  // Error formatting or hashing the key
+    {
       kovri::core::Exception ex(__func__);
       ex.Dispatch();
       throw;
@@ -751,29 +757,11 @@ IdentHash CreateRoutingKey(
   return key;
 }
 
-std::vector<std::uint8_t> FormatRoutingKey(const IdentHash& ident)
+// TODO(unassigned): should not be a free function
+std::string GetFormattedDate()
 {
-  if (ident.IsZero())
-    {
-      throw std::invalid_argument(
-          "Identity: cannot create routing key from zero-initialized ident");
-    }
-
-  // Get current UTC date
   boost::gregorian::date date(boost::gregorian::day_clock::universal_day());
-  const std::string date_str = boost::gregorian::to_iso_string(date);
-
-  std::vector<std::uint8_t> buf;
-  buf.reserve(ident.size() + date_str.size());
-
-  // Append the ident
-  for (std::size_t i = 0; i < ident.size(); ++i)
-    buf.push_back(ident[i]);
-
-  // Append the ISO date
-  buf.insert(buf.end(), date_str.begin(), date_str.end());
-
-  return buf;
+  return boost::gregorian::to_iso_string(date);
 }
 
 XORMetric operator^(
diff --git a/src/core/router/identity.h b/src/core/router/identity.h
index 6743b922..ee82a02b 100644
--- a/src/core/router/identity.h
+++ b/src/core/router/identity.h
@@ -292,15 +292,16 @@ struct XORMetric {
   }
 };
 
+// TODO(unassigned): should not be a free function
 /// @brief Hash identity with appended mod date according to spec
 /// @param ident Identity hash used as the basis for the routing key
 /// @throw std::invalid_argument if supplied a zero-initialized ident
 IdentHash CreateRoutingKey(const IdentHash& ident);
 
-/// @brief Format routing key with mod date according to spec
-/// @param ident Identity hash used as the basis for the routing key
-/// @throw std::invalid_argument if supplied a zero-initialized ident
-std::vector<std::uint8_t> FormatRoutingKey(const IdentHash& ident);
+// TODO(unassigned): should not be a free function
+/// @return Formatted UTC date used in routing key
+/// @notes Must return yyyyMMdd
+std::string GetFormattedDate();
 
 XORMetric operator^(
     const IdentHash& key1,
diff --git a/tests/unit_tests/core/router/identity.cc b/tests/unit_tests/core/router/identity.cc
index 1beb829c..6acacf0d 100644
--- a/tests/unit_tests/core/router/identity.cc
+++ b/tests/unit_tests/core/router/identity.cc
@@ -35,6 +35,7 @@
 
 #include <array>
 #include <memory>
+#include <regex>
 
 #include "core/crypto/signature.h"
 #include "core/router/identity.h"
@@ -86,63 +87,24 @@ BOOST_AUTO_TEST_CASE(ParseIdentityFailure)
         0);
 }
 
-BOOST_AUTO_TEST_CASE(ValidCreateRoutingKey)
+BOOST_AUTO_TEST_CASE(ValidRoutingKey)
 {
   core::IdentityEx ident;
-  BOOST_CHECK(
-      ident.FromBuffer(m_AliceIdentity.data(), m_AliceIdentity.size()));
+  BOOST_CHECK(ident.FromBuffer(m_AliceIdentity.data(), m_AliceIdentity.size()));
   BOOST_CHECK_NO_THROW(core::CreateRoutingKey(ident.GetIdentHash()));
-  // Ensure a valid identity hash is returned
-  BOOST_CHECK(core::CreateRoutingKey(ident.GetIdentHash()).ToBase32().size());
 }
 
-BOOST_AUTO_TEST_CASE(CreateInvalidRoutingKey)
+BOOST_AUTO_TEST_CASE(InvalidRoutingKey)
 {
-  kovri::core::IdentHash zeroed_ident;
+  kovri::core::IdentHash hash;
   BOOST_CHECK_THROW(core::CreateRoutingKey(nullptr), std::invalid_argument);
-  BOOST_CHECK_THROW(core::CreateRoutingKey(zeroed_ident), std::invalid_argument);
-}
-
-BOOST_AUTO_TEST_CASE(ValidFormatRoutingKey)
-{
-  core::IdentityEx ident;
-  BOOST_CHECK(
-      ident.FromBuffer(m_AliceIdentity.data(), m_AliceIdentity.size()));
-
-  // Ensure no thrown errors when formatting a valid key
-  BOOST_CHECK_NO_THROW(core::FormatRoutingKey(ident.GetIdentHash()));
-  auto formatted_key = core::FormatRoutingKey(ident.GetIdentHash());
-
-  // Extract the date from the formatted key
-  std::vector<std::uint8_t> date_vec;
-  // Set the capacity for the expected date size
-  date_vec.reserve(formatted_key.size() - ident.GetIdentHash().size());
-
-  for (std::size_t i = 0; i < date_vec.capacity(); ++i)
-    {  // insert the date in reverse order
-      date_vec.emplace(date_vec.begin(), formatted_key.back());
-      formatted_key.pop_back();
-    }
-  const std::string date_string(date_vec.begin(), date_vec.end());
-
-  // Ensure the date is a proper UTC date
-  BOOST_CHECK_NO_THROW(boost::gregorian::from_undelimited_string(date_string));
-  BOOST_CHECK(
-      !boost::gregorian::from_undelimited_string(date_string).is_not_a_date());
-
-  // Formatted key should now only contain the original ident
-  const core::IdentHash formatted_ident(formatted_key.data());
-
-  // Ensure extracted key matches the original ident
-  BOOST_CHECK(ident.GetIdentHash() == formatted_ident);
+  BOOST_CHECK_THROW(core::CreateRoutingKey(hash), std::invalid_argument);
 }
 
-BOOST_AUTO_TEST_CASE(InvalidFormatRoutingKey)
+BOOST_AUTO_TEST_CASE(ValidDateFormat)
 {
-  core::IdentHash ident;
-  BOOST_CHECK_THROW(core::FormatRoutingKey(nullptr), std::invalid_argument);
-  // Ensure formatting a zero-initialized ident hash throws
-  BOOST_CHECK_THROW(core::FormatRoutingKey(ident), std::invalid_argument);
+  std::regex regex("(20\\d{2})(\\d{2})(\\d{2})");  // Valid for only this century
+  BOOST_CHECK(std::regex_search(core::GetFormattedDate(), regex));
 }
 
 BOOST_AUTO_TEST_SUITE_END()

Create routing key using Boost DateTime for memory and thread safety. Callers are responsibile for error handling. Referencing monero-project#882

Tests for creating and formatting routing keys. Referencing monero-project#882

ebebffc Tests: CreateRoutingKey unit-tests (oneiric) e038980 Identity: use Boost DateTime to create routing key (oneiric)

anonimal reviewed May 30, 2018

View reviewed changes

coneiric force-pushed the snprintf-create-routing-key branch 2 times, most recently from 937767e to 78354e4 Compare May 30, 2018 23:05

coneiric added a commit to coneiric/kovri that referenced this pull request May 30, 2018

Identity: use std library to create routing key

78354e4

Create routing key using standard library functions for memory safety. Referencing monero-project#882

coneiric force-pushed the snprintf-create-routing-key branch from 78354e4 to 4753687 Compare May 30, 2018 23:11

coneiric added a commit to coneiric/kovri that referenced this pull request May 30, 2018

Identity: use std library to create routing key

4753687

Create routing key using standard library functions for memory safety. Referencing monero-project#882

coneiric changed the title ~~Identity: use snprintf to create routing key~~ Identity: use standard library to create routing key May 31, 2018

rakhimov reviewed Jun 3, 2018

View reviewed changes

anonimal suggested changes Jun 5, 2018

View reviewed changes

coneiric force-pushed the snprintf-create-routing-key branch from 4753687 to f6b2046 Compare June 6, 2018 21:09

anonimal suggested changes Jun 6, 2018

View reviewed changes

coneiric changed the title ~~Identity: use standard library to create routing key~~ Identity: use Boost DateTime when creating router keys Jun 7, 2018

coneiric force-pushed the snprintf-create-routing-key branch 2 times, most recently from 7fe3e12 to 25b1e9d Compare June 8, 2018 17:20

coneiric added a commit to coneiric/kovri that referenced this pull request Jun 8, 2018

Identity: use Boost DateTime to create routing key

6b86e60

Create routing key using Boost DateTime for memory and thread safety. Callers are responsibile for error handling. Referencing monero-project#882

coneiric added a commit to coneiric/kovri that referenced this pull request Jun 8, 2018

Identity: format routing key

df7f43b

Separate routing key formatting from creation. Useful for testing proper formatting according to spec. Referencing monero-project#882

coneiric force-pushed the snprintf-create-routing-key branch from 25b1e9d to ba20fb2 Compare June 8, 2018 21:00

coneiric added a commit to coneiric/kovri that referenced this pull request Jun 8, 2018

Identity: use Boost DateTime to create routing key

de0aa11

Create routing key using Boost DateTime for memory and thread safety. Callers are responsibile for error handling. Referencing monero-project#882

coneiric added a commit to coneiric/kovri that referenced this pull request Jun 8, 2018

Identity: format routing key

6d3b237

Separate routing key formatting from creation. Useful for testing proper formatting according to spec. Referencing monero-project#882

anonimal suggested changes Jun 11, 2018

View reviewed changes

anonimal reviewed Jun 11, 2018

View reviewed changes

coneiric force-pushed the snprintf-create-routing-key branch from ba20fb2 to bb50ea3 Compare June 12, 2018 04:55

coneiric commented Jun 12, 2018

View reviewed changes

anonimal suggested changes Jun 12, 2018

View reviewed changes

coneiric force-pushed the snprintf-create-routing-key branch 2 times, most recently from deb08e3 to 44b3ec8 Compare June 13, 2018 22:12

anonimal suggested changes Jun 13, 2018

View reviewed changes

coneiric force-pushed the snprintf-create-routing-key branch 2 times, most recently from b15b381 to e4cda16 Compare June 13, 2018 23:13

Identity: use Boost DateTime to create routing key

e038980

Create routing key using Boost DateTime for memory and thread safety. Callers are responsibile for error handling. Referencing monero-project#882

coneiric force-pushed the snprintf-create-routing-key branch from e4cda16 to ebebffc Compare June 13, 2018 23:50

Tests: CreateRoutingKey unit-tests

ebebffc

Tests for creating and formatting routing keys. Referencing monero-project#882

anonimal changed the title ~~Identity: use Boost DateTime when creating router keys~~ Identity: refactor routing key creation Jun 13, 2018

anonimal approved these changes Jun 14, 2018

View reviewed changes

anonimal merged commit ebebffc into monero-project:master Jun 14, 2018

anonimal added a commit that referenced this pull request Jun 14, 2018

Merge pull request #882

2e19376

ebebffc Tests: CreateRoutingKey unit-tests (oneiric) e038980 Identity: use Boost DateTime to create routing key (oneiric)

coneiric deleted the snprintf-create-routing-key branch July 13, 2018 03:06

Identity: refactor routing key creation #882

Identity: refactor routing key creation #882

Conversation

coneiric commented May 30, 2018 • edited Loading

anonimal left a comment

Choose a reason for hiding this comment

coneiric commented May 30, 2018

anonimal commented May 30, 2018

coneiric commented May 30, 2018 • edited Loading

anonimal commented May 30, 2018

coneiric commented May 30, 2018 • edited Loading

rakhimov Jun 3, 2018 • edited Loading

Choose a reason for hiding this comment

rakhimov Jun 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coneiric commented Jun 6, 2018

anonimal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coneiric Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coneiric commented Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anonimal left a comment

Choose a reason for hiding this comment

coneiric commented May 30, 2018 •

edited

Loading

coneiric commented May 30, 2018 •

edited

Loading

coneiric commented May 30, 2018 •

edited

Loading

rakhimov Jun 3, 2018 •

edited

Loading

rakhimov Jun 3, 2018 •

edited

Loading

coneiric Jun 6, 2018 •

edited

Loading

coneiric commented Jun 12, 2018 •

edited

Loading