💬 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)) {
```

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

nit: snake_case

```suggestion
for (size_t vin_idx = 0; vin_idx < tx->vin.size(); ++vin_idx) {
```

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

`spk_1` is unused.

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

This comment doesn't make sense to me. It suggests that a different child transaction has spent the coin. But what actually happens in this case is that this transaction (which is already in the mempool) is the child of a transaction that is also in the mempool.

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

This function appears to be unused.

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

These should be scoped in the loop iterating the inputs.

🚀 achow101 merged a pull request: "test: Deduplicate assert_mempool_contents()"
(https://github.com/bitcoin/bitcoin/pull/31338)

💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851951264)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850750700

I think this makes sense for -blocksdir and could make sense for -walletdir. But if the datadir was initiially created by an older version of Bitcoin and since upgraded it won't actually have a wallets/ subdirectory and will just store wallets in the datadir directly. So it could be reasonable to support -nowalletdir or -walletdir="" as a ways to request the same behavior when opening an old datadir. However, current wal
...

💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851914389)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850720954

I could be missing something but it seems pretty obvious to me that -nocolor should be the same as -color=never. I don't think this PR should change that, but it might make sense to have a PR in the future that removes unjustified uses of disallow_negation so options so parsing is more uniform and less surprising. It could be the same PR that adds disallow_negation to prevent -noblocksdir as suggested https://github.com/
...

👍 ryanofsky approved a pull request: "util: Improve documentation and negation of args"
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2451143073)
Code review ACK 2bf7e59776fcfb2c8a477cf2abb59606f7b4aacc. This is a nice improvement over status quo, but I'm pretty sure two of the commits 21bc3a3657fea5ed44172963639ee8ca5cd8e46c and 879a8d3a6820a86e6faa8d91f92d0cf374700fb8 are more complicated than they need to be and less robust than they could be.

I posted a simplified [branch](https://github.com/ryanofsky/bitcoin/commits/review.31212.4-edit) ([diff](https://github.com/ryanofsky/bitcoin/compare/review.31212.4..review.31212.4-edit)) and
...

💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853186542)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850778259

As long as you are only adding the test coverage *after* fixing the bug (like this PR) I think the question of whether the test changes are in the same commit or a separate one are basically a question of personal style, and different developers will have different preferences.

But a much better practice IMO is to add the new test coverage in a separate commit *before* fixing the bug. Then the bugfix commit will inclu
...

⚠️ Parkeragan opened an issue: " "$comment": "Newer unreleased libevent versions cause https://github.com/bitcoin/bitcoin/issues/30096","
(https://github.com/bitcoin-core/gui/issues/846)
https://github.com/Parkeragan/bitcoin/blob/master/vcpkg.json#L60