💬 stickies-v commented on pull request "log: use error level for critical log messages":
(https://github.com/bitcoin/bitcoin/pull/30255#discussion_r1633007398)
nit: since we're calling `GetNotifications().fatalError()` next, I think this log line can just be removed?

💬 maflcko commented on issue "RPC wont bind without an IP address on a non-localhost interface":
(https://github.com/bitcoin/bitcoin/issues/13155#issuecomment-2157988114)
Reopening for now, because it came up again. cc @m3dwards

💬 maflcko commented on pull request "log: use error level for critical log messages":
(https://github.com/bitcoin/bitcoin/pull/30255#discussion_r1633033058)
My understanding is that the notification does not include `err.what()`, so the debug log in this line is still needed.

💬 maflcko commented on pull request "log: use error level for critical log messages":
(https://github.com/bitcoin/bitcoin/pull/30255#discussion_r1633037105)
Good point, deferred for the follow-up that handles warnings.

⚠️ maflcko reopened an issue: "RPC wont bind without an IP address on a non-localhost interface"
(https://github.com/bitcoin/bitcoin/issues/13155)
If the only interface which has an IP address and is up is lo, one of the two default rpcbinds (:: and 0.0.0.0) will fail with "libevent: getaddrinfo: address family for nodename not supported".

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1632970572)
nit 0044c1dad9ffc3d58afd06d7533f16beb0d0a829:

Would prefer to not have magic number comments

```suggestion
// Don't consider replacements that would cause us to remove a large number of mempool entries.
// This limit is not increased in a package RBF. Use the aggregate number of transactions.
```

🤔 glozow reviewed a pull request: "Cluster size 2 package rbf"
(https://github.com/bitcoin/bitcoin/pull/28984#pullrequestreview-2107324391)
looks pretty good 7b74cbf6ed7cafdbd458471ae89fa88c52256278

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633036250)
Needs rebase for #29496?

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1632980812)
nit: 7d895935aef63b4aa728310b580f418ce35fc234

we started using `low_fee_amt` instead of 200 in this test

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633026172)
Can check `m_wtxids_fee_calculations` and `m_effective_feerate`.

Wrote up the changes for this + using `CheckPackageMempoolAcceptResult`:
```diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp
index cd69fce2f5..b68686cfcb 100644
--- a/src/test/txpackage_tests.cpp
+++ b/src/test/txpackage_tests.cpp
@@ -975,28 +975,22 @@ BOOST_FIXTURE_TEST_CASE(package_rbf_tests, TestChain100Setup)

LOCK(m_node.mempool->cs);
const auto submit1 = ProcessNewPackage(
...

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633053605)
No mentions of BIP125 outside of signaling pls
```suggestion
- Replacements must pay more total total fees at the incremental relay fee (analogous to regular [replacement rules](./mempool-replacements.md) 3 and 4).
```

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633039485)
aa3a4da248fe9d4a765fa4c80797ca333ee37f07: is the static cast necessary?

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1632990659)
Should use `CheckPackageMempoolAcceptResult` to check that results are populated as expected, which also means you can remove a few of these checks:

```suggestion
if (auto err_1{CheckPackageMempoolAcceptResult(package1, submit1, /*expect_valid=*/true, m_node.mempool.get())}) {
BOOST_ERROR(err_1.value());
}
auto it_parent_1 = submit1.m_tx_results.find(tx_parent->GetWitnessHash());
auto it_child_1 = submit1.m_tx_results.find(tx_child_1->GetWitnes
...

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633049147)
91a7e8ba7606b68401b29d2609c240a604ab6fe1: this isn't a perfect test for cluster size 2... why not call `CheckConflictTopology`?

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1633045871)
5dd74f9c6ddc218417bda841f493ca2ed3f2b327

I don't 100% remember why we did this, but I think it was so we don't check mempool contents when `test_accept=true`?

🚀 fanquake merged a pull request: "build: Remove --enable-gprof"
(https://github.com/bitcoin/bitcoin/pull/30257)

💬 stickies-v commented on pull request "log: use error level for critical log messages":
(https://github.com/bitcoin/bitcoin/pull/30255#discussion_r1633060350)
Can't we just merge them?

<details>
<summary>git diff on b90f1c36ff</summary>

```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 89aa84a551..ac95824443 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -6303,12 +6303,10 @@ bool ChainstateManager::ValidatedSnapshotCleanup()
fs::path p_old,
fs::path p_new,
const fs::filesystem_error& err) {
-
...

💬 fanquake commented on pull request "build: Remove --enable-gprof":
(https://github.com/bitcoin/bitcoin/pull/30257#issuecomment-2158040840)
Removed the label because nothing was ever ported.

💬 stickies-v commented on pull request "log: use error level for critical log messages":
(https://github.com/bitcoin/bitcoin/pull/30255#issuecomment-2158082194)
re-ACK 2222f2573c8de0a7f7171e6fc9e844ad448e1f65

💬 maflcko commented on pull request "build: Remove --enable-gprof":
(https://github.com/bitcoin/bitcoin/pull/30257#issuecomment-2158089097)
It needs to be ported to the table in https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e