💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2345084353)
7f44c6d9893df97bb413e01aa737028cbe52ce75

Think you can delete `CompareTxMemPoolEntryByScore`

commit message is also a bit weird since it's replaced by something related

💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2345144877)
5de7f1378a3146368975c10511dfec086d522ff4

This needs to be checking number of ancestors of the (only) child to make sure it doesn't have multiple in-mempool parents. Here it's re-checking the same logic as the line above?

This logic seems to persist to the end of the PR.

I think this could only happen in a reorg, resulting in something like a 2p1c TRUC sibling. Looks like we only test for 1p2c et al.

Using txgraph I feel like we can make these checks extremely compact anyways, maybe c
...

💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2345163699)
20d6bcb2e7f2c3016280321ef31421ca4039b017

nit: child isn't large

💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2345196854)
cbcfdf4cb0ef05773bf430ebf6beafc59c1c7bc6

grammar: you're not `avoid`ing disabling sibling eviction here

💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2345024271)
7f44c6d9893df97bb413e01aa737028cbe52ce75

I was unable to break any existing tests by messing with this. Added a few lines to ::check which would at least catch asymmetry in orderings:

```
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index f44113ed8b..64e8de251d 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -442,4 +442,6 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
CCoinsViewCache mempoolDuplicate(const_cast<CCoinsViewCach
...

📝 PiRK opened a pull request: "test: Fix TestFlushBehavior when coin has non-zero dynamic memory usage"
(https://github.com/bitcoin/bitcoin/pull/33381)
If the random coin in this test has a non-null scriptpubkey, `cache->SanityCheck();` fails an assertion because the `BOOST_CHECK_THROW` line leaves `CCoinsViewCache::cachedCoinsUsage` in a corrupted state: the erroring `AddCoin` call decrements the value to 0 even though the coin is still in the cache, then the next `SpendCoin` call decrements the value again causing a `size_t` underflow.

Move the `BOOST_CHECK_THROW` line to the very end of the test so it does not break following tests.

💬 BenWestgate commented on pull request "doc: update multisig tutorial to use multipath descriptors":
(https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2345324305)
Okay, I've updated the tutorial to use `bitcoin rpc` everywhere possible (getcoins.py won't accept `bitcoin rpc -signet` yet).

I also fixed two bugs where we forgot to specify `-signet`.
https://github.com/bitcoin/bitcoin/pull/33286/commits/3d42607fb5966d8e3b79fdb878d59483432625d9

https://github.com/bitcoin/bitcoin/pull/33286/commits/3d42607fb5966d8e3b79fdb878d59483432625d9

💬 PiRK commented on pull request "test: Fix TestFlushBehavior when coin has non-zero dynamic memory usage":
(https://github.com/bitcoin/bitcoin/pull/33381#issuecomment-3286670785)
I force pushed to fix a use-after-move bug

✅ Raimo33 closed a pull request: "[WIP] cache: remove redundant find() call"
(https://github.com/bitcoin/bitcoin/pull/33376)

💬 Raimo33 commented on pull request "[WIP] cache: remove redundant find() call":
(https://github.com/bitcoin/bitcoin/pull/33376#issuecomment-3286747889)
Following up in https://github.com/l0rinc/bitcoin/pull/34/commits/b23c6a1fc8f9a44561add79427d2f3fff4c3718f as reviewer/coauthor

💬 hodlinator commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3286779725)
(I'd be in slight favor of changing raw arrays to `std::array` as done in https://github.com/davidgumberg/bitcoin/commit/eda422cfa56245a9b78b9a4f499a490d44e58eb5 since some debug builds in CI run with extra bounds-checking for std-types. Although maybe MSAN builds already catch out of bounds access for raw arrays, and I don't suspect we would detect any new issues for `base_uint`/`arith_uint256`).

💬 Raimo33 commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3286813683)
same. i think https://github.com/davidgumberg/bitcoin/commit/eda422cfa56245a9b78b9a4f499a490d44e58eb5 is worth a PR even if it's a minor change. Maybe you can pack it with other commits where `std::array` is needed.

💬 stringintech commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2345431507)
I think docs should be updated for these two about "not being thread safe" now that we have `LOCK(cs_main)` calls in the implementations?

💬 stringintech commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2345384878)
Is there a reason `cs_main` isn't acquired for the first two functions?

💬 stringintech commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2345483298)
Also noticed in this part that the two functions alter the same global value (`m_categories`) which happens to be atomic. We have acquired `cs_main` for one of them (but not the other?). Since the global value is already updated atomically, I assume locking `cs_main` serves a different purpose? In general, the `cs_main` locks for logging functions are a bit unclear to me. I would appreciate it if you could elaborate a bit on what you had in mind when adding them.

💬 mzumsande commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-3286964369)
Maybe covered by the previous comments, but running `test_bitcoin` on master now prints out a warning, caused by `node_init_tests`:

```
Running 671 test cases...
Warning: Disk space for "/tmp/test_common bitcoin/node_init_tests/init_test/bf78678cb7723a3e84b5/blocks" may not accommodate the block files. Approximately 810 GB of data will be stored in this directory.
```

🤔 hodlinator reviewed a pull request: "coins: warn on oversized `-dbcache`"
(https://github.com/bitcoin/bitcoin/pull/33333#pullrequestreview-3218940692)
Concept ACK

Confirmed to work on Windows:
```
$ ./build/bin/Release/bitcoind.exe -regtest -dbcache=64534
2025-09-12T22:14:09Z Bitcoin Core version v29.99.0-080a21cb127f-dirty (release build)
...
2025-09-12T22:14:09Z [warning] A 64524 MiB dbcache may be too large for a system memory of only 65413 MiB.
```

📝 caesrcd opened a pull request: "Bash completion for bitcoin(1) wrapper"
(https://github.com/bitcoin/bitcoin/pull/33382)
Uses `bitcoin --help` / `bitcoin help` to list available commands and options for the `bitcoin` wrapper.
Integrates completions for `bitcoind`, `bitcoin-cli`, and `bitcoin-tx`.

Placed under `contrib/` so it can be installed or used easily by distros/users who want completion.

💬 davidgumberg commented on issue "bench: unrealistic ConnectBlock benchmarks":
(https://github.com/bitcoin/bitcoin/issues/33375#issuecomment-3287201170)
I am [skeptical](https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2655685417) of trying to measure end-to-end performance of Bitcoin Core systems like block connection including LevelDB performance using a microbenchmark suite. In my opinion, these benchmarks work best at measuring the performance of specific components and functions a-la unit tests, and I would not object to adding more scenarios to `ConnectBlock()` that are meant to exercise the performance of certain segments of blo
...

💬 furszy commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2345636999)
I was likely thinking of cases where the max tx fee is set below the relay fee rate for any standard tx size but still above the dust limit (iff this is ever possible..).

I assume this already fails on the feerate checks you added, but it might make sense to disallow such a max tx fee in the first place.

It seems we could compute the feerate for the smallest possible standard tx the wallet could create and reject a max tx fee that would fall below the relay feerate.

---------------

T
...