💬 maflcko commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#discussion_r1740531844)
Looks like this was merged

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2324270555)
> CI is unhappy :-(

Yeah, I've been trying to fix that but it's tricky because I can't reproduce it locally. I guess I need to ask someone with more CMake experience.

💬 maflcko commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1740614882)
missing COMMAND_ERROR_IS_FATAL? Otherwise, a failure is silently ignored, no?

🤔 maflcko reviewed a pull request: "Embed default ASMap as binary dump header file"
(https://github.com/bitcoin/bitcoin/pull/28792#pullrequestreview-2275159808)
I presume the CI fails because xxd is missing?

💬 maflcko commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1740619122)
Seems a bit odd to implement this both in Python and cmake (by calling xxd). Wouldn't it be easier to just implement it once and require that to be called before use in the other place?

💬 hebasto commented on pull request "ci: Delete no longer needed workaround":
(https://github.com/bitcoin/bitcoin/pull/30777#issuecomment-2324304484)
> Is there a reason why the msvc task no longer uses ccache?

The internal `ccache` integration was dropped in https://github.com/hebasto/bitcoin/pull/215. I see two options for proceeding:
- make another attempt to integrate `ccache` into the build system, which would benefit all MSVC users, or
- re-add `ccache` support to the CI job

💬 maflcko commented on pull request "refactor: Extend CScript with `operator<<` for `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1740636806)
not sure about this. If someone in the future has an `std::array<std::byte, N>` and want to serialize it, will this be touched again? What if someone wants to serialize the first `N/2` bytes only? Will they need to create a copy? Though, avoiding the vector conversion was the whole point of this pull request, so it just seems like right now this is adding more code while leaving the underlying issue.

Maybe this could be a span or a range?

In any case, the documentation needs to be clear th
...

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1740640186)
Added, let's see how it goes, I didn't know the function could fail silently.

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2324327583)
> I presume the CI fails because xxd is missing?

Maybe but it would really surprise me because the failures are in Ubuntu 22.04 environments and I tried reproduce the error in a fresh Ubuntu 22.04 VM and it seemed xxd was included from the start.

💬 maflcko commented on pull request "ci: Delete no longer needed workaround":
(https://github.com/bitcoin/bitcoin/pull/30777#issuecomment-2324328203)
I see. I guess it is fine to not have it, given that it only shaves off a small percentage of the overall build time. Running the functional or fuzz tests in `Debug` here requires each more time than just the build without ccache.

But up to you.

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2324330370)
Sounds like it can't find the raw file...

```
CMake Error at /ci_container_base/cmake/script/WriteIPASN.cmake:8 (execute_process):
execute_process error getting child return code: No such file or directory
```

💬 maflcko commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2324333044)
> Maybe but it would really surprise me because the failures are in Ubuntu 22.04 environments and I tried reproduce the error in a fresh Ubuntu 22.04 VM and it seemed xxd was included from the start.

Are you sure? Note that the CI doesn't use a desktop VM image, but a minimal docker image. I get:

```
# xxd
bash: xxd: command not found
```

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2324336425)
> Note that the CI doesn't use a desktop VM image, but a minimal docker image

Ah, then I guess that was my mistake

💬 maflcko commented on pull request "ci: Add missed configuration options to "Win64 native" job":
(https://github.com/bitcoin/bitcoin/pull/30755#issuecomment-2324365679)
review ACK 60f21202e038960bca859a86451181277856ef61

💬 ismaelsadeeq commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1740688427)
The new comment is much better, thanks for the example here.

👍 fanquake approved a pull request: "build: Remove Autotools-based build system"
(https://github.com/bitcoin/bitcoin/pull/30664#pullrequestreview-2275282934)
ACK faa382ae7642da0e436ea2c7f7eac67386280a7e

> The gitignore still seem inconsistent,

Especially given the comments in https://github.com/bitcoin/bitcoin/pull/30731#discussion_r1734503675. Seems those should have been removed for the same reasoning as https://github.com/bitcoin/bitcoin/pull/30664#discussion_r1740411900. Changes here some completely arbitrary.

🚀 fanquake merged a pull request: "build: Remove Autotools-based build system"
(https://github.com/bitcoin/bitcoin/pull/30664)

💬 maflcko commented on pull request "build: Remove Autotools-based build system":
(https://github.com/bitcoin/bitcoin/pull/30664#issuecomment-2324425893)
My latest review comments are on lines that are unrelated to switching branches. It is about temporary files in the source dir. Such files were created before cmake and will be created after cmake, because developers editing the source files will create them.

Personally I don't use gitignore, so I don't care, but it would be good to explain why the source-editing temporary-files changes are made and how they relate to autotools/cmake at all. Otherwise, there will be more follow-ups and confus
...

🤔 danielabrozzoni reviewed a pull request: "scripted-diff: LogPrint -> LogDebug"
(https://github.com/bitcoin/bitcoin/pull/30750#pullrequestreview-2275308548)
utACK fa09cb41f58d0483ffe134eb274b9048c5260faa

👍 TheCharlatan approved a pull request: "scripted-diff: LogPrint -> LogDebug"
(https://github.com/bitcoin/bitcoin/pull/30750#pullrequestreview-2275310978)
ACK fa09cb41f58d0483ffe134eb274b9048c5260faa