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

💬 TheCharlatan commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2118336056)
Updated 856c8563ca342303a83977146df22768bb6e2c7e -> f2f50a4db2f61b767fd16ca2e99e136eb7dff8a6 ([mempoolArgs_10](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_10) -> [mempoolArgs_11](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_11), [compare](https://github.com/TheCharlatan/bitcoin/compare/mempoolArgs_10..mempoolArgs_11))

* Addressed @achow101's [comment](https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1605494321), fixing unreachable Assert.
* Slightly adjusted
...

💬 brunoerg commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-2118348115)
I couldn't reproduce on MacOS 14.3 with that input file.

> If I add limits (1024 for example) to ConsumeRandomLengthByteVector and ConsumeRandomLengthString the harness works. But that could be defeating the purpose of fuzzing.

Changing the harness can invalidate the input. Also, `ConsumeRandomLengthByteVector` is used in a lot of other targets without a limit. Does it happen for other targets as well?

📝 theStack opened a pull request: "test: remove unneeded `-maxorphantx=1000` settings"
(https://github.com/bitcoin/bitcoin/pull/30133)
It's unclear what the motivation for increasing the orphan pool is here, and it seems that this not needed at all. None of these tests involve orphan transactions explicitly, and if they would occur occasionally, there is no good reason to prefer a value of 1000 over the default of 100 (see DEFAULT_MAX_ORPHAN_TRANSACTIONS).

📝 brunoerg opened a pull request: "fuzz: add more coverage for `ScriptPubKeyMan`"
(https://github.com/bitcoin/bitcoin/pull/30134)
This PR adds more coverage for `ScriptPubKeyMan`:

- Check `GetKey` and `HasPrivKey` after adding descriptor key.
- Cover `GetEndRange` and `GetKeyPoolSize`.
- Cover `MarkUnusedAddresses` with the scripts from ScriptPubKeys and `GetMetadata` with the destinations from them.

Also, this PR removes unnecessary includes from `fuzz/scriptpubkeyman.cpp`