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

👍 hebasto approved a pull request: "bench: enable wallet creation benchmarks on all platforms"
(https://github.com/bitcoin/bitcoin/pull/30122#pullrequestreview-2064368273)
re-ACK 7c8abf3c2001152423da06d25f9f4906611685ea, I agree with changes since my recent [review](https://github.com/bitcoin/bitcoin/pull/30122#pullrequestreview-2061694682).

💬 brunoerg commented on issue "Wallet fuzzing tracking issue":
(https://github.com/bitcoin/bitcoin/issues/29901#issuecomment-2118365692)
Added https://github.com/bitcoin/bitcoin/pull/30134 to the list for review. It adds more coverage for `ScriptPubKeyMan`.

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

Convinced myself that this parser works reliably partly by looking at the original code, partly by extending the test framework's BDB parser to match this PR's logic (see #30125), which helped a lot in understanding the structure. Also tested with a few random signet legacy wallets that I still had around, though those were all quite small.

Btw, I couldn't manage to find or create a BDB wallet where the DELETE flag is set in a record header (wou
...

💬 theStack commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1605585592)
typo nit
```suggestion
parser.add_argument("--swap-bdb-endian", action="store_true",help="When making Legacy BDB wallets, always make them byte swapped internally")
```

💬 theStack commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1605553167)
nit: isn't thrown anywhere, can be removed
```suggestion
```

💬 theStack commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1605552180)
nit: isn't thrown in the parser (commented out), hence can be removed here
```suggestion
```

💬 jonatack commented on pull request "lint/contrib/doc updates":
(https://github.com/bitcoin/bitcoin/pull/30084#issuecomment-2118449438)
> If anything, I think the docs could be updated in some way to say that running the linters outside the container, or with versions that differ from the CI is generally unsupported.

Thanks for the feedback, done. Open to further suggestions (like whether to update the other dependencies).

⚠️ asp2alirezasusanichezeh opened an issue: "#asusani"
(https://github.com/bitcoin/bitcoin/issues/30135)

✅ willcl-ark closed an issue: "#asusani"
(https://github.com/bitcoin/bitcoin/issues/30135)

💬 ariard commented on pull request "policy: restrict all TRUC (v3) transactions to 10KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2118488640)
I think 10kvb is okay as a limit though pray for someone telling it more to LN folks.

There is very likely 2018 / 2019 channels open with `max_accepted_htlcs` at max 483 both-sides. With no dynamic upgrades deployed, no way to restraint, without force-close and re-open. Worst than pinning exposure is no propagating TRUC transaction due to “naive” upgrade.

In the (far) future, 10kvb could be dynamic at the transaction-relay level, though this would assume LN upgrading its channel policy set
...

:lock: fanquake locked an issue: "#asusani"
(https://github.com/bitcoin/bitcoin/issues/30135)

📝 rustyrussell opened a pull request: "doc: note that you can assume C++20."
(https://github.com/bitcoin/bitcoin/pull/30136)
We check this in configure.ac, line 99:

dnl Require C++20 compiler (no GNU extensions)

This was introduced in:

commit fa67f096bdea9db59dd20c470c9e32f3dac5be94
Author: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Date: Sun Aug 27 10:45:39 2023 +0200

build: Require C++20 compiler

Which git says was before v27.0rc1:

$ git describe --contains fa67f096bdea
v27.0rc1~224^2~4

<!--
*** Please remove the following help text before submitting: ***

Pull requests without
...

💬 fanquake commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1605655974)
If we are outlawing thread_local usage, then at the same time, this commit should remove other comments from our codebase suggesting further thread_local usage. i.e [ * If thread-safety is needed, the global could be made thread_local (given](https://github.com/bitcoin/bitcoin/blob/058af75874ffa2b4064e3d6d30cc50f0ec754ba8/src/test/util/random.h#L17)

💬 laanwj commented on pull request "doc: note that you can assume C++20.":
(https://github.com/bitcoin/bitcoin/pull/30136#discussion_r1605657935)
It's useful to document what version of the C++ standard can be used, but i don't think this needs to mention the version it was introduced (the document always applies to the branch it is in), nor is this really the place for documenting a compiler requirement.

Would word it as "Code needs to adhere to the C++20 standard.".