💬 paplorinc commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1597600777)
nit:
```suggestion
* point to an unused file location where separator fields will be written, followed by the serialized CBlock data.
```

💬 paplorinc commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1597601896)
does `[[nodiscard]]` still make sense now?

💬 paplorinc commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1597601920)
+1

💬 paplorinc commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1597603939)
would it make sense to add a Python test which runs this exact script and checks that the result equals `NUMS_H_DATA`?
Maybe there's something like this already, found parts of it, could you please check? In that case we wouldn't need to put code into a comment, we could just point to the test instead.

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1597605164)
nit: BOOST_CHECK_MESSAGE often produces more readable results, but it may be inconvenient to set it up properly. If you think it adds value, it may be helpful to add some debug info as a message in case of likely failures.

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1597605647)
nit: would it make sense to extract the `96` to a const?
```C++
constexpr size_t SECP256K1_KEYPAIR_SIZE = 96;
using KeyType = std::array<unsigned char, SECP256K1_KEYPAIR_SIZE>;
```

💬 paplorinc commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1597608363)
is this 7 actually a `CHECKSUM_SIZE + 1`, or completely unrelated?
```suggestion
if (pos == str.npos || pos == 0 || pos + CHECKSUM_SIZE >= str.size()) {
```

👍 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?