kernel: Use spans instead of vectors for passing block headers to validation functions

[sedited]

Makes it friendlier for potential future users of the kernel library if they do not store the headers in a std::vector, but can guarantee contiguous memory.

Take this opportunity to also change the argument of ImportBlocks previously taking a std::vector to a std::span.

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, maflcko, danielabrozzoni, achow101
Concept ACK	theuni
Stale ACK	jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

[theuni]

Concept ACK.

[theuni]

Actually, I read this assuming we'd already migrated away from Span, which isn't the case. So probably best not to start introducing std::spans until that's happened?

[theuni]

Relevant: #29119

[sedited]

Thanks for the review @theuni!

Updated 65e4404 -> c584c1f (headersSpan_0 -> headersSpan_1, compare)

• Addressed @theuni's comment, made the span take a const CBlockHeader, which allows dropping the annoying std::span invocations again.

Actually, I read this assuming we'd already migrated away from Span, which isn't the case. So probably best not to start introducing std::spans until that's happened?

There already are users of std::span, so I did not have the impression that I was introducing something novel here. Now that #29071 is merged I would also expect #29119 to move forward soon. But can always revert to Span if that is what reviewers prefer.

[theuni]

Now that #29071 is merged I would also expect #29119 to move forward soon. But can always revert to Span if that is what reviewers prefer.

@maflcko ?

[sedited]

Updated c584c1f -> 45a9005 (headersSpan_1 -> headersSpan_2, compare)

• Addressed @theuni's comment, dropped superfluous const.

[jonatack]

ACK 45a9005

[sedited]

Thanks for the review @jonatack.

Updated 45a9005 -> 797eede (headersSpan_2 -> headersSpan_3, compare)

• Addressed @jonatack's comment, dropped now unneeded temporaries.

[jonatack]

ACK 797eede

Doesn't conflict with the changes in #29119 and LGTM provided we're migrating to std::span and modulo the tidy CI iwyu checks on the updated files (the task hasn't run yet at the time of writing this).

[stickies-v]

ACK 797eede

[maflcko]

Avoids the need for copying by having to wrap the header in a std::vector when just a single block header is passed.

Can you explain this a bit more? In theory there is C++26 with https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2447r6.html , but I don't think any compiler implements it and the code here is compiled in C++20, so I haven't checked if that fixes your problem.

Currently in the code, you are even creating explicit copies via the GetBlockHeader method.

Maybe I am missing something obvious?

[sedited]

but I don't think any compiler implements it and the code here is compiled in C++20, so I haven't checked if that fixes your problem.

Thanks, you are right, my initial patch avoided copies where possible by explicitly creating the span, but the brace initializers that theuni suggested instead create temporary arrays that copy the headers. I should update the description, or I can revert to what I was doing before. Do you have a preference? I don't think the copies matter all that much either way. The main motiviation for me is that it makes life a bit easier for potential external callers if you use spans over vector references in the interfaces.

[maflcko]

I think your initial commit also had some calls to GetBlockHeader to create a copy (explicitly), but I agree that it doesn't matter much either way.

Maybe remove it from the motivation, since it doesn't seem to be a goal, and if it was a goal, it would be hard to achieve?

[sedited]

797eede -> b31483e (headersSpan_3 -> headersSpan_4, compare)

• Corrected commit message and pull request description, dropping mentions of copies as per @maflcko's suggestion.
• Added a commit for also moving to std::span in ImportBlocks.

[maflcko]

This changes vector to span, and where copies were done previously, they are still done.

ACK b31483e 🖍

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK b31483e32b7d111eeef5fea8ebdc927a32c22bf4 🖍
KluDd3R0iPVwSdNpepkr2LEtfCpZ259JdLgMHassQji86noEGgOSkRAX6C0unxwVFzvv9ROsLYzZd5fV2XJGDw==

[stickies-v]

ACK b31483e

[sedited]

b31483e -> a2955f0 (headersSpan_4 -> headersSpan_5, compare)

• Modernizing naming as suggested by @stickies-v's comment.

[stickies-v]

re-ACK a2955f0 - no changes except further walking the file path of modernizing variable names.

[maflcko]

ACK a2955f0 🕑

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK a2955f09792b6232f3a45aa44a498b466279a8b7 🕑
lTCtNlp6GgzlMVUtGi0UeE37JlHEPqVxzu84w9qah6aDOvdY6VVedlJf9Dorj/Kjkm8MX2kvBnBkv+eI9aifAQ==

[danielabrozzoni]

ACK a2955f0

[maflcko]

CI failure looks unrelated and can be ignored: #30797

[achow101]

ACK a2955f0