💬 0xB10C commented on pull request "doc: Fix missing comma in JSON example in REST-interface.md":
(https://github.com/bitcoin/bitcoin/pull/31259#issuecomment-2466122408)
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.

✅ maflcko closed a pull request: "doc: Clarify GUI Compilation Instructions for Qt Libraries in build-n…"
(https://github.com/bitcoin/bitcoin/pull/31261)

💬 maflcko commented on pull request "doc: Clarify GUI Compilation Instructions for Qt Libraries in build-n…":
(https://github.com/bitcoin/bitcoin/pull/31261#issuecomment-2466187878)
I don't see how this is an improvement. I think the previous wording was even more explicit in mentioning libqrencode.

Closing for now. Generally, when there isn't a bug or obvious mistake, the style is left as-is. Shuffling around wording in one way or the other doesn't improve anything and can be done endlessly (with or without the help of LLMs). However, the review bandwidth is limited.

💬 maflcko commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1835348907)
should be 28?

💬 maflcko commented on pull request "ci: make ctest stop on failure":
(https://github.com/bitcoin/bitcoin/pull/31257#issuecomment-2466191528)
lgtm ACK 36a22e5683375b7925767de9daa9df4c48831c15

I think the same happened when the project was using `make`, but I haven't double-checked.

💬 maflcko commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2466194861)
> Ok, that force-push actually contained a behavior change.

Ugh, I missed that, I didn't see the `!` negation. Generally writing a simple if with an else branch as `if (!a) ...` is confusing, because each branch will have a (double) negation. It would be better to just write `if (a)` and have a single negation in the else branch. But you didn't change that code, so it can be done in a follow-up.

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

Only change is the default behavior change, which I somehow missed.

💬 maflcko commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1835351305)
nit: My preference would be to use the current time (maybe in nanosecond precision) and make the path `time/test_name`. This way, it is easy to find the last test execution easily, if there is more than one.

With the current approach, one would have to look up which rand32 corresponds to the last execution.

⚠️ gmart7t2 opened an issue: "importdescriptors always rescans"
(https://github.com/bitcoin/bitcoin/issues/31263)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Running `importdescriptors` with `timestamp: now` rescans the last 2 hours worth of blocks.

The docs say that '"now" can be specified to bypass scanning' but that doesn't appear to be true:

```
"timestamp": timestamp | "now", (integer / string, required) Time from which to start rescanning the blockchain for this descriptor, in UNIX epoch time

...

📝 maflcko opened a pull request: "doc: Fixup bitcoin-wallet manpage chain selection args"
(https://github.com/bitcoin/bitcoin/pull/31264)
The sentence is missing `-testnet4` and `-chain`. Instead of duplicating the full list (and having to keep it in sync), just refer to them as `(test)chain selection arguments`.

💬 maflcko commented on pull request "Update manpage descriptions":
(https://github.com/bitcoin/bitcoin/pull/29686#discussion_r1835353534)
Fixed in https://github.com/bitcoin/bitcoin/pull/31264

💬 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'
```