💬 laanwj commented on pull request "net, init: derive default onion port if a user specified a -port":
(https://github.com/bitcoin/bitcoin/pull/31223#issuecomment-2466206753)
> Another alternative: don't listen for Tor on regtest by default (were all reports for this for regtest?).

Agree with @mzumsande in that i don't particularly like this; remember that regtest is primarily for testing mainnet behavior in our functional tests, so we want the behavior to be as close to that as possible. Every extra flag needed makes for more test-only code.

💬 laanwj commented on pull request "init: warn, don't error, when '-upnp' is set":
(https://github.com/bitcoin/bitcoin/pull/31198#issuecomment-2466212369)
Maybe unknown settings in settings.json should never be a fatal startup error. But it also shouldn't be silently ignored (in case the setting changed important behavior). It could warn once at startup that they are there then remove them. This is probably the most user friendly way, as we don't really ever want to force GUI users to manually edit a json file?

💬 laanwj commented on pull request "doc: Fix dead links to mailing list archives":
(https://github.com/bitcoin/bitcoin/pull/31240#issuecomment-2466216308)
Concept ACK. It's starting to look unlikely that the archives will be back.

i slightly prefer the current approach to relying on the server-side URL mapping script, because `pi` archives are super easy to mirror locally, and then the canonical link will (i assume) just port over to the new domain. Also setting up the mapping script is more work.

💬 edilmedeiros commented on pull request "doc: Fixup bitcoin-wallet manpage chain selection args":
(https://github.com/bitcoin/bitcoin/pull/31264#issuecomment-2466218323)
Concept ACK.

I understand and like the rational for not being repetitive, but don't you think the terminology might be confusing? I could not think of any better alternatives, though.

💬 edilmedeiros commented on pull request "doc: Fix dead links to mailing list archives":
(https://github.com/bitcoin/bitcoin/pull/31240#issuecomment-2466219877)
I personally prefer to update the links because I can't imagine the mess of updating this again in the future if gnusha is taken down and we end up with two indirections on the URLs.

💬 theStack commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2466227961)
Concept ACK

💬 furszy commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1835445136)
sure. Done as suggested.

💬 furszy commented on issue "importdescriptors always rescans":
(https://github.com/bitcoin/bitcoin/issues/31263#issuecomment-2466279297)
> The docs say that '"now" can be specified to bypass scanning' but that doesn't appear to be true

The message also specify that "Blocks up to 2 hours before the earliest timestamp of all descriptors being imported will be scanned as well as the mempool".
This occurs regardless of the provided timestamp. It serves as a grace period to account for possible variations in block times.

👍 tdb3 approved a pull request: "bench: add support for custom data directory"
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2425519599)
re ACK b842ad61c7d9507db415b5d6225f5444f6bf2654

Liking the use of time rather than random.

Re-ran https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2401711134

```
'tmp/test_common bitcoin/BlockAssemblerAddPackageTxns/1731173028231274080'
```

💬 tdb3 commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1835472395)
non-blocking nit:
Since time is being used now, the name isn't quite as random as before. Could update the comment and `rand_str` name.

💬 furszy commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1835476601)
sure, pushed. Thanks

👍 tdb3 approved a pull request: "test: Shut down framework cleanly on RPC connection failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2425524929)
Code Review re ACK 68b0530d5fe867dbee5e551ecf6d491fecbc77f1

👍 tdb3 approved a pull request: "bench: add support for custom data directory"
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2425525188)
re ACK 5adb4a44776de3f9295cd6bf965f8beba0e4e368

Change since last push is a simple removal of a semi-accurate comment.

👍 tdb3 approved a pull request: "test: Introduce ensure_for helper"
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2425525972)
Code Review re ACK f04529fc40f2f7c8e4e63d892cdcbe6c91dfbf96

Would also review again if the suggestion in https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1834183665 was implemented.

✅ achow101 closed an issue: "importdescriptors always rescans"
(https://github.com/bitcoin/bitcoin/issues/31263)

💬 achow101 commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1835479468)
Oops yes.

💬 secp512k2 commented on pull request "doc: Fix missing comma in JSON example in REST-interface.md":
(https://github.com/bitcoin/bitcoin/pull/31259#issuecomment-2466381262)
> LLM-PR?
>
> Not sure how useful it is to fix the example output in an interface doc. LLM operator and reviewer time is probably spent better elsewhere.

Not a bot. Just thought having accurate docs would be helpful for a newcomer troubleshooting. I understand the limited PR review capability though, and will limit to more major things.

💬 luke-jr commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2466406625)
>Disallow -noconf since a config file is required

But it's not. Seems like `-noconf` should work to ignore the config file.

🤔 tdb3 reviewed a pull request: "rpc: increase the defaults for -rpcthreads and -rpcworkqueue"
(https://github.com/bitcoin/bitcoin/pull/31215#pullrequestreview-2425588523)
Approach ACK

Performed a very simplistic sanity check comparing 6463117a29294f6ddc9fafecfd1e9023956cc41b (commit under head) to head (e56fc7ce6a92eae7e80657d9f57a148cc002358d).

Ran `bitcoin-cli gettxoutsetinfo` 8x simultaneously on a system with 4 cores and timed the result (not claiming this to be the best test, just a quick check). The idea was to see if a core-constrained system would see a performance degradation from the PR change.

Without the increase of rpcthreads and rpcworkqueu
...

💬 maciejsszmigiero commented on pull request "dbwrapper: Bump LevelDB max file size to 128 MiB to avoid system slowdown from high disk cache flush rate":
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2466468043)
@l0rinc
> are you still working on this or should we take over?

I can obviously change the default in this PR to 16 MiB but I think having #30059 is important too: as you measured here on Oct 2 the best performing size on HDD storage actually seems to be 32 MiB.