💬 S3RK commented on pull request "wallet: return error msg for "too-long-mempool-chain"":
(https://github.com/bitcoin/bitcoin/pull/24845#discussion_r1144434340)
I realised my mistake. You can resolve this and ignore my comments. I'll go over the code one more time before I ack

💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1144467948)
Thanks, helpful comments!

- `m_addr_name`: my reasoning for passing `m_addr_name` as string instead of binary is that we likely don't want to implement the binary decoding in each tracing script. I think the string is closer to what users need. However, not opposing to add the binary if there's need.
- `nKeyedNetGroup`: I added the netgroup as argument to better identify connections coming from the same network. For my use-case, it's not problematic if `nKeyedNetGroup` isn't consistent acros
...

💬 glozow commented on pull request "bench: Expand mempool eviction benchmarking":
(https://github.com/bitcoin/bitcoin/pull/27019#discussion_r1144457784)
Can you apply the changes to the other loops too? i.e.
```suggestion
for (size_t put_index{0}; put_index < num_puts; ++put_index) {
```

💬 glozow commented on pull request "bench: Expand mempool eviction benchmarking":
(https://github.com/bitcoin/bitcoin/pull/27019#discussion_r1144470215)
Wait, if you're using `txns[put_index]` *and* you're using the same `txns` reference each time you call `add_parents_child`, aren't you having all subsequent packages spend outputs from txns[0:24] too? That means all these packages conflict with each other.

You could fix this by adding `set_num * package_size` or something to the index. But what would probably make a better interface is if `add_parents_child` returns the list of transactions it creates, and you append them to your larger list
...

💬 glozow commented on pull request "bench: Expand mempool eviction benchmarking":
(https://github.com/bitcoin/bitcoin/pull/27019#discussion_r1144457060)
Didn't remove?

💬 MarcoFalke commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1144475716)
```suggestion
Ticks<std::chrono::seconds>(m_connected);
```

nit, use `Ticks`, or `count_seconds` (if you want the compile failure if m_connected is changed to higher precision than seconds)?

🚀 fanquake merged a pull request: "Refactor: Remove unused FlatFilePos::SetNull"
(https://github.com/bitcoin/bitcoin/pull/27289)

💬 fanquake commented on pull request "Refactor: Remove unused FlatFilePos::SetNull":
(https://github.com/bitcoin/bitcoin/pull/27289#issuecomment-1479227767)
Door open for anyone to followup with more `::SetNull()` removal.

💬 fanquake commented on issue "test: `interface_bitcoin_cli.py --descriptors` failure under `--valgrind`":
(https://github.com/bitcoin/bitcoin/issues/27281#issuecomment-1479232890)
> What is the combined log?

Unfortunately don't have one, and cannot seem to recreate this failure. Happy to close until have more useful info.

💬 MarcoFalke commented on issue "test: `interface_bitcoin_cli.py --descriptors` failure under `--valgrind`":
(https://github.com/bitcoin/bitcoin/issues/27281#issuecomment-1479235761)
ok. Let's reopen then

✅ MarcoFalke closed an issue: "test: `interface_bitcoin_cli.py --descriptors` failure under `--valgrind`"
(https://github.com/bitcoin/bitcoin/issues/27281)

💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1479241540)
re-ACK 95ad70ab652ddde7de65f633c36c1378b26a313a 🚥

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 95ad70ab652ddde7de65
...

🚀 fanquake merged a pull request: "test: Fix TypeError (expected str instance, bytes found) in wait_for_debug_log"
(https://github.com/bitcoin/bitcoin/pull/27280)

📝 fanquake opened a pull request: "guix: import/sync python-lief (0.12.3) package definition from upstream"
(https://github.com/bitcoin/bitcoin/pull/27296)
Update to version 0.12.3.
Retain our PPC64 patch.
Mention when we can drop our local definition.

Also switch to using cmake-minimal (see #27172), which fixes atleast one build failure I've seen on aarch64, where cmake dependencies fail to build. Fix that by using the cmake without all the dependencies we don't actually need:
```bash
The following derivations will be built:
/gnu/store/7qqvqq2g7l5ylrjv0gn6zha565a12kar-python-lief-0.12.1.drv
/gnu/store/f9zwh1ldy63ga0i5w6cbbqlj6sfq226j-
...

💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1144532693)
Same commit: Can remove the now unused constructors for all other structs as well? See diff, plus unrelated cleanup: (Maybe for a follow-up to avoid another round of re-review?)


```diff
diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp
index 012b0eb321..975dace633 100644
--- a/src/test/checkqueue_tests.cpp
+++ b/src/test/checkqueue_tests.cpp
@@ -57,7 +57,6 @@ struct FakeCheckCheckCompletion {
struct FailingCheck {
bool fails;
FailingCheck(bool _fa
...

💬 hebasto commented on pull request "guix: import/sync python-lief (0.12.3) package definition from upstream":
(https://github.com/bitcoin/bitcoin/pull/27296#issuecomment-1479266165)
Concept ACK.

💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1144539820)
> Maybe for a follow-up to avoid another round of re-review?

Yes, please :)

💬 darosior commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1479296136)
Concept ACK. Have you looked into a fuzz target for this parser?

💬 fanquake commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1144580869)
In 7a9d2fb8cf11eef8b6bd87ad1066df374bf3b620: Make these Doxygen comments?

💬 fanquake commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1144582632)
In 7a9d2fb8cf11eef8b6bd87ad1066df374bf3b620: These could be
```suggestion
constexpr static size_t RFC8439_KEYLEN{32};
```

💬 fanquake commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1144584874)
In 7a9d2fb8cf11eef8b6bd87ad1066df374bf3b620: nit use c++ headers, i.e `<cassert>`