💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2332633451)
#### Invalid chars check

Here follows an implementation of the low bar of what I stated in https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2273568357. It's implemented on top of fa72ce66421d3f90a6794b3e54e56873ae81265f.

<details>
<summary>

##### Diff

</summary>

I've confirmed that "aAcdfeEfFgGinopsuxX" matches what I linked to in cppreference and also the Python version being replaced.

```diff

diff --git a/src/test/util_string_tests.cpp b/src/test/util_strin
...

👍 hodlinator approved a pull request: "Remove unsafe uint256S() and test-only uint160S()"
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2284131255)
re-ACK b60013ed71b697bab98884aa475fb64ae7736c2e

`git range-diff master 9ef049d b60013`

Collapsed `conversion` test change into one commit.

New `ParseHashV` commit is more user-friendly.

💬 hodlinator commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1746191005)
nit: The old `conversion` test compared against integer literals here. The `methods` test in *arith_uint256_tests.cpp* compares against `0`, so this is covered in part.

Still, what do you think about adding something like:
```diff
diff --git a/src/test/arith_uint256_tests.cpp b/src/test/arith_uint256_tests.cpp
index f596951dac..d437b6a54b 100644
--- a/src/test/arith_uint256_tests.cpp
+++ b/src/test/arith_uint256_tests.cpp
@@ -565,6 +565,8 @@ BOOST_AUTO_TEST_CASE(conversion)
for (u
...

💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745984816)
do we have functional test coverage that it *is* allowed elsewhere? Just check that it doesn't fail rather than rip out?

💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745994360)
I wonder if we want to change this string to "insufficient fee, does not improve feerate diagram" to give a hint at what to do to user

🤔 instagibbs reviewed a pull request: "[WIP] Cluster mempool implementation"
(https://github.com/bitcoin/bitcoin/pull/28676#pullrequestreview-2283792936)
poked at the function tests a bit

💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745982880)
nothing fails when I remove this

💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1746041322)
Here's a diff to clean up some of the test language and double-checking the right number of clusters
```
diff --git a/test/functional/mempool_package_rbf.py b/test/functional/mempool_package_rbf.py
index f75b13a571..201f303525 100755
--- a/test/functional/mempool_package_rbf.py
+++ b/test/functional/mempool_package_rbf.py
@@ -176,95 +176,103 @@ class PackageRBFTest(BitcoinTestFramework):
assert_equal(f"package RBF failed: insufficient anti-DoS fees, rejecting replacement {failure
...

💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745997228)
I think `test_rbf_carveout_disallowed` is meaningless post-cluster mempool, we should be relying on other tests to ensure we never go above cluster count limit

💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1746092665)
this test is pretty ancient and tbh not sure worth holding around. But in case we want to hold onto it, modifications to make things more sensible in cluster mempool world

```
diff --git a/test/functional/mempool_packages.py b/test/functional/mempool_packages.py
index f7fe381f43..9cf1aa01f5 100755
--- a/test/functional/mempool_packages.py
+++ b/test/functional/mempool_packages.py
@@ -1,92 +1,91 @@
#!/usr/bin/env python3
# Copyright (c) 2014-2022 The Bitcoin Core developers
# Distri
...

💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1746201647)
heh, if you try 99 cluster conflicts + sibling eviction, that implies the new transaction has a vsize cap of 1kvB since it's a child, and since it will be north of 5kvB it will be rejected. I don't think we can have sibling eviction count meaningfully towards exceeding max cluster conflicts. Worth the coverage anyways?

```
diff --git a/test/functional/mempool_truc.py b/test/functional/mempool_truc.py
index e3fcc45059..4e8e5943fc 100755
--- a/test/functional/mempool_truc.py
+++ b/test/func
...

💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1746202560)
the rest of these args can be deleted and the test passes

💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1746098370)
can obviously ignore this but instead of deleting keys in two spots, could just do:
`assert_equal({**last_entry, "clusterid": None}, {**new_entry, "clusterid": None})`

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2332719825)
@glozow copied some of the text to the OP

💬 achow101 commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#issuecomment-2332722279)
ACK bb08c227de158dbd88a80d904edb209e1734ab91

💬 achow101 commented on pull request "[28.x] rc backports":
(https://github.com/bitcoin/bitcoin/pull/30762#issuecomment-2332729594)
ACK b2a137929a20baed161988e24de592b1f59c0096

🚀 achow101 merged a pull request: "[28.x] rc backports"
(https://github.com/bitcoin/bitcoin/pull/30762)

📝 achow101 opened a pull request: "[28.x] Further backports and rc2"
(https://github.com/bitcoin/bitcoin/pull/30827)
* #30821

📝 achow101 converted_to_draft a pull request: "[28.x] Further backports and rc2"
(https://github.com/bitcoin/bitcoin/pull/30827)
* #30821

💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2332788365)
@mzumsande

> I don't get this at all.
> The suggestion above was, if you receive an unrequested TX, and you already have it, ignore it but don't disconnect the
> peer. Which is the same as current behavior on master.

Let’s say you have Peer1 and Peer2 announcing the Transaction ABC, if ABC is concurrently fetch from both Peer1 and Peer2, the `GETDATA` (32-bytes) for ABC would have been wasted as outgoing bandwidth by Local Node. This 32 byte leak can be amplified if batch of transactio
...