LNS: Work-around lns::generic_owner hashing bug in rpc

[Doy-lee]

      // TODO(loki): Appears to be some bug overloading std::hash and using
      // std::map<lns::generic_owner, size_t> causing hash collisions
      // and overwriting the same entry every time, so manually create and store
      // the hash gives us the correct result.

      // But, we now also just serialize both owner and backup_owner, since if
      // we specify an owner that is backup owner, we don't show the (other)
      // owner. This makes the fix redundant but for RPC compatibility we keep
      // it around until the next hard fork (16)

[jagerman]

A std::map does insertion sorting, no hashing at all. (But for it to work, a < b must work).

[jagerman]

Ouch, I see why the existing code even compiles -- because generic_owner has this:

operator bool() const { ... }

which is non-explicit so that when std::map needs to compare elements when it does a a < b comparison the only workable solution is doing implicit conversion to bool on both sides.

[Doy-lee]

That explains everything :(

[jagerman]

generic_signature also has an operator bool that should be made explicit while you're there.

[jagerman]

Also a side note in passing: with unordered_map/unordered_set, even if you get hash collisions from a poor hash you still don't overwrite values, you just get degraded performance because you end up with one bucket containing lots of values rather than values spread across the available buckets. Worst case: your hash functions returns a constant for all values and your unordered map/set degrades into a linear search.

[Doy-lee]

Added explicit to operator bool(). Decided to keep the std::map and manually hash as its temporary, then deprecate in hard fork 16.

[jagerman]

This looks fine, except for this part:

Decided to keep the std::map and manually hash

With the std::hash specialization already here, an unordered_map here seems an obviously better choice: hash collisions won't be destructive (they are with std::map + hash as keys), the code is slightly simpler (you can eliminate the hash call), and performance is better (map is an insertion sorted container, thus O(NlogN) on N insertions).

Also yesterday I came across this variable and comment in daemon/core.h, committed more than 4 years ago:

  // TEMPORARY HACK - Yes, this creates a copy, but otherwise the original
  // variable map could go out of scope before the run method is called
  boost::program_options::variables_map const m_vm_HACK;

These sorts of things have a way of unintentionally surviving.

[jagerman]

This should be an unordered_map (but keeping the std::hash specialization). This isn't just an optimal data structure thing: using a map here has a potential (if rare) data omission on collision. (And switching to an unordered_map is a nearly trivial change).

[Doy-lee]

Ok, updated