💬 TheCharlatan commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1605278661)
Mmh, I think I prefer this as is.

💬 edilmedeiros commented on pull request "contrib/signet/miner: increase miner search space":
(https://github.com/bitcoin/bitcoin/pull/30130#issuecomment-2117967175)
A small caveat: I couldn't get rid of the `Could not satisfy difficulty target` (see the debug log just before timestamp `2024-05-17 12:56:01`). This is emitted by `bitcoin-util` itself before failing. I could not find a way to make the Python script intercept that to keep the terminal clean. I fear the complexity to achieve such an effect would not justify the benefit.

👍 theuni approved a pull request: "util: avoid using thread_local variable that has a destructor"
(https://github.com/bitcoin/bitcoin/pull/30095#pullrequestreview-2063982122)
Agree with @laanwj's take.

utACK d35ba1b3f16071b8fe9b36398ba15352dbf2a54d

👍 theuni approved a pull request: "build: Enable `thread_local` for MinGW-w64 builds"
(https://github.com/bitcoin/bitcoin/pull/30099#pullrequestreview-2063984747)
utACK 5822a82a88e46ef08e5c18c41489ffd3e9d899c6.

After this and #30095, is there any need to keep the thread_local option at all?

💬 theuni commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2118018627)
@willcl-ark Thanks, that's very helpful. Out of curiosity, what build options are you using for these? Are optimizations on?

💬 edilmedeiros commented on pull request "Update manpage descriptions":
(https://github.com/bitcoin/bitcoin/pull/29686#discussion_r1605307850)
```suggestion
"Usage: bitcoind [options] Start " PACKAGE_NAME "\n"
```

🤔 edilmedeiros requested changes to a pull request: "Update manpage descriptions"
(https://github.com/bitcoin/bitcoin/pull/29686#pullrequestreview-2063952610)
Concept ACK

I would keep the old comment for each command even in face of the additional information provided (see review comments). I tend to rely a lot on those to quickly remember what the commands do (but will not oppose taking them out if this is the consensus). I understand that they may break the formatting of the manual pages, so feel free to discard them.

For the `bitcoin-util` tool, I suggest taking the description from the PR that introduced the tool (see comment).

💬 edilmedeiros commented on pull request "Update manpage descriptions":
(https://github.com/bitcoin/bitcoin/pull/29686#discussion_r1605316111)
```suggestion
"Usage: bitcoin-tx [options] <hex-tx> [commands] Update hex-encoded bitcoin transaction\n"
```

💬 edilmedeiros commented on pull request "Update manpage descriptions":
(https://github.com/bitcoin/bitcoin/pull/29686#discussion_r1605316427)
```suggestion
"or: bitcoin-tx [options] -create [commands] Create hex-encoded bitcoin transaction\n"
```

💬 edilmedeiros commented on pull request "Update manpage descriptions":
(https://github.com/bitcoin/bitcoin/pull/29686#discussion_r1605322723)
```suggestion
"\nUsage: bitcoin-cli [options] <command> [params] Send command to " PACKAGE_NAME "\n"
"or: bitcoin-cli [options] -named <command> [name=value]... Send command to " PACKAGE_NAME " (with named arguments)\n"
"or: bitcoin-cli [options] help List commands\n"
"or: bitcoin-cli [options] help <command> Get help for a command\n"
```

💬 edilmedeiros commented on pull request "Update manpage descriptions":
(https://github.com/bitcoin/bitcoin/pull/29686#discussion_r1605327472)
```suggestion
"The bitcoin-util tool provides bitcoin related functionality that does not rely on the ability to access a running node.\n"
```

Why not use the description from #19937 which introduced the tool?

💬 edilmedeiros commented on pull request "Update manpage descriptions":
(https://github.com/bitcoin/bitcoin/pull/29686#discussion_r1605320367)
Kind of not agree since the tool is intended to be used with Bitcoin Core RPC and maintained for that only purpose. On the other hand, I understand that knots (and other forks) is compatible in this regard with Core and the bitcoin-cli tool will work anyhow without discriminating the server.

💬 tdb3 commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#discussion_r1605333835)
Was talking with @pinheadmz about this. Maybe as an alternative, the test could fetch the `getversion` RPC response (e.g. with "27.0..."), then use `assert_debug_log()` to check that the RPC response matches what's in the log.

🤔 marcofleon reviewed a pull request: "fuzz: wallet, add target for `Crypter`"
(https://github.com/bitcoin/bitcoin/pull/28074#pullrequestreview-2064071922)
I used ASan and UBSan. I think it might just be a mac specifc issue, I'm fuzzing on a mac with m3 chip.

Here's the error:
```bash
INFO: seed corpus: files: 111934 min: 1b max: 5242880b total: 2040057759b rss: 216Mb
#8192 pulse cov: 938 ft: 2162 corp: 81/730b exec/s: 4096 rss: 318Mb
#16384 pulse cov: 1049 ft: 2510 corp: 127/2096b exec/s: 5461 rss: 416Mb
#32768 pulse cov: 1571 ft: 4427 corp: 219/9334b exec/s: 4681 rss: 565Mb
#65536 pulse cov: 1774 ft: 7969 corp: 371/56Kb exec/s: 4681
...

💬 achow101 commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#issuecomment-2118128913)
ACK 9408a04e424cee0d226bde79171bd4954f9caeb0

🚀 achow101 merged a pull request: "crypto: add `NUMS_H` const"
(https://github.com/bitcoin/bitcoin/pull/30048)

💬 sipa 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-2118163439)
It appears that RocksDb (more-developed derivative of LevelDB) uses a default of 64 MiB (https://github.com/facebook/rocksdb/blob/main/include/rocksdb/advanced_options.h#L468). See also my comment in https://github.com/bitcoin/bitcoin/pull/30059#issuecomment-2117888385.

🤔 ryanofsky reviewed a pull request: "refactor: interfaces, make 'createTransaction' less error-prone "
(https://github.com/bitcoin-core/gui/pull/807#pullrequestreview-2064121988)
Concept ACK edc3f9a733aaa1a1cc2547013d09c27828151e8a, but this has some changes I like and other changes I don't think are good. Left a more detailed comment below.

💬 ryanofsky commented on pull request "refactor: interfaces, make 'createTransaction' less error-prone ":
(https://github.com/bitcoin-core/gui/pull/807#discussion_r1605397646)
In commit "gui: remove unreachable AmountWithFeeExceedsBalance error" (355199c7a6c58eb812dd7ef8e204e6e068c142ae)

This is a nice catch noticing that this code doesn't work. But I think the fact that it doesn't work is a pretty unfortunate problem, and the current PR will make fixing it harder.

It seems like #20640 unintentionally introduced a bug where the more accurate error message "The total exceeds your balance when the %1 transaction fee is included" can never trigger, and instead only
...

💬 achow101 commented on issue "stringop-overflow warning with GCC 14":
(https://github.com/bitcoin/bitcoin/issues/30114#issuecomment-2118174319)
> Looks like it derives that `.begin()` is the same as `.end()` which would bring the size to `-1`. This would require `CPubKey::GetLen` to return 0, which is possible if the first byte is invalid. Maybe it'd make sense to make it return `1` in that case because there's always the first byte?

That worked, but since `CPubKey` is used in a ton of places, I'm a little bit hesitant to change `GetLen` since that may have unexpected side effects. However, I do have a fix that seems to work that cha
...