💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1912840515)
Testnet3, testnet4 and mainnet all have a difficulty 1 genesis block, so it wouldn't make a difference.

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1912853526)
Fixed

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1912853717)
I changed it to "to 2"

💬 Sjors commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2586547919)
@hodlinator a Western Digital spinning disk. Much slower than SSD but fine for just writing out blocks to disk.

(I'm assuming we're not doing something foolish like writing the block, then reading it again to do the xor and writing it back)

@l0rinc I wiped the blocks, indexes and chainstate dirs between runs. Both times I mention exclude the 8 minute flush, which remained similar.

I agree with @maflcko that a difference of 10 minutes is probably not statistically significant. I don't ha
...

💬 vasild commented on pull request "ci: change the build to be verbose by default":
(https://github.com/bitcoin/bitcoin/pull/31619#issuecomment-2586556645)
> How is this different from just looking at the `C++ compiler flags ....................` output?

The exact compilation command contains more information than just the flags. I guess this is why the second build (after a failure) is verbose. There is also this:

```
NOTE: The summary above may not exactly match the final applied build flags
if any additional CMAKE_* or environment variables have been modified.
To see the exact flags applied, build with the --verbose option.
...

💬 Sjors commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2586587671)
Ok, so now we have `bitcoin-util` and `bitcoin-chainstate` in `libexec`, with `bitcoin-gui` and `bitcoin-node` back in `bin` - which can be moved by #31375. That seems fine to me.

When I do `sudo cmake --install build` on macOS 15.2 it puts the binaries in `/usr/local/bin` and `/usr/local/lib` (not `/usr/local/libexec` or `/usr/libexec`). Is that intended?

📝 vasild opened a pull request: "test: avoid internet traffic"
(https://github.com/bitcoin/bitcoin/pull/31646)
Avoid generating outbound traffic on a non-loopback interface during tests. Fix all tests, including ones that generate DNS traffic.

---

This is a subset of https://github.com/bitcoin/bitcoin/pull/31349 containing only the changes to the tests, without the CI changes to detect future regressions.

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2586591525)
Extracted the changes to the tests in https://github.com/bitcoin/bitcoin/pull/31646 (suggested by @fjahr and @maflcko).

👍 hodlinator approved a pull request: "doc: Improve dependencies documentation"
(https://github.com/bitcoin/bitcoin/pull/31634#pullrequestreview-2546121330)
re-ACK 1fcc1ce0edcc8ae522ab9103ad79adf58fc0fe0e

Thanks for applying my suggestions.

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2586611555)
@fanquake, is the problem you reported above https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2573295321 when the tests run on the host, without a docker?

💬 l0rinc commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2586629442)
@Sjors, your setup is already extremely fast - it seems this optimization shines mostly on commodity hardware, which I assume is used more often.
Let's wait for other reproducers.

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2586643113)
> When I do `sudo cmake --install build` on macOS 15.2 it puts the binaries in `/usr/local/bin` and `/usr/local/lib` (not `/usr/local/libexec` or `/usr/libexec`). Is that intended?

Target output locations and their installation paths are independent properties. This PR modifies the former only. I'll look into modifying the latter accordingly.

💬 fanquake commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1912951874)
Python isn't required.

👍 dergoegge approved a pull request: "p2p: track and use all potential peers for orphan resolution"
(https://github.com/bitcoin/bitcoin/pull/31397#pullrequestreview-2546241855)
Code review ACK 86d7135e36efd39781cf4c969011df99f0cbb69d

Other reviewers already mentioned the nits that I would have had as well. I think they are fine to address in a follow up.

💬 NicolaLS commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1913007533)
ah yes..thanks. should I move [Linux Kernel](https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md?plain=1#L22) to optional too then ?

💬 Sjors commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2586797138)
Concept ACK

I tested this with https://github.com/Sjors/bitcoin/pull/76 on my own CI setup, which uses Cirrus workers configured using (roughly) the instructions here (using podman-docker): https://github.com/bitcoin/bitcoin/blob/master/.cirrus.yml#L8

💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2586828644)
I guess an env var is leaking into the docker. To reproduce you could try setting the URL via the env var `DEBUGINFOD_URLS=https://debuginfod.fedoraproject.org/ `, but I haven't tried it.

💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#issuecomment-2586835482)
Rebased for merge conflict, and updated for @sipa's comment (thanks) to add a constant for the initial mocked clock value.

💬 maflcko commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1913034308)
For the merge conflict, isn't this missing in the next line?

```cpp
if (!mocktime.count()) {
g_used_system_time = true;
}

💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1913040379)
i first didn't because i thought it would require all tests that indirectly use the steady clock to be updated, which i deemed outside the scope of this PR.
But it actually doesn't, the new clock is (intentially) only used in one place. And besides, this is checked for the fuzz framework not the unit tests.
Will add.