Why not just

for(auto& share: m_client_shares)
  offered_groups.push_back(share.group());

Fair. Now that we have C++20 and ranges I thought that's a good opportunity to use it. For reference:

return m_client_shares
   | std::views::transform([](const auto& s){ return s.group(); })
   | to<std::vector<Named_Group>>();

Caveat: the to<>() is actually proposed for C++23 only. We'd need to implement it ourselves; which seems like a reasonable thing to do for the time being. Sigh!
Godbolt: https://godbolt.org/z/771M7PEzK

But certainly not in this PR.

I guess the modern thing to do, is to return the view directly and not create a new owning container at all. These things should happen at least somewhat consistently, though.

Should we ignore (or reject?) DTLS versions being mixed in here? Or does that happen elsewhere already.

Perhaps for RFC 8446 purposes DTLS counts as an "unknown version" of TLS but that doesn't really seem correct to me.

DTLS as such is not explicitly filtered out anywhere. TLS 1.2 / 1.3 distinctions are typically done using Protocol_Version::is_pre_tls_13() or ::is_tls_13_or_later() which transparently takes DTLS into account.

That said, DTLS 1.3 won't pass the default implementation of Policy::acceptable_protocol_version() and won't make it into the result vector. For reference:

bool Policy::acceptable_protocol_version(Protocol_Version version) const
   {
#if defined(BOTAN_HAS_TLS_13)
   if(version == Protocol_Version::TLS_V13 && allow_tls13())
      return true;
#endif

#if defined(BOTAN_HAS_TLS_12)
   if(version == Protocol_Version::TLS_V12 && allow_tls12())
      return true;

   if(version == Protocol_Version::DTLS_V12 && allow_dtls12())
      return true;
#endif

   return false;
   }

Why this change? Fine with it just curious why it was required.

The c'tor of Server_Hello_Internal takes the random as a std::vector (which is usually provided from make_hello_random(), that returns a std::vector). Leaving this as a std::array would require a conversion or a c'tor overload just for that.

std::make_unique<Server_Hello_Internal>(.... , HELLO_RETRY_REQUEST_MARKER, ....);
// vs.
std::make_unique<Server_Hello_Internal>(.... , make_hello_random(), ....);

Or use a std::span in the Server_Hello_Internal c'tor? But wouldn't that cause UB because the std::span would refer to data in the temporary std::vector falling out of make_hello_random()?

My understanding is, that the temporary gets destroyed at the end of the expression (i.e. after the invoked c'tor returned). So as long as the c'tor copies the data out of the span, there should be no lifetime issues. This quick test does support my view. Side note: An interesting article about views, ranges, life times and dangling pointers: https://tristanbrindle.com/posts/rvalue-ranges-and-views

But it would result in an unnecessary copy (from the temporary, via the span, into a member of Server_Hello_Internal). I think, my vote is to keep the std::vector<> for HELLO_RETRY_REQUEST_MARKER.