Concept ACK

I'm not convinced this is an improvement. Sure, it's a trivial function, but by having it this way there is symmetry with the V2 version of this function (I assume that was the reason for writing it this way).

I would agree, but I don't see a V2 version of this function.

There is a SerializeV1Array(), though, so that could be a case for not changing this. Either way is fine with me.

I think it's okay to get rid of this intermediate function. The signature uint8_t (&arr)[V1_SERIALIZATION_SIZE] looks really strange to me.

Code review ACK eaa6aae

cr ACK eaa6aae

Agree with @laanwj

The reasoning is to have pairs of ser/unser functions, e.g. SerializeFoo() / UnserializeFoo() and have UnserializeFoo(SerializeFoo(x)) == x. This makes the code easier to verify visually and easier to modify - should one of those be altered, it is clear that a corresponding reverse mod should be applied to the other.

We currently have SerializeV1Array() which is not going away. Thus it would be nice to have UnserializeV1Array() (ie not to remove it in this PR).

There is no ser/unser for V2 to/from arrays because V2 length is variable.

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• Ignore banlist.dat #22570 by MarcoFalke

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

We currently have SerializeV1Array() which is not going away. Thus it would be nice to have UnserializeV1Array() (ie not to remove it in this PR).

OK makes sense, let's close this then. I think API symmetry can be important but hadn't noticed it's an issue here.

SGTM