🤔 sipa reviewed a pull request: "test: Add submitblock test in interface_ipc"
(https://github.com/bitcoin/bitcoin/pull/33380#pullrequestreview-3218428079)
utACK 557644ee9499583b6d00efda289fb65e8359e084

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3286511327)
Rebased 1857296c067be3dce5d33fbd1c2ae0cb8a01bef8 -> e450549ae94736fc727d0d8a4a7a6a80e768ecd2 ([kernelApi_63](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_63) -> [kernelApi_64](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_64), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_63..kernelApi_64))

* Fixed conflict with 33321

🤔 instagibbs reviewed a pull request: "Cluster mempool implementation"
(https://github.com/bitcoin/bitcoin/pull/28676#pullrequestreview-3217898607)
first pass review through 7683eeed07050fbcfa4f3b77d32303c177e2683e

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

nit: could we add some napkin math how we're getting to 9 here

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