💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2005207380)
"acceptable quality" is more than just a topologically valid order.
It refers to a linearization that may not be optimal but is the best we can achieve for that cluster within resource limitations.

💬 Sjors commented on pull request "cmake: Add `NO_CACHE_IF_FAILED` option for checking linker flags":
(https://github.com/bitcoin/bitcoin/pull/32027#issuecomment-2739809540)
This seems useful, but I'm having a hard time reproducing the issue, or something similar, on macOS, because XCode 15.2 ships with most of the sanitizers.

I can hit an earlier error:

```
cmake -B build -DSANITIZERS=memory
...
-- Performing Test CXX_SUPPORTS__FSANITIZE_MEMORY - Failed
CMake Error at CMakeLists.txt:362 (message):
Compiler did not accept requested flags.
```

But then I'd probably have to recompile LLVM.

Might be easier to reproduce on a linux distro, like the or
...

💬 Sjors commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#issuecomment-2739873757)
ACK d190f0facc8379da7610d7161e532d57c6a7eb96

I verified that the commits here match 095801b8999851b10e43567389cd293fd7957497 and 45d439dab13153a3b570957a9eab63e3e6274611 in #31375 where I reviewed them.

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2005273964)
In 101a8ee3280e50c3272a80939b46a67faca838e4 "txgraph: (optimization) avoid per-group vectors for clusters & dependencies"
```suggestion
/** Where the dependencies for this cluster group in ClusterSet::m_deps_to_add start. */

```

👍 pablomartin4btc approved a pull request: "refactor: Remove redundant and confusing calls to IsArgSet"
(https://github.com/bitcoin/bitcoin/pull/31896#pullrequestreview-2702084258)
re-ACK fa0cf040baa74b161b4b00ddea95bfb5aac0bb53

Since my last review: outdated TODO [comment](https://github.com/bitcoin/bitcoin/pull/31896#discussion_r1997633914) was removed (on new commit) - no impact.

💬 Sjors commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2739919199)
re-ACK a9935d4dae8847d0549ec51ddc4349c7e8c61763

Briefly tested another guix build on Windows.

📝 ismaelsadeeq opened a pull request: "doc: clarify the documentation of `Assume` assertion"
(https://github.com/bitcoin/bitcoin/pull/32100)
Statements inside `Assume` may be optimized away in production builds when the compiler proves they are side-effect-free.
This makes `Assume` useful for testing invariants that are always evaluated in debug builds, allowing violations to be caught during fuzzing with the hope that it wont incur runtime cost in release builds.
This use case is demonstrated in #31363 and is suggested to be documented in https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2736410023.

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2740049673)
> That makes sense. Might be a good idea to update doc/developer-notes.md pointing out that use case? It currently says:

Done in #32100

💬 l0rinc commented on pull request "[IBD] specialize block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#discussion_r2005441297)
Now that #31519 was merged, I've rebased an moved it out of draft.

👋 l0rinc's pull request is ready for review: "[IBD] specialize block serialization"
(https://github.com/bitcoin/bitcoin/pull/31868)

💬 willcl-ark commented on issue "ci: failure in interface_usdt_utxocache.py":
(https://github.com/bitcoin/bitcoin/issues/31951#issuecomment-2740183900)
From reading up on this a little it seems to be possible that if the tracepoint:

- includes runtime [lookups](https://github.com/bitcoin/bitcoin/blob/aa87e0b44600a32b32a4b123d4f90d097f1f106f/src/txmempool.cpp#L513-L518)
- is executed in a multi-threaded (or multi-CPU) environment
- (possibly other factors)

...then it may not be possible to totally guarantee ordering at userspace collection points. One can use BPF [queues](https://docs.kernel.org/bpf/map_queue_stack.html), but I suspect this wo
...

🤔 ismaelsadeeq reviewed a pull request: "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods"
(https://github.com/bitcoin/bitcoin/pull/31785#pullrequestreview-2702397335)
Concept ACK

In 64a2795fd4fe223a55564c31e9fa36264e79ac22 "rpc: handle shutdown during long poll and wait methods"

is a use for returning the last known tip?

If not, should we simplify the behavior by always returning nullopt when the tip has not changed or no better block is available?

Or It will be more consistent to to return the last known tip if it is set, when this edge case happen handle it by returning nullopt.

✅ vasild closed a pull request: "i2p: make a time gap between creating transient sessions and using them"
(https://github.com/bitcoin/bitcoin/pull/32065)

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005472525)
A new comment has been posted to that bug report.

💬 0xB10C commented on issue "ci: failure in interface_usdt_utxocache.py":
(https://github.com/bitcoin/bitcoin/issues/31951#issuecomment-2740221745)
> To change only this test is relatviely simple: https://github.com/bitcoin/bitcoin/compare/master...willcl-ark:bitcoin:usdt-order-independant but it will be more work to apply to all the tests...

This was the first, and for now, the only time we saw this test fail due to ordering problems. I think we can't completely rule out ordering problems, but IMO it would be good to see this happening in some other test(s) before we start changing them.

💬 ismaelsadeeq commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2740221944)
re-ACK 226d81f8b708ea1c3a39ec58446e291fe7440fdd

Latest change added functional test coverage for the `sigops` and `fee` field of txs returned from `getblocktemplate` RPC

💬 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-2740226777)
`a22a3b3c4c...48a636cdde`: rebase due to conflicts and reduce the pre-created sessions to 1, given the above. Some logs from running this:

```
974be1f338 startup +129s: made new session
974be1f338 startup +290s, 161s after creation: connect failed
974be1f338 startup +372s, 243s after creation: connect failed
974be1f338 startup +678s, 549s after creation: connect ok, connection duration: 1305s, destroyed after that

55723ba1fc startup +1385s: made new session
55723ba1fc startup +1441s,
...

📝 vasild reopened a pull request: "i2p: make a time gap between creating transient sessions and using them"
(https://github.com/bitcoin/bitcoin/pull/32065)
Connecting to an I2P peer consists of creating a session (the
`SESSION CREATE` command) and then connecting to the peer using that
session (`STREAM CONNECT ID=session_id ...`).

This change is only relevant for transient sessions because when a
persistent session is used it is created once and used for all
connections.

Before this change Bitcoin Core would create the session and use it in
quick succession. That is, the `SESSION CREATE` command would be
immediately followed by `STREAM
...

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2005488831)
The issue can be observed in the following scenario on an `aarch64` machine:
```
$ env HOSTS=x86_64-linux-gnu ./contrib/guix/guix-build
...
Building qt...
ninja: error: 'qtbase/src/platformsupport/input/CMakeFiles/InputSupportPrivate.dir/cmake_pch.hxx.gch', needed by 'qtbase/src/platformsupport/input/CMakeFiles/InputSupportPrivate.dir/InputSupportPrivate_autogen/mocs_compilation.cpp.o', missing and no known rule to make it
make: *** [funcs.mk:303: /bitcoin/depends/work/build/x86_64-linux-
...

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2740271781)
Some of @fanquake's recent feedback has been addressed.

Additionally, the behaviour when cross-compiling for `HOST=x86_64-w64-mingw32` on a system with a native Vulkan package installed has been fixed. Qt 6 erroneously (a bug?) labels Vulkan as available for cross-compiling. Vulkan has been disabled for `mingw32`, which effectively makes it disabled for all platforms.