💬 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

🚀 fanquake merged a pull request: "scripted-diff: LogPrint -> LogDebug"
(https://github.com/bitcoin/bitcoin/pull/30750)

🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: optimized candidate search"
(https://github.com/bitcoin/bitcoin/pull/30286#pullrequestreview-2275360087)
light code review and Tested ACK 100e1b9ecb29c76187112fda9fed1c01da192c99

🤔 ismaelsadeeq reviewed a pull request: "build: Introduce CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2275372899)
Post-merge Tested ACK 41051290ab3b6c36312cec26a27f787cea9961b4


Tested on M2 Pro, Sonoma 14.1.1

Clang version 18.1.7

CMake version 3.29.5

Configuration and compilation completed smoothly without any issues.

Started bitcoind without any issue and tested a few RPCs.

I also ran unit tests and benchmarks, all ran smoothly.

💬 garlonicon commented on issue "UpdateTime() needs to account for timewarp fix on testnet4 ":
(https://github.com/bitcoin/bitcoin/issues/30614#issuecomment-2324516655)
If someone uses 600 seconds as a MAX_TIMEWARP, then that node can be stuck on block 42335. More information: https://bitcointalk.org/index.php?topic=5499150.msg64488234#msg64488234

I wonder, if all blocks after 42335 will be reorged, or if MAX_TIMEWARP will be bigger than 600. I guess v28.0 was released with that value, and some nodes are stuck, because of that.

💬 hebasto commented on pull request "ci: Add missed configuration options to "Win64 native" job":
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1740764357)
Thanks! Your proposal has been accepted.

✅ TheCharlatan closed a pull request: "kernel: Run sanity checks on context construction"
(https://github.com/bitcoin/bitcoin/pull/28228)