📝 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()) {
```

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

Why not `GetBlockChecked`?

Additionally, both functions will throw if the block was not found on disk, i.e. pruned. However, since multiple blocks may be passed in, perhaps it should continue with scanning the blocks it can find? Although in that case, it should have a way to report to the user which blocks were skipped.

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

I would prefer to omit these fields entirely rather than placing dummy values for unconfirmed transactions. This is what the wallet does in `gettransaction`.

Same below for receive.

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

nit: `.contains()` is more readable

Same below for vouts

```suggestion
if (scripts_to_watch.contains(coin.out.scriptPubKey)) {
```