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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
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.
4384 | @@ -4384,7 +4385,7 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida 4385 | } 4386 | 4387 | // Exposed wrapper for AcceptBlockHeader 4388 | -bool ChainstateManager::ProcessNewBlockHeaders(const std::vector<CBlockHeader>& headers, bool min_pow_checked, BlockValidationState& state, const CBlockIndex** ppindex) 4389 | +bool ChainstateManager::ProcessNewBlockHeaders(const std::span<CBlockHeader> headers, bool min_pow_checked, BlockValidationState& state, const CBlockIndex** ppindex)
I think you want std::span<const CBlockHeader> for these. In addition to preserving constness, it allows elements to be passed in as {{foo}}, rather than specifying spans everywhere.
Concept ACK.
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?
Thanks for the review @theuni!
Updated 65e440433e7223511a4effb568cdb187fa0aa22c -> c584c1f23b8107dade11d81ada2baf741acbac62 (headersSpan_0 -> headersSpan_1, compare)
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.
4384 | @@ -4384,7 +4385,7 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida 4385 | } 4386 | 4387 | // Exposed wrapper for AcceptBlockHeader 4388 | -bool ChainstateManager::ProcessNewBlockHeaders(const std::vector<CBlockHeader>& headers, bool min_pow_checked, BlockValidationState& state, const CBlockIndex** ppindex) 4389 | +bool ChainstateManager::ProcessNewBlockHeaders(const std::span<const CBlockHeader> headers, bool min_pow_checked, BlockValidationState& state, const CBlockIndex** ppindex)
No need to pass as const.
foo(std::span<const CBlockHeader>) is functionally equivalent to foo(const std::vector<CBlockHeader>&).
Whoops, missed that, thanks!
Updated c584c1f23b8107dade11d81ada2baf741acbac62 -> 45a90050290b046a8c0f417eab679f36032da297 (headersSpan_1 -> headersSpan_2, compare)
5067 | @@ -5068,7 +5068,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, 5068 | mapBlockSource.emplace(hash, std::make_pair(pfrom.GetId(), true)); 5069 | 5070 | // Check claimed work on this block against our anti-dos thresholds. 5071 | - if (prev_block && prev_block->nChainWork + CalculateClaimedHeadersWork({pblock->GetBlockHeader()}) >= GetAntiDoSWorkThreshold()) { 5072 | + auto header{pblock->GetBlockHeader()}; 5073 | + if (prev_block && prev_block->nChainWork + CalculateClaimedHeadersWork({{header}}) >= GetAntiDoSWorkThreshold()) {
nit, as this is only used inside the conditional, may as well keep the scope somaller
- auto header{pblock->GetBlockHeader()};
- if (prev_block && prev_block->nChainWork + CalculateClaimedHeadersWork({{header}}) >= GetAntiDoSWorkThreshold()) {
+ if (prev_block && prev_block->nChainWork + CalculateClaimedHeadersWork({{pblock->GetBlockHeader()}}) >= GetAntiDoSWorkThreshold()) {
ACK 45a90050290b046a8c0f417eab679f36032da297
Thanks for the review @jonatack.
Updated 45a90050290b046a8c0f417eab679f36032da297 -> 797eede7609ca0d4d788e62f35e70e85103a19dc (headersSpan_2 -> headersSpan_3, compare)
ACK 797eede7609ca0d4d788e62f35e70e85103a19dc
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).
ACK 797eede7609ca0d4d788e62f35e70e85103a19dc
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?
<!-- https://godbolt.org/z/9znj6axv1
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.
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?
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.
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.
797eede7609ca0d4d788e62f35e70e85103a19dc -> b31483e32b7d111eeef5fea8ebdc927a32c22bf4 (headersSpan_3 -> headersSpan_4, compare)
std::span in ImportBlocks.
This changes vector to span, and where copies were done previously, they are still done.
ACK b31483e32b7d111eeef5fea8ebdc927a32c22bf4 🖍
<details><summary>Show signature</summary>
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==
</details>
429 | @@ -429,7 +430,7 @@ class BlockManager 430 | void CleanupBlockRevFiles() const; 431 | }; 432 | 433 | -void ImportBlocks(ChainstateManager& chainman, std::vector<fs::path> vImportFiles); 434 | +void ImportBlocks(ChainstateManager& chainman, std::span<const fs::path> vImportFiles);
nit: would be nice to modernize the name to e.g. import_paths since it's no longer a vector.
void ImportBlocks(ChainstateManager& chainman, std::span<const fs::path> import_paths);
Oh right, finally!
ACK b31483e32b7d111eeef5fea8ebdc927a32c22bf4
b31483e32b7d111eeef5fea8ebdc927a32c22bf4 -> a2955f09792b6232f3a45aa44a498b466279a8b7 (headersSpan_4 -> headersSpan_5, compare)
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.
re-ACK a2955f09792b6232f3a45aa44a498b466279a8b7 - no changes except further walking the ~file~ path of modernizing variable names.
ACK a2955f09792b6232f3a45aa44a498b466279a8b7 🕑
<details><summary>Show signature</summary>
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==
</details>
ACK a2955f09792b6232f3a45aa44a498b466279a8b7
CI failure looks unrelated and can be ignored: https://github.com/bitcoin/bitcoin/issues/30797
ACK a2955f09792b6232f3a45aa44a498b466279a8b7
