💬 rkrux commented on pull request "test: fix MiniWallet script-path spend (missing parity bit in leaf version)":
(https://github.com/bitcoin/bitcoin/pull/30076#discussion_r1597569771)
Yes, this is also good.

💬 rkrux commented on pull request "test: fix MiniWallet script-path spend (missing parity bit in leaf version)":
(https://github.com/bitcoin/bitcoin/pull/30076#discussion_r1597570020)
Great, thanks for sharing, I'm digging into this!

💬 levantah commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2106151512)
Tested ACK, it works my current tip is

```
2024-05-12T07:22:24Z UpdateTip: new best=00000000000000165fe9709ac837645716d480a230a61ca5745530f6692a235d height=24361 version=0x2e5e2000 log2_work=65.708890 tx=94453 date='2024-05-12T07:22:17Z' progress=1.000000 cache=3.3MiB(27967txo)
```

But, the `bitcoin-cli` in 06c2c713c52b60231efc3e00d2c5eb0bf9e345f9 does not know anything about `testnet4` (different RPC port) and I would personally prefer also `-chain=test4` additional flag (both for `bitc
...

💬 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()) {
```