💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953783352)
Well `choices` counts the actual number of choices, consisting of:
* All transactions in sim.simmap (`tx_count`)
* All transactions in sim.removed (`sim.removed.size()`)
* The empty Ref (`1`).

When picking one of them, we want one in the range from 0 up to and including `choices - 1`.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953785604)
It doesn't matter; `TxGraph` ignores self-dependencies anyway (because `DepGraph` does), so having it doesn't hurt, and adds testing for that case.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953788209)
It's not clear to me what you're asking for. I would describe the test as "big simulation test, which performs a number of operations on a real TxGraph, and on a simpler reimplementation, and in the end compares the two". Something like that? Which `cluster_linearize` fuzz test are you referring to?

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953790028)
I'm not sure what you mean here.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953791082)
Agreed, fixed.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953791264)
That was outdated. Fixed.

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953815957)
Passing something like `search_system_path=true` to `try_exec()` is misleading because `try_exec()` uses `execvp(3)` which will search the system path but not based on the `search_system_path=true` argument but based on whether there is `/` in the executable: https://linux.die.net/man/3/execvp. Maybe rename `search_system_path` to `allow_notfound`?

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953823282)
Before it was like what you suggest, then the `check_tip_changed()` lambda was introduced because that code had to be called in two places, then, following suggestions, it was changed to be called in just one place. I agree it would be better to remove it now, given that it is called in just one place.

💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953825128)
> Assume(tip_block) ... if tip_block is null the right thing to do is just wait ...

Hmm, if `tip_block` is null then `Assume()` will not wait but stop the program?

💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1953840453)
> if timeout is exceeded while waiting for a new tip, this returns information about the current tip

True. What I meant was that if there is no tip within the timeout. The function does `wait_for(... tip_hash && tip_hash != current_tip ...)` - `tip_hash` to be set and to be different than the current. I meant that it could remain unset while the timeout passes. I see how the comment I suggested is misleading. What about:

```cpp
@retval std::nullopt if the given `timeout` passes before the
...

💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1953850030)
I mean to avoid `CHECK_NONFATAL()` if the tip is null and use `JSONRPCError` instead.

If the tip is null at the start, then wait for it to be set, if not set within the timeout, then `JSONRPCError`.

💬 davidgumberg commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2655685417)
ACK https://github.com/bitcoin/bitcoin/pull/31689/commits/1c6b886465df0f00549e7d10c3bfefd27be7f1c2

I like that the benchmarks added here for `ConnectBlock` are idealized scenarios that map to specific paths we want to test the performance of, namely Schnorr signature validation and ECDSA signature validation. Measuring performance in realistic scenarios is valuable for establishing ground truth, but it seems to me that microbenchmarks like these are most useful when their focus is narrow, sin
...

💬 maflcko commented on issue "Memory-only wallet (for faucets)":
(https://github.com/bitcoin/bitcoin/issues/31816#issuecomment-2655746412)
> Before I was doing also `scantxoutset` without any wallet. It actually provided me with unspent outputs that were confirmed in blocks and then I could just `signrawtransactionwithkey`. But using a wallet has many advantages

This would have been my suggestion as a workaround. Is there something that would be missing? I haven't tried, but `removeprunedfunds` could be another alternative to reduce the wallet file size?

👍 TheCharlatan approved a pull request: "cmake: add a component for each binary"
(https://github.com/bitcoin/bitcoin/pull/31844#pullrequestreview-2614109983)
ACK 686ebcfe93efa9ecca9b94e78da23a36c3b01f90

👍 TheCharlatan approved a pull request: "guix: use GCC 13 to build releases"
(https://github.com/bitcoin/bitcoin/pull/29881#pullrequestreview-2614124544)
Re-ACK 0c1b29a05777

💬 maflcko commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2655798240)
> @maflcko Mind taking a look at the first commit here to see if you'd prefer a different approach?

The first commit lgtm. Anything should be fine in the fuzz ci task as long as it compiles and runs the fuzz executable.

💬 hodlinator commented on pull request "qa debug: Add --debugnode/-waitfordebugger [DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/31723#issuecomment-2655806242)
- Added CMake option `WAIT_FOR_DEBUGGER`.
- Changed the Python logic from debugging specific nodes, to debugging specific node runs/executions, which is more specific.
- Use `prctl()` on Linux to allow any process to attach to `bitcoind`. This removes the necessity to disable `kernel.yama.ptrace_scope` on some systems, which may be seen as decreasing security, but is compiled out unless `WAIT_FOR_DEBUGGER=ON`.
- (Confirmed PR works on Windows).
- (Rebased).

👍 vasild approved a pull request: "cmake: Add `CheckLinkerSupportsPIE` module"
(https://github.com/bitcoin/bitcoin/pull/31359#pullrequestreview-2614092008)
ACK 81c174e3186efae084829dcde314b081cad3d3cb

I re-checked this. Without this PR I get the warning as mentioned in https://github.com/bitcoin/bitcoin/pull/31359#pullrequestreview-2465699855 and with this PR there is no warning. Further, without this PR, there is no `-fPIC` or `-fPIE` when compiling. With this PR `-fPIC` is added (not `-fPIE`).

💬 vasild commented on pull request "cmake: Add `CheckLinkerSupportsPIE` module":
(https://github.com/bitcoin/bitcoin/pull/31359#discussion_r1953962295)
> See the configure log

It would be nice to mention the actual file name of the "configure log" because that may not be obvious.
```suggestion
message(WARNING "PIE is not supported at link time. See ${CMAKE_BINARY_DIR}/CMakeFiles/CMakeConfigureLog.yaml for details.")
```

💬 hebasto commented on pull request "cmake: Add `CheckLinkerSupportsPIE` module":
(https://github.com/bitcoin/bitcoin/pull/31359#discussion_r1954013138)
It might depend on the used CMake version: https://github.com/bitcoin/bitcoin/blob/048ef98626b9ed62355923f089ad5cc5b6a898a3/ci/test/GetCMakeLogFiles.cmake#L5-L9