💬 mzumsande commented on issue "Avoid internet traffic from tests":
(https://github.com/bitcoin/bitcoin/issues/31339#issuecomment-2492157450)
> Honestly, I forgot which test was that.

Maybe it was https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1701616675 (I independently ran into that a few weeks ago and included a fix in #31142)

📝 Parkeragan opened a pull request: "Add files via upload"
(https://github.com/bitcoin/bitcoin/pull/31342)


Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->

✅ willcl-ark closed a pull request: "Add files via upload"
(https://github.com/bitcoin/bitcoin/pull/31342)

📝 fanquake locked a pull request: "Add files via upload"
(https://github.com/bitcoin/bitcoin/pull/31342)


Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->

💬 TheCharlatan commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#issuecomment-2492366799)
Thank you for the reviews and the test case @instagibbs,

Updated 563278b6a125f1a3d2111d4ebd74b9cc16d83ec5 -> 73db95c65c1d372822166045ca8b9f173d5fd883 ([submitblock_prechecks_3](https://github.com/TheCharlatan/bitcoin/tree/submitblock_prechecks_3) -> [submitblock_prechecks_4](https://github.com/TheCharlatan/bitcoin/tree/submitblock_prechecks_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/submitblock_prechecks_3..submitblock_prechecks_4))

* Addressed @Sjors' [comment](https://
...

👍 TheCharlatan approved a pull request: "Drop script_pub_key arg from createNewBlock"
(https://github.com/bitcoin/bitcoin/pull/31318#pullrequestreview-2452805746)
ACK 027ce3e9634b2f47c508899942d05690572de516

💬 andremralves commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#discussion_r1852949174)
> This could also be because the user might not have built the binaries yet even in the case of default build path.

Yes, I haven't added it to the message because I though it would be too verbose, but I can reconsider it.

👍 hodlinator approved a pull request: "policy: ephemeral dust followups"
(https://github.com/bitcoin/bitcoin/pull/31279#pullrequestreview-2452817327)
ACK 466e4df3fb83ef82b6add22e202e3a70dbf83a12

Mostly responded to my feedback since my last review.

Compiled with BDB and passed functional tests, unittests and ran `ephemeral_package_eval` fuzz target for 1 minute.

Still have [reservations about the prioritisetransaction RPC](https://github.com/bitcoin/bitcoin/pull/31279/files#r1848453460), but not blocking.

🤔 hebasto reviewed a pull request: "guix: swap `moreutils` for just `sponge`"
(https://github.com/bitcoin/bitcoin/pull/31323#pullrequestreview-2452825209)
My Guix build:
```
riscv64
74a02707614d7fc7ed460148b37e90c126d09c2e9b2962765de14d267a3f22d3 guix-build-c4ee9b8aa078/output/aarch64-linux-gnu/SHA256SUMS.part
263f079da730485eb583995c1fc6304b7ca957a668fd5aac8f879bc59746dc67 guix-build-c4ee9b8aa078/output/aarch64-linux-gnu/bitcoin-c4ee9b8aa078-aarch64-linux-gnu-debug.tar.gz
745a9bfdd9550cf9abf80415c4efe818446ea05e9c4f659f643669c3dcb49893 guix-build-c4ee9b8aa078/output/aarch64-linux-gnu/bitcoin-c4ee9b8aa078-aarch64-linux-gnu.tar.gz
5810b1ef
...

💬 andremralves commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#discussion_r1852957327)
I've updated it, thank for the suggestion.

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1852962780)
`-version=true` and `-version=false` are both interpreted as `-version=0` (there is no special parsing for booleans expressed as strings).
`-version=0` is equivalent to `-noversion` or `-noversion=1`.

`IsArgSet()` returns `true` if the user has set a given arg to *any value* (including negation), through command line or settings-file.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2492442146)
Updated 97fe2b25af31ca612c1f8d9f3de739fa3dee3902 -> 5910f9bb18da35101144304d385618d7b82420f5 ([kernelApi_2](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_2) -> [kernelApi_3](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_2..kernelApi_3))

* As discussed with @stickies-v out of band, make callbacks only return `const` pointers, which further ensures that the user does not de-allocate or take ownership of th
...

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1852982119)
What would you use as category at the beginning of commit messages when implementations are merged with tests? - Feels logical to have for example `net: ...` commit + `test net: ...` commit, you would just have the first message and merge them together?

If the pre-existing tests were to be broken by changing the implementations, I can understand changing them in the same commit, but AFAIK that is not the case in this PR.

My impression is that this project encourages separate commits for te
...

📝 theStack opened a pull request: "test: avoid internet traffic in rpc_net.py"
(https://github.com/bitcoin/bitcoin/pull/31343)
Rather than opening a connection to a public IP, use one in the [link-local address range](https://en.wikipedia.org/wiki/Link-local_address#IPv4) in order to avoid the problems described in #31339. This is still not ideal and could be annoying to users if there is an actual machine reachable on this address, so I'm open for better suggestions. As far as I understand, we don't want to use localhost once again here, since for testing the filtering a different IP than the one already added before s
...

💬 theStack commented on issue "Avoid internet traffic from tests":
(https://github.com/bitcoin/bitcoin/issues/31339#issuecomment-2492457208)
> @fanquake Honestly, I forgot which test was that. During latest coredev I investigated that briefly with @maflcko or @0xB10C (?) and we observed the non local traffic with `tcpdump(1)`. Now, my point is to enforce this in the CI, even if such test does not exist currently - has been fixed since coredev or my investigation back then was wrong.

I think that was us back then, we looked at rpc_net.py and found this line:
https://github.com/bitcoin/bitcoin/blob/17834bd1976df7a2ff6c2f5f05a59ae3f
...

💬 hodlinator commented on pull request "test: locking -testdatadir when not specified and then deleting lock and dir at end of test":
(https://github.com/bitcoin/bitcoin/pull/31328#issuecomment-2492536261)
Code review 8963bc860f6f55462851fbda28627b4483ba8240

Think this PR might be overly paranoid.

Regardless of whether `G_TEST_GET_FULL_NAME` returns a value for the given test, is locking really necessary if directories are sufficiently unique (random) thanks to faaaf59f71ede057b2c1d369ef8db973c2f2dbc2 being included in the already merged #31317?

```C++
const auto rand{HexStr(g_rng_temp_path.randbytes(10))}
```
Should provide this many possible patterns:
```math
2^{8*10} = 256^{10} \a
...

💬 achow101 commented on pull request "contrib: fix BUILDDIR in gen-bitcoin-conf script":
(https://github.com/bitcoin/bitcoin/pull/31332#issuecomment-2492544118)
`contrib/devtools/README.md` needs to be updated to mention this as then the script would no longer work for in-tree builds (if we ever do that with cmake, but I have asked for it before).

Furthermore, that documentation is pretty clear that out of tree builds should set `BUILDDIR`, and with cmake, all builds will be out of tree, so I'm not sure that this changei s actually useful.

💬 achow101 commented on pull request "test: Deduplicate assert_mempool_contents()":
(https://github.com/bitcoin/bitcoin/pull/31338#issuecomment-2492546569)
ACK a0eafc10f94362408f54195ffd5a9237dc1ef638

💬 achow101 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1853082380)
In dd19d076c528a075d02fa66aaf906f96fa314450 "rpc: add getdescriptoractivity"

This check seems superfluous as it isn't possible to have two different block hashes at the same height if both are in the main chain.

💬 achow101 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1853091630)
In dd19d076c528a075d02fa66aaf906f96fa314450 "rpc: add getdescriptoractivity"

nit: `tx->IsCoinbase()` is more readable.

```suggestion
if (!tx->IsCoinbase()) {
```