👍 TheCharlatan approved a pull request: "wallet: Implement independent BDB parser"
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-2051323067)
Re-ACK a0943b812ef38826a4ee2732af5f24e470281117

The test improvements look good and give some more confidence.

Can you run the commits with newly added files through `contrib/devtools/clang-format-diff`? There are a bunch of simple whitespace issues.

It would also be nice to mention the byte-swapped database type and that it is only used for testing purposes until the final deprecation in the pull request description.

💬 TheCharlatan commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1597586524)
In commit 9dd2b9aa228f5d30b3620840d893b5a50daafe49:
Nit: Can you also say why this database type is generated in the commit message? Something like "This is added to make testing other endian databases easier.".

💬 TheCharlatan commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1597604870)
Nit: Would be nice to say what LSN means, e.g. "Check all LSNs (log sequence numbers)..."

💬 paplorinc commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1597623211)
we could use `CHECKSUM_SIZE` constant here as well

💬 paplorinc commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1597623319)
instead of preallocation we could do a reserve here:
```C++
data ret;
ret.reserve(CHECKSUM_SIZE);
```

💬 iw4p commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1597626699)
Great point, Thank you.

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-2106231136)
silent merge conflict with https://github.com/bitcoin/bitcoin/pull/29939

💬 TheCharlatan commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2106231470)
There is an error in commit fd342cb40b63ca00d23946743a038847673f8726, which can be fixed with:
```diff
diff --git a/src/warnings.cpp b/src/warnings.cpp
index 5b6ace63dd..40043cd8e7 100644
--- a/src/warnings.cpp
+++ b/src/warnings.cpp
@@ -11,0 +12 @@
+#include <util/string.h>
```

💬 Saraeutsza commented on pull request "wallet: Target a pre-defined utxo set composition by adjusting change outputs":
(https://github.com/bitcoin/bitcoin/pull/29442#issuecomment-2106232151)
remyers:2024-05-bnb-excess

💬 paplorinc commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1597631104)
would it make sense to preallocate the vouts before pushing?
```C++
txNew.vout.reserve(txNew.vout.size() + vecSend.size());
```

💬 paplorinc commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1597631354)
nit: found it a but confusing that we have two different style comments for the same line

💬 TheCharlatan commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2106237892)
The check script is very useful, could you add it as the first commit? I think it could make review a bit easier.

💬 hebasto commented on pull request "Correct tooltip wording for watch-only wallets":
(https://github.com/bitcoin-core/gui/pull/792#issuecomment-2106238212)
@hernanmarino

Are you still working on this?

👍 hebasto approved a pull request: "Bugfix: GUI: Help messages already have a trailing newline, so don't add an extra one"
(https://github.com/bitcoin/bitcoin/pull/29658#pullrequestreview-2051376920)
ACK d1ed09a7643b567e021b2ecb756bc925c48fc708, tested on Ubuntu 24.04.

This change indeed aligns the behavior of `bitcoin-qt` with that of `bitcoind` and other Bitcoin Core binaries.

💬 sipa commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1597636692)
I'd prefer to just call this BECH32, and come up with another name for variants that have other limits.

🚀 hebasto merged a pull request: "Bugfix: GUI: Help messages already have a trailing newline, so don't add an extra one"
(https://github.com/bitcoin/bitcoin/pull/29658)

💬 hebasto commented on pull request "refactor: interfaces, make 'createTransaction' less error-prone ":
(https://github.com/bitcoin-core/gui/pull/807#issuecomment-2106255970)
While introducing a new `wallet/util_spend.h` header, it seems reasonable to address all related IWYU warnings, no?

💬 hebasto commented on pull request "refactor: interfaces, make 'createTransaction' less error-prone ":
(https://github.com/bitcoin-core/gui/pull/807#issuecomment-2106256091)
cc @achow101 @ryanofsky

💬 cbergqvist commented on pull request "Bugfix: RPC/Mining: getblocktemplate: Delay updating nTransactionsUpdatedLast and time_start until after the new template is cached":
(https://github.com/bitcoin/bitcoin/pull/30088#issuecomment-2106258218)
A unique lock on `cs_main` is taken on line 605. It is temporarily released `if (!lpval.isNull())`, but re-locked again before exiting that if-block, well before the block with the static variables around line 740 and the rest of the function body. So only one thread should be executing that section at a time.

That would mean having a null `pindexPrev` should be enough for the null `pblocktemplate` to be recalculated after an OOM. But maybe something less obvious is happening here?

💬 jlopp commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2106262769)
TACK 06c2c713c52b60231efc3e00d2c5eb0bf9e345f9