💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1451496426)
> nit: I think constants are not supposed to be prefixed with g_

https://github.com/bitcoin/bitcoin/blob/3ba8de1b704d590fa4e1975620bd21d830d11666/doc/developer-notes.md?plain=1#L87-L93

> I think you could also just use the genesis block (Params().GenesisBlock()) for these two values.

Done.

💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1451496443)
Done.

👍 hebasto approved a pull request: "build: remove `--enable-lto`"
(https://github.com/bitcoin/bitcoin/pull/29185#pullrequestreview-1820011320)
ACK 2d1b1c7daeeada3f737e62ceb2db7484cde5ff4e.

🤔 hebasto reviewed a pull request: "ci: move CMake into base packages"
(https://github.com/bitcoin/bitcoin/pull/29225#pullrequestreview-1820012896)
Post merge ACK f3ca6db8d349941f6bad6e8b328033222af8f063.

🤔 maflcko reviewed a pull request: "fuzz: make FuzzedDataProvider usage deterministic"
(https://github.com/bitcoin/bitcoin/pull/29043#pullrequestreview-1820019492)
I hacked together a quick clang-query, but it will catch too many false positives, so I guess hardcoding a list of class names and only checking those is a better alternative?


```
match expr(anyOf(cxxConstructExpr(unless(isListInitialization())).bind("ctor"),callExpr().bind("fun")),
allOf(hasDescendant(cxxMemberCallExpr(
unless(hasDeclaration(cxxMethodDecl(isConst()))),
callee(cxxMethodDecl(hasParent(recordDecl().bind("rd1"))))
).bind("mc1")),
hasDescendant(cxxMemberCallExpr(
...

💬 maflcko commented on pull request "fuzz: make FuzzedDataProvider usage deterministic":
(https://github.com/bitcoin/bitcoin/pull/29043#discussion_r1451520124)
nit: Obviously this is fine here, but in a function call where a copy was previously elided and now is not, due to a missing `std::move`, this can make code perform worse.

💬 fjahr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#issuecomment-1890480213)
> There's still some work in progress on fixing determinism, see [fjahr/asmap-data#6](https://github.com/fjahr/asmap-data/pull/6), but getting close!

This should be fixed now, the issue appears to have been the ordering of sets after combining/diffing them. The results are now explicitly sorted. On my system, I could replicate the issue by using different python versions and it was fixed with this change.

Credits to @sipa for pointing me in the right direction.

💬 hebasto commented on pull request "depends: remove dependency on Darwin libtool":
(https://github.com/bitcoin/bitcoin/pull/29232#discussion_r1451554686)
nit: Using the underlying build system call directly makes CMake re-build all targets again. That could be avoided by using CMake's invocation like that:
```suggestion
cmake --install . --prefix $($(package)_staging_prefix_dir)
```

Could be verified by building sequentially the `miniupnpc_built` and `miniupnpc_staged` targets.

💬 hebasto commented on pull request "depends: remove dependency on Darwin libtool":
(https://github.com/bitcoin/bitcoin/pull/29232#discussion_r1451560368)
nit: Maybe skip headers that are used only to build the package:
```suggestion
install ../natpmp.h ../natpmp_declspec.h $($(package)_staging_prefix_dir)/include &&\
```
?

👍 hebasto approved a pull request: "depends: remove dependency on Darwin libtool"
(https://github.com/bitcoin/bitcoin/pull/29232#pullrequestreview-1820037343)
ACK a28c6bbde0bcd064869cf861d92a6e23df4a7b73

💬 hebasto commented on pull request "depends: remove dependency on Darwin libtool":
(https://github.com/bitcoin/bitcoin/pull/29232#discussion_r1451556682)
nit: Maybe add a comment that this line might be revised in the future considering https://github.com/miniupnp/miniupnp/pull/659?

💬 hebasto commented on pull request "depends: remove dependency on Darwin libtool":
(https://github.com/bitcoin/bitcoin/pull/29232#discussion_r1451557398)
```suggestion
@@ -116,7 +116,7 @@ if (UPNPC_BUILD_STATIC)
```
to avoid `Hunk #1 succeeded at 116 (offset 17 lines).`

💬 kevkevinpal commented on pull request "test: Remove all-lint.py script":
(https://github.com/bitcoin/bitcoin/pull/29228#issuecomment-1890593620)
looks like we can replace the [comment to run all the lint commands](https://github.com/bitcoin/bitcoin/blob/master/test/README.md)


we can swap this
`test/lint/all-lint.py`
for
`test/lint/lint-*.py`

💬 achow101 commented on pull request "wallet: Reset chain notifications handler if AttachChain fails":
(https://github.com/bitcoin/bitcoin/pull/29243#issuecomment-1890593625)
> Is this for backport?

This is a problem that goes back several major versions (it is possible to hit without assumeutxo), so probably not.

💬 kevkevinpal commented on pull request "log mempool loading progress":
(https://github.com/bitcoin/bitcoin/pull/29227#issuecomment-1890595882)
Concept ACK [eb78ea4](https://github.com/bitcoin/bitcoin/pull/29227/commits/eb78ea4eebfe150bc1746282bfdad6eb0f764e3c)

💬 fanquake commented on pull request "build: depends move macOS C(XX) FLAGS out of C & CXX":
(https://github.com/bitcoin/bitcoin/pull/29233#issuecomment-1890627039)
> Or are we adding it to the linker elsewhere?

We are passing the min version as part of `-platform_version` in `darwin_LDFLAGS`. Note that we also check that the release bins have this min os set in `symbol-check.py` (along with sdk & linker version).

🤔 murchandamus reviewed a pull request: "wallet: Reset chain notifications handler if AttachChain fails"
(https://github.com/bitcoin/bitcoin/pull/29243#pullrequestreview-1820086857)
ACK ea2551e55d260854a5cca8aa95034970d4adca1c

👍 TheCharlatan approved a pull request: "wallet: Reset chain notifications handler if AttachChain fails"
(https://github.com/bitcoin/bitcoin/pull/29243#pullrequestreview-1820104671)
ACK ea2551e55d260854a5cca8aa95034970d4adca1c

💬 TheCharlatan commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#discussion_r1451613773)
This check seems like the right thing to do, but this could be handled by the caller, by checking `if (!(pindex->nStatus & BLOCK_HAVE_DATA))`, like done in `net_processing.cpp:2292`. Maybe still add this in a separate commit to clearly explain what is going and why being defensive here is an improvement?

👍 furszy approved a pull request: "wallet: Reset chain notifications handler if AttachChain fails"
(https://github.com/bitcoin/bitcoin/pull/29243#pullrequestreview-1820111062)
Code review ACK ea2551e5

Plus, made a quick test replicating the behavior https://github.com/furszy/bitcoin-core/commit/b6aefde6bdb9bf52e43c54d98921d8da2f0a6b5f (it is not meant to be merged, just to trigger the failure locally). It fails without the ea2551e5, and passes with it.