💬 rkrux commented on pull request "test: fix MiniWallet script-path spend (missing parity bit in leaf version)":
(https://github.com/bitcoin/bitcoin/pull/30076#issuecomment-2106153073)
@theStack Thanks for the quick update on this, reACK [57eb590](https://github.com/bitcoin/bitcoin/pull/30076/commits/57eb59024b4d5b776ddb08736a81585c2ea0e734)

💬 rkrux commented on pull request "test: add conflicting topology test case":
(https://github.com/bitcoin/bitcoin/pull/30066#discussion_r1597573455)
Oh I see, it makes sense to me now.

💬 rkrux commented on pull request "test: add conflicting topology test case":
(https://github.com/bitcoin/bitcoin/pull/30066#discussion_r1597573798)
However, I do feel that the description can be updated to let others know that this topology will be relaxed in the future and not now.

👍 rkrux approved a pull request: "test: add conflicting topology test case"
(https://github.com/bitcoin/bitcoin/pull/30066#pullrequestreview-2051309716)
tACK [fba1565](https://github.com/bitcoin/bitcoin/pull/30066/commits/fba1565f4b6bafbf2516f03184cf58aa80d9843f)

💬 hebasto commented on pull request "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP":
(https://github.com/bitcoin/bitcoin/pull/29790#issuecomment-2106172833)
Rebased and updated with usage of https://github.com/hebasto/bitcoin/pull/184.

👍 paplorinc approved a pull request: "blockstorage: Separate reindexing from saving new blocks"
(https://github.com/bitcoin/bitcoin/pull/29975#pullrequestreview-2051341978)
Nice, only left a few nit comments

💬 paplorinc commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1597600395)
nit: extra line

💬 paplorinc commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1597601015)
nit:
```suggestion
* During reindex, only the block file statistics are updated.
```

💬 paplorinc commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1597602370)
nit (in commit message):
```
This would means that MaxBlockfileNum
```
vs
```
This would mean that MaxBlockfileNum
```

💬 paplorinc commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1597601611)
nit:
```suggestion
// Account for the 4 magic message start bytes + the 4 length bytes (8 bytes total, defined as BLOCK_SERIALIZATION_HEADER_SIZE).

```

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