🤔 furszy reviewed a pull request: "test: test_bitcoin: allow -testdatadir=<datadir>"
(https://github.com/bitcoin/bitcoin/pull/26564#pullrequestreview-1798560817)
Left few comments

💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1437956165)
Could mention that the datadir will not be removed automatically.

Also, the default value isn't just a random string. It should be:
```suggestion
m_node.args->AddArg("-testdatadir", strprintf("Custom data directory (default: %s<random_string>)", fs::PathToString(fs::temp_directory_path() / "test_common_" PACKAGE_NAME / "")), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
```

💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1437960487)
Same here; let's avoid using the string pointer `c_str()`, and instead use `fs::PathFromString(path)`

💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1437962170)
Is this because you are running all tests within a boost fixture test suite or is it happening when you run a single test case?
E.g. fixture test suite: `./test_bitcoin --run_test=db_tests` vs single test case `./test_bitcoin --run_test= db_tests/getwalletenv_file`.

💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1437963129)
Can use `GetPathArg("-testdatadir")` directly.

Also, I would check the path correctness here and error out early when it is invalid.

💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1437959375)
Let's avoid using the string pointer (`c_str()`), could instead use:
```c++
testdatadir = fs::PathFromString(g_insecure_rand_ctx_temp_path.rand256().ToString())
```

💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1437965595)
If a concurrent process locks the datadir right after the lock release, this process would remove the root path even when the other process is using it?

🤔 furszy reviewed a pull request: "index: block filters sync, reduce disk read operations by caching last header"
(https://github.com/bitcoin/bitcoin/pull/28955#pullrequestreview-1798579472)
Finished testing this on a custom directory. Results are favorable @Sjors and @luke-jr.
I have observed that access to the OS temp directory is faster than accessing regular directories locally. Benchmark outputs are provided at the end.

The testing methodology is as following:

Running `./bench_bitcoin -filter="BlockFilterIndexSync" -testdatadir=<custom_test_datadir_path>` on any of the following branches:

[Branch 1](https://github.com/furszy/bitcoin-core/tree/2023_index_blockfilter_ca
...

💬 luke-jr commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#issuecomment-1871716006)
There we go, tests all happy now

💬 luke-jr commented on pull request "Remove `dnsseed.bitcoin.dashjr.org` temporarily":
(https://github.com/bitcoin/bitcoin/pull/29149#issuecomment-1871717493)
> just fix (or explain if there is a legit reason) the issue with only returinng old nodes.

They weren't old nodes when that change was made - they were the latest, intentionally excluding old nodes which didn't enforce Taproot.

I already updated it last night after you brought it to my attention.

However, it should be noted that the nodes returned were perfectly fine, and there wasn't actually a real issue.

>Removal is warranted as explained in PR description or we need to change po
...

💬 1440000bytes commented on pull request "Remove `dnsseed.bitcoin.dashjr.org` temporarily":
(https://github.com/bitcoin/bitcoin/pull/29149#issuecomment-1871732739)
> However, it should be noted that the nodes returned were perfectly fine, and there wasn't actually a real issue.

They were missing a few important bug fixes

> The PR description is pure lies. You are just outing yourself as a bad actor.

You wanted questions in https://github.com/bitcoin/bitcoin/pull/29149#issuecomment-1871680509 which I shared in https://github.com/bitcoin/bitcoin/pull/29149#issuecomment-1871700172 and are not answered yet

⚠️ Laughter79 opened an issue: "Hi"
(https://github.com/bitcoin/bitcoin/issues/29150)
https://www.tiktok.com/t/ZPRcUSXeB/

💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1438055454)
Good idea, but what do you mean by path correctness?

💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1438055076)
I can't get this to work.

💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1438054972)
The `g_printed_dir` guard is only needed because the user may be running more than one test case (like your first example). I don't love this global variable but it seems weird to print this out on every individual test case, and I think it's okay for a test program. I convinced myself that no race condition is possible, but we could use `std::call_once()` here but I didn't think it was worth it.

💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1438057148)
Good catch, I was actually aware of this but concluded that it is such a small timing window, that it's not a problem in practice. The problem is that on Windows, you can't remove an open file (and having a file lock is a form of having the file open). I could make this conditional on `WIN32` (I think it is), but I'm not sure that's even worth it.

💬 ns-xvrn commented on pull request "Remove `dnsseed.bitcoin.dashjr.org` temporarily":
(https://github.com/bitcoin/bitcoin/pull/29149#issuecomment-1871798839)
I agree with @mzumsande and request the author to close this PR.

💬 1440000bytes commented on pull request "Remove `dnsseed.bitcoin.dashjr.org` temporarily":
(https://github.com/bitcoin/bitcoin/pull/29149#issuecomment-1871799612)
In case its not clear many new devs/users: this is not reddit and up/down vote will be meaningless. Either PR gets merged or not and maintainer decide it.

💬 ns-xvrn commented on pull request "Remove `dnsseed.bitcoin.dashjr.org` temporarily":
(https://github.com/bitcoin/bitcoin/pull/29149#issuecomment-1871801467)
You're not a dev, you're just a troll harassing devs.

💬 anibilthare commented on pull request "init: handle empty settings file gracefully":
(https://github.com/bitcoin/bitcoin/pull/29144#discussion_r1438073495)
I agree with @shaavan [here](https://github.com/bitcoin/bitcoin/pull/29144#pullrequestreview-1798049445) . What if we simply modify the error message here and keep rest of the code as is to be more user friendly. Referring to https://github.com/bitcoin/bitcoin/pull/23096, https://github.com/bitcoin/bitcoin/pull/22591, and https://github.com/bitcoin/bitcoin/issues/21340#issuecomment-880147010 as mentioned earlier, following change can be added to the error message
```suggestion
errors.
...