💬 mzumsande commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#discussion_r1999313868)
This is not the timeout specific for IBD stalling but the general one (which is generally longer). So we'd still disconnect for stalling (see log message "Peer is stalling block download") - is that on purpose / can you explain a bit more the reasoning behind this?

💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2730598301)
> Update: Just saw the PR https://github.com/bitcoin/bitcoin/pull/31835 which fixes this issue. :)

Heh, looks like you are at least the third person who found that bug independently, first one was in #16856!

> Since both branches behave similarly (even on master the m_best_header and BLOCK_FAILED_CHILD statuses are set correctly because of the final call to InvalidChainFound - except for the ones not set as BLOCK_FAILED_CHILD because of the potential bug), is it correct that the main goal
...

🤔 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.