🤔 marcofleon reviewed a pull request: "fuzz: Use serial task runner to increase fuzz stability"
(https://github.com/bitcoin/bitcoin/pull/31841#pullrequestreview-2691764413)
ACK fa4fb6a8f15c295a2a3d3ffd737e115b8be46c1f

Seems fine to me to use `ImmediateTaskRunner` for fuzz tests. I checked stability for `partially_downloaded_block` and saw it go from 90% to 94%.

Am I correct in saying that this change makes calling something like `SyncWithValidationInterfaceQueue()` in fuzz targets effectively a no-op?

💬 janb84 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1999479349)
The extra `-g` just gives a lot of `Unexecuted instantiation:` messages and a lot lower coverage. No sure why but don't think this gives a good image of the actual coverage.

💬 maflcko commented on pull request "fuzz: Use immediate task runner to increase fuzz stability":
(https://github.com/bitcoin/bitcoin/pull/31841#issuecomment-2730623476)
> Am I correct in saying that this change makes calling something like `SyncWithValidationInterfaceQueue()` in fuzz targets effectively a no-op?

Yes, they could be removed now. Happy to include that here, but they should also be harmless.

💬 maflcko commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r1999497982)
stray leftover debug statement?

🤔 maflcko reviewed a pull request: "Feature: Use different datadirs for different signets"
(https://github.com/bitcoin/bitcoin/pull/29838#pullrequestreview-2691801719)
are you still working on this?

👋 naiyoma's pull request is ready for review: "test: Add test coverage for rpcwhitelistdefault when unset"
(https://github.com/bitcoin/bitcoin/pull/32079)

💬 VolodymyrBg commented on pull request "qt: Add addressList field to SendCoinsRecipient for multiple addresses":
(https://github.com/bitcoin-core/gui/pull/857#issuecomment-2730662322)
@maflcko done

👍 willcl-ark approved a pull request: "Shuffle depends instructions and recommend modern make for macOS"
(https://github.com/bitcoin/bitcoin/pull/32086#pullrequestreview-2691813509)
reACK 31a0c5f3caa80c8e6baf4e8f909a673442a852d5

💬 tnndbtc commented on pull request "test: fix intermittent failure in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/pull/32063#issuecomment-2730664318)
Hi @mzumsande , Followed your hint that the node needs to successfully disconnect the peer before bumping mocktime, I added one line of "debugging" code to reproduce the issue 100%: in `src/net.cpp:CConnman::DisconnectNodes()`, add a one second sleep before calling `DeleteNode(pnode);`
```
{
// Delete disconnected nodes
std::list<CNode*> nodes_disconnected_copy = m_nodes_disconnected;
for (CNode* pnode : nodes_disconnected_copy)
{
// Destroy
...

💬 naiyoma commented on pull request "test: Add test coverage for rpcwhitelistdefault when unset":
(https://github.com/bitcoin/bitcoin/pull/32079#issuecomment-2730670316)
> Concept ACK, nice follow-up! Why is it in draft?

had to force push some changes, but its now open

💬 tnndbtc commented on issue "intermittent issue in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/issues/31700#issuecomment-2730670414)
@maflcko Even with @mzumsande 's [fix](https://github.com/bitcoin/bitcoin/pull/32063) in , I am able to add a one second sleep in src/net.cpp:CConnman::DisconnectNodes() to 100% reproduce the issue. For more details, please checkout https://github.com/bitcoin/bitcoin/pull/32063#issuecomment-2730664318

💬 maflcko commented on pull request "depends: Avoid using helper variables in toolchain file":
(https://github.com/bitcoin/bitcoin/pull/31360#issuecomment-2730672659)
CI fails with https://cirrus-ci.com/task/6136289713455104?logs=ci#L531 :

```
copying packages: boost libevent qt expat libxcb xcb_proto libXau xproto freetype fontconfig libxkbcommon libxcb_util libxcb_util_render libxcb_util_keysyms libxcb_util_image libxcb_util_wm qrencode systemtap zeromq
to: /ci_container_base/depends/x86_64-pc-linux-gnu
To build Bitcoin Core with these packages, pass '--toolchain /ci_container_base/depends/x86_64-pc-linux-gnu/toolchain.cmake' to the first CMake invo
...

⚠️ maflcko reopened an issue: "intermittent issue in p2p_orphan_handling.py"
(https://github.com/bitcoin/bitcoin/issues/31700)
Looks like the test may overall intermittently fail, because it assumes the tx delay isn't exponentially distributed, but has a constant delay, sufficient for the test to pass.

I think it would be better to use mocktime to halt the test when needed, so that it can even pass when the delay is zero (or close to it).

I haven't looked in detail whether this is the same, but there was one instance in the CI: (Possibly the getdata was split into two, so the check didn't pick it up, because it assume
...

💬 maflcko commented on pull request "qt: Add addressList field to SendCoinsRecipient for multiple addresses":
(https://github.com/bitcoin-core/gui/pull/857#issuecomment-2730699636)
You'll also need to make sure the test, and CI pass. Also, there needs to be a test for a new feature.

💬 maflcko commented on pull request "fuzz: Speed up *_package_eval fuzz targets a bit":
(https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1999553734)
`mempool_outpoints` can't be changed, because the order matters here. An unordered_set isn't ordered and may result in a non-deterministic fuzz target, given the same fuzz input.

💬 l0rinc commented on pull request "fuzz: Speed up *_package_eval fuzz targets a bit":
(https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1999563234)
Ah, that's what https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1967477525 meant - please resolve this comment

💬 maflcko commented on pull request "fuzz: Speed up *_package_eval fuzz targets a bit":
(https://github.com/bitcoin/bitcoin/pull/31457#discussion_r1999589480)
Not sure if the two are comparable, inserting into an empty set or inserting into a set with one value (probably the most common cases) seems plausible to be faster than doing the same in an unordered_set. The goal here is to avoid fuzz timeouts (or long running large fuzz inputs) when using sanitizers.

💬 maflcko commented on pull request "contrib: Add `deterministic-fuzz-coverage/Cargo.lock` to version control":
(https://github.com/bitcoin/bitcoin/pull/32082#issuecomment-2730858179)
Duplicate of fa3940b1cbc94c8ccfde36be1db1adca04fbcaa6?

Otherwise, needs rebase.

💬 maflcko commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2730873997)
> After this change I keep getting `untracked files will be overwritten by rebase` for `contrib/devtools/deterministic-fuzz-coverage/Cargo.lock`: added a follow-up to commit it #32082

When testing locally, my recommendation would be to generally try to rebase a pull request onto current master. This ensures you get the latest fixes, keep your ccache temperature high by focussing on master, and may even find silent merge conflicts.

💬 maflcko commented on pull request "doc: shallow clone `qa-assets`":
(https://github.com/bitcoin/bitcoin/pull/32083#issuecomment-2730877899)
lgtm ACK 6f9f415a4fa3a3c1dc87c207b0192a5ed0a91e0f