🤔 vasild reviewed a pull request: "Improve LastCommonAncestor performance + add tests"
(https://github.com/bitcoin/bitcoin/pull/33515#pullrequestreview-3294562269)
Approach ACK 4fe3de2aa7fda58d5c56987aee13ae87191da1c6

💬 vasild commented on pull request "Improve LastCommonAncestor performance + add tests":
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2398800190)
This assumes that all blocks at the same height have equal pskip height. This is true currently, but if changed, then this code will trigger the Assume in debug and will do null pointer dereference in release builds. Looking at `GetSkipHeight()`, it derives its result deterministically from `height` only (good).

Is it worth here, if `pa->pskip->nHeight != pb->pskip->nHeight`, then to fall back to one-by-one iterating?

💬 vasild commented on pull request "Improve LastCommonAncestor performance + add tests":
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2398805880)
nit: I guess that if this check fails, then the test should be interrupted without performing further checks. If so, maybe use `BOOST_REQUIRE_EQUAL()`?

💬 vasild commented on pull request "Improve LastCommonAncestor performance + add tests":
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2398812630)
nit: similar as above: `BOOST_REQUIRE(a == b)` or `BOOST_REQUIRE_EQUAL(a, b)`

💬 vasild commented on pull request "Improve LastCommonAncestor performance + add tests":
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2398807972)
nit: similar as above: `BOOST_REQUIRE_EQUAL()`?

💬 ryanofsky commented on pull request "Update libmultiprocess subtree to support reduced logging":
(https://github.com/bitcoin/bitcoin/pull/33518#issuecomment-3361255971)
> I think the nix config is not using an instrumented libcpp, which could be a reason for the difference?

Yes, that was the reason! Enabling "-DLLVM_USE_SANITIZER=Thread" for libcxx in nix lets me reproduce this. Will make a PR updating the CI job and fixing the bug, once I figure out what the bug is

💬 vasild commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#issuecomment-3361306154)
`a42449cb30...fc1ba52a4d`: rebase due to conflicts

💬 sipa commented on pull request "Improve LastCommonAncestor performance + add tests":
(https://github.com/bitcoin/bitcoin/pull/33515#discussion_r2398962673)
I don't think so - that eventuality (pskip depending on more than just height) is dealt with through the `Assume()`. I guess I can add a comment to clarify that.

💬 Eunovo commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2398976897)
I also want to note that the generated blocks already contain `OP_RETURN` outputs in coinbase transactions, so we don't need to create a specific `OP_RETURN` output in the test.

This might also remove the need to have a second block at all, since there's no need to create a new tip for the `OP_RETURN` test

💬 fanquake commented on pull request "Mempool: Do not enforce TRUC checks on reorg":
(https://github.com/bitcoin/bitcoin/pull/33504#issuecomment-3361440727)
Backported to 29.x in #33474.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2399012085)
I tried to mimic the script validation like `GetCheckQueue`. But, I guess this is different enough. Will change next time I push.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2399016753)
I've updated to use semaphores instead of mutex. That should be more efficient.

> especially if we sort the keys first so that threads are more likely to access different regions

I don't understand what this has to do with being lock free.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2399019592)
I updated the thread name to ThreadLoop, which just does the loop. There is another function now, `FetchInputsOnThread`, that fetches for each block until finished.

💬 alexanderwiederin commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2399024714)
I suggest we make these params `int` instead of `bool` in this commit already

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2399025372)
Prefiltering on the main thread is too slow, it's faster if we do the filtering in parallel. So, we still need to have a smaller batch size because then work will not be divided evenly. One thread could get all cache misses while the others all have cached inputs.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2399027783)
Prefiltering on the main thread is too slow. It is several milliseconds to check every input in large blocks whether they exist in the cache.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2399028082)
Done.

💬 alexanderwiederin commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2399028947)
Feel like we can remove this added line

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2399032242)
I believe this is resolved.

💬 l0rinc commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-3361478681)
For future me, wondering how to make fuzzing work on a Mac with latest Clang + AppleClang - this is the version that mostly works for me (though it fails in a lot of places where Linux doesn't):
```
rm -rfd build_fuzz && cmake --preset=libfuzzer \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DCMAKE_OSX_SYSROOT="$(xcrun --show-sdk-path)" \
-DCMAKE_EXE_LINKER_FLAGS="-fuse-ld=lld -L$(brew --prefix llvm)/lib/c++
...