💬 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>`

💬 fanquake commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1144588967)
Please don't add `for xyz` comments. They are unmaintainable, and if someone wants to know why an include is used, they can look at IWYU etc. Note that you could also add newly-added files to `ci/test/06-script_b.sh` to have the includes checked.

💬 fanquake commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1144594272)
```suggestion
std::memcpy(header_and_contents.data(), &flags, BIP324_HEADER_LEN);
```
Same for memset etc.

💬 fanquake commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1144580119)
In 7a9d2fb8cf11eef8b6bd87ad1066df374bf3b620:
It is fairly awkward to introduce a second copy of `timingsafe_bcmp` (one already in `chacha_poly_aead`), with a different name, and broken #ifdef logic, just to then rename it when you delete the original `timingsafe_bcmp` in a later commit. Although I guess re-using/extracting `timingsafe_bcmp` early from the to-be-deleted file is also a bit awkward.