📝 ismaelsadeeq converted_to_draft a pull request: "Fee Estimation: Ignore all transactions that are CPFP'd"
(https://github.com/bitcoin/bitcoin/pull/30079)
Another attempt at #25380 with an alternate approach

This PR updates `CBlockPolicyEstimator` to ignore all transactions that are CPFP'd by some child when a new block is received.
This fixes the assumption that all transactions are confirmed solely because of their fee rate.
As some transactions are confirmed due to a CPFP by some child.



This approach linearize all transactions removed from the mempool because of the new block, and only ignore transactions whose mining score is not
...

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2157939780)
Marking as draft, as I attempt to fix the fuzzing overflow error.

💬 hebasto commented on pull request "build: add `-Wundef`":
(https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2157962927)
> We'll probably need to fixup the failing ARM and previous releases job upstream:
>
> ```shell
> In file included from minisketch/src/fields/generic_common_impl.h:12,
> from minisketch/src/fields/generic_4bytes.cpp:12:
> minisketch/src/fields/../int_utils.h:162:7: error: "HAVE_CLZ" is not defined, evaluates to 0 [-Werror=undef]
> 162 | #elif HAVE_CLZ
> | ^~~~~~~~
> ```

The upstream issue has been addressed in https://github.com/sipa/minisketch/pull/88.
...

💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#issuecomment-2157981878)
Rebased to fix conflict after https://github.com/bitcoin/bitcoin/pull/28366 was merged.
[b24b7a9a6..845d16c1b](https://github.com/bitcoin/bitcoin/compare/b24b7a9a6a1a10024c639e9d4ee27aa4d0ce653e..845d16c1bbffb574b22c79ce08e2b6eba31865bc)

💬 maflcko commented on pull request "ci: move ASan job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1633026790)
> I thought it was nicer for someone to be able to run the USDT tests outside of CI by changing an environment variable

This is not enough. They will have to set up the exact system (kernel) outside the CI, as is used inside the CI. Either in a VM or on metal.

Also the new build arg may invalidate the layer cache of unrelated images.

Seems fine, if you feel strongly. Happy to ACK either approach.

👍 stickies-v approved a pull request: "log: use error level for critical log messages"
(https://github.com/bitcoin/bitcoin/pull/30255#pullrequestreview-2107384597)
ACK b90f1c36ff0e469bc64830ff35e591a9aab63fb2 - more consistent logging

💬 stickies-v commented on pull request "log: use error level for critical log messages":
(https://github.com/bitcoin/bitcoin/pull/30255#discussion_r1633005257)
I don't think this should be `LogError` as this can be recovered from, e.g. [here](https://github.com/bitcoin/bitcoin/blob/bd642ee15bda313bcf801cf63e4428c7a7d252c8/src/validation.cpp#L6332). Ideally, we just return error messages here and let the callsite log - but barring that I'd just leave as is since all the call sites that do lead to node termination will already log on the error level?

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