💬 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
...

📝 achow101 opened a pull request: "wallet, tests: Avoid stringop-overflow warning in PollutePubKey"
(https://github.com/bitcoin/bitcoin/pull/30131)
Fixes #30114

💬 ryanofsky commented on pull request "refactor: interfaces, make 'createTransaction' less error-prone ":
(https://github.com/bitcoin-core/gui/pull/807#issuecomment-2118182017)
I just noticed https://github.com/bitcoin/bitcoin/pull/25269, which I hadn't seen when I posted my comment above. It looks like that PR was closed for inactivity but it does look like a good approach that solves the issue I didn't like in my comment (though maybe it is still a slight regression because it doesn't include the amount of the fee in the error message). I wonder if you'd consider reopening that PR and maybe including the changes in this PR or basing this PR on that one?

Or if you
...

💬 laanwj commented on issue "stringop-overflow warning with GCC 14":
(https://github.com/bitcoin/bitcoin/issues/30114#issuecomment-2118192325)
Yes, could also handle it at another level, though it's strange for a class to return `begin()==end()` when there's actually one byte in it.

💬 achow101 commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#issuecomment-2118259798)
ACK b47bd959207e82555f07e028cc2246943d32d4c3

🚀 achow101 merged a pull request: "kernel: De-globalize fReindex"
(https://github.com/bitcoin/bitcoin/pull/29817)

🤔 furszy reviewed a pull request: "wallet: Implement independent BDB parser"
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-2064293082)
reACK d51fbab4b32d56765e8faab6ad01245fb259b0ca

💬 achow101 commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2118299920)
ACK 856c8563ca342303a83977146df22768bb6e2c7e

💬 achow101 commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1605494321)
```
../../../src/test/fuzz/tx_pool.cpp: In function ‘CTxMemPool {anonymous}::MakeMempool(FuzzedDataProvider&, const node::NodeContext&)’:
../../../src/test/fuzz/tx_pool.cpp:134:1: error: control reaches end of non-void function [-Werror=return-type]
134 | }
| ^
```

Also seems like the `Assert` is unreachable too.

👍 laanwj approved a pull request: "wallet: Implement independent BDB parser"
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-2064310065)
re-ACK d51fbab4b32d56765e8faab6ad01245fb259b0ca

💬 TheCharlatan commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1605497660)
Ugh, I'll push a fix.

📝 TheCharlatan opened a pull request: "indexes: Don't wipe indexes again when continuing a prior reindex"
(https://github.com/bitcoin/bitcoin/pull/30132)
When restarting `bitcoind` during an ongoing reindex without setting the `-reindex` flag again, the block and coins db is left intact, but any data from the optional indexes is discarded. While not a bug per se, wiping the data again is
wasteful, both in terms of having to write it again, as well as potentially leading to longer startup times. So keep the index data instead when continuing a prior reindex.

Also includes some smaller code cleanups around the reindexing code.