Improve netaddress implementation #11231

Making most of the implementation simpler and safer.

please don't add another boost dependency for a simple equality comparison.

@gmaxwell There is already a dependency on this boost library in other places in the code. It doesn't have a standard equivalent, and it makes the code clearer here.

@danra we are working on eliminating boost, and adding the additional header here visibly slows down the compile (30% longer for netaddress.cpp). If it were for something more complex than a match there might be an argument to use it for now and replace later, but it's just a string comparison and should be doable in one fairly clean and idiomatic line (slice and match; or reverse iterators).

@gmaxwell If you don't mind, I'd be happy to discuss this a bit, since I think the approach of just "eliminating boost" is wrong.

First of all, I don't that goal listed under the github "Projects" - I only see a goal of transitioning from boost to C++11. If removing boost in general from the project is a goal, even for cases where there is no standard substitute, I suppose it should be listed under "Projects" as well.

Second, I think a reasonable compilation time increase is worth improving code clarity. Consider the "endswith" functionality here:

• checking it inline instead of using a function requires a couple of if conditions, making it less readable and more error prone, which also means harder to review and less secure. One reading the code has only so much of an attention span, and it's bad to force details which are unnecessary in the context - implementing "endswith" inline is certainly out of the scope of the code where it is used.
• Writing our own function is for the bitcoin repo in some utility header is fine, but it means a) using less tested code than the one boost uses and b) just more work which could be spent on more useful things.

@gmaxwell Do you have a specific suggestion for an elegant one-liner? I don't see one which doesn't involve also checking the size, or doing some min() with it

I don't see anything wrong with

template<typename T>
bool BeginsWith(const T& a, const T& b)
{
    return a.size() >= b.size() && std::equal(b.begin(), b.end(), a.begin());
}

template<typename T>
bool EndWith(const T& a, const T& b)
{
    return a.size() >= b.size() && std::equal(b.begin(), b.end(), a.end() - b.size());
}

@danra It's written on there: The long term future goal is to remove Boost as a dependency.

Should this be using .size() everywhere on the std::arrays instead of sizeof?

@MeshCollider You're right. I think it's a bad goal, though (unlike boost->C++11 migration under which it's listed). For instance, is Bitcoin Core really interested in rewriting boost::multi_index?

@jimpo I thought sizeof would be the same (and compile-time evaluable) since std::array is an aggregate type, but it seems that is not guaranteed. Will fix, thanks!

@sipa It's not wrong, but this sort of utility functions tend to pile up, and I think it's better to use well-tested (e.g. boost provided) utility functions where it's available, rather than maintain them yourself, even if the cost is some compilation time. It makes the codebase easier to maintain and review -> more secure.

@danra That's a fair point, though I personally think that a boost dependency is worse than the cost of maintaining such a simple function.

@gmaxwell @sipa Reverted boost dependency, due to both your review and the fact that using the boost function changed the semantics slightly - the ".onion" address (with no preceding characters) was previously rejected immediately by the condition, which is probably safer than the situation after my modification which resulted in passing an empty string to DecodeBase32 (that function should handle empty strings correctly, but it's probably better to code defensively) @jimpo Fixed sizeof->size() (hopefully that would also fix onion address-related tests failing on some platforms :)

Closing due to inactivity, let me know if you start work on this again and want to re-open this.