💬 theuni commented on pull request "ci: Use C++23 in one task":
(https://github.com/bitcoin/bitcoin/pull/30735#issuecomment-2315907989)
Fwiw `APPEND_CXXFLAGS` was added specifically for cases like this where CMake sets a flag internally that we want to override.

💬 github12101 commented on issue "b-msghand[4988] general protection fault":
(https://github.com/bitcoin/bitcoin/issues/30706#issuecomment-2315941447)
Thanks for fantastic instructions! I was able to trip bitcoind, and do full backtrace with proper symbols. Thanks!
I will now await another crash, using upgraded bitcoin core 27.1. It may not happen anytime soon :)

💬 TheCharlatan commented on pull request "kernel: Use spans instead of vectors for passing block headers to validation functions":
(https://github.com/bitcoin/bitcoin/pull/30742#issuecomment-2315942210)
Thanks for the review @theuni!

Updated 65e440433e7223511a4effb568cdb187fa0aa22c -> c584c1f23b8107dade11d81ada2baf741acbac62 ([headersSpan_0](https://github.com/TheCharlatan/bitcoin/tree/headersSpan_0) -> [headersSpan_1](https://github.com/TheCharlatan/bitcoin/tree/headersSpan_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/headersSpan_0..headersSpan_1))

* Addressed @theuni's [comment](https://github.com/bitcoin/bitcoin/pull/30742#discussion_r1735049626), made the span take a
...

🤔 theuni reviewed a pull request: "ci: Use C++23 in one task"
(https://github.com/bitcoin/bitcoin/pull/30735#pullrequestreview-2266924928)
Concept ACK.

If we wanted, we could actually start conditionally enabling c++23 features for these builds. For byteswap, something like:
```c++
#if __cplusplus >= 202302L && defined(__cpp_lib_byteswap) && __cpp_lib_byteswap >= 202110L
# define bitcoin_builtin_bswap16(x) std::byteswap(x)
# define bitcoin_builtin_bswap32(x) std::byteswap(x)
# define bitcoin_builtin_bswap64(x) std::byteswap(x)
# elif defined __has_builtin
...
```

Another I've found useful while working
...

💬 theuni commented on pull request "kernel: Use spans instead of vectors for passing block headers to validation functions":
(https://github.com/bitcoin/bitcoin/pull/30742#discussion_r1735095909)
No need to pass as const.
`foo(std::span<const CBlockHeader>)` is functionally equivalent to `foo(const std::vector<CBlockHeader>&)`.

💬 theuni commented on pull request "kernel: Use spans instead of vectors for passing block headers to validation functions":
(https://github.com/bitcoin/bitcoin/pull/30742#issuecomment-2315967003)
> 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 ?

💬 TheCharlatan commented on pull request "kernel: Use spans instead of vectors for passing block headers to validation functions":
(https://github.com/bitcoin/bitcoin/pull/30742#discussion_r1735111117)
Whoops, missed that, thanks!

💬 TheCharlatan commented on pull request "kernel: Use spans instead of vectors for passing block headers to validation functions":
(https://github.com/bitcoin/bitcoin/pull/30742#issuecomment-2315985818)
Updated c584c1f23b8107dade11d81ada2baf741acbac62 -> 45a90050290b046a8c0f417eab679f36032da297 ([headersSpan_1](https://github.com/TheCharlatan/bitcoin/tree/headersSpan_1) -> [headersSpan_2](https://github.com/TheCharlatan/bitcoin/tree/headersSpan_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/headersSpan_1..headersSpan_2))

* Addressed @theuni's [comment](https://github.com/bitcoin/bitcoin/pull/30742#discussion_r1735095909), dropped superfluous const.

💬 jonatack commented on pull request "ci: Use C++23 in one task":
(https://github.com/bitcoin/bitcoin/pull/30735#issuecomment-2315988336)
Concept ACK

🤔 jonatack reviewed a pull request: "kernel: Use spans instead of vectors for passing block headers to validation functions"
(https://github.com/bitcoin/bitcoin/pull/30742#pullrequestreview-2266989327)
ACK 45a90050290b046a8c0f417eab679f36032da297

💬 jonatack commented on pull request "kernel: Use spans instead of vectors for passing block headers to validation functions":
(https://github.com/bitcoin/bitcoin/pull/30742#discussion_r1735122911)
nit, as this is only used inside the conditional, may as well keep the scope somaller

```diff
- auto header{pblock->GetBlockHeader()};
- if (prev_block && prev_block->nChainWork + CalculateClaimedHeadersWork({{header}}) >= GetAntiDoSWorkThreshold()) {
+ if (prev_block && prev_block->nChainWork + CalculateClaimedHeadersWork({{pblock->GetBlockHeader()}}) >= GetAntiDoSWorkThreshold()) {
```

💬 jonatack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2316022026)
Interesting proposal to review; note to self to look at why move peer dis(connection) logic.

👍 TheCharlatan approved a pull request: "ci: Use C++23 in one task"
(https://github.com/bitcoin/bitcoin/pull/30735#pullrequestreview-2267044020)
ACK fac587ea070fe1354708aacce33ebb9cebd35e5b

🤔 jonatack reviewed a pull request: "rpc: net: follow-ups for #30062"
(https://github.com/bitcoin/bitcoin/pull/30183#pullrequestreview-2267065357)
ACK 53f38f6fceec35819439e4b8491c4d7b700bf33

Sorry for the delay. Happy to re-review if you prefer the suggestion.

💬 jonatack commented on pull request "rpc: net: follow-ups for #30062":
(https://github.com/bitcoin/bitcoin/pull/30183#discussion_r1735170311)
nit, "peer" is slightly confusing to me here, as the `getrawaddrman` help uses the word "node" instead

```suggestion
{RPCResult::Type::NUM, "source_mapped_as", /*optional=*/true, "Mapped AS (Autonomous System) number in the BGP route to the source, used for diversifying peer selection (only displayed if the -asmap config option is set)"}
```

💬 TheCharlatan commented on pull request "kernel: Use spans instead of vectors for passing block headers to validation functions":
(https://github.com/bitcoin/bitcoin/pull/30742#issuecomment-2316078929)
Thanks for the review @jonatack.

Updated 45a90050290b046a8c0f417eab679f36032da297 -> 797eede7609ca0d4d788e62f35e70e85103a19dc ([headersSpan_2](https://github.com/TheCharlatan/bitcoin/tree/headersSpan_2) -> [headersSpan_3](https://github.com/TheCharlatan/bitcoin/tree/headersSpan_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/headersSpan_2..headersSpan_3))

* Addressed @jonatack's [comment](https://github.com/bitcoin/bitcoin/pull/30742#discussion_r1735122911), dropped now unnee
...

💬 maflcko commented on pull request "ci: Re-add configs removed in cmake migration":
(https://github.com/bitcoin/bitcoin/pull/30740#issuecomment-2316091747)
> > > many configs were removed from the CI without explanation.
> >
> >
> > Nice catch. It's not clear why a bunch of CI configs would have been removed..
>
> Sorry for overlooking them.

No worries. It is my fault for not reviewing them properly before. Also, it looks like the other reviewers missed them as well, so not your fault again.

💬 maflcko commented on pull request "ci: Re-add configs removed in cmake migration":
(https://github.com/bitcoin/bitcoin/pull/30740#discussion_r1735184833)
I am using `python3-zmq` as a proxy for that, which should probably be enough, no? And if libzmq was missing, the CI would be failing, no?

🤔 jonatack reviewed a pull request: "Deniability - a tool to automatically improve coin ownership privacy"
(https://github.com/bitcoin-core/gui/pull/733#pullrequestreview-2267095566)
Didn't conceptually review yet. While rebasing, the clang-tidy CI highlighted these issues to be addressed:

```
wallet/spend.cpp:1539:9: error: function 'CalculateDeniabilizationFeeEstimate' is within a recursive call chain [misc-no-recursion,-warnings-as-errors]
1539 | CAmount CalculateDeniabilizationFeeEstimate(const CScript& shared_script, CAmount total_value, unsigned int num_utxos, unsigned int deniabilization_cycles, const CFeeRate& fee_rate)
| ^
wallet/spend.cpp:1539
...

💬 jonatack commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-2316104843)
@vasild Please ping me to re-review/ACK when you update.