🚀 fanquake merged a pull request: "ci: Switch IWYU to `clang_17` branch"
(https://github.com/bitcoin/bitcoin/pull/28826)

👍 ismaelsadeeq approved a pull request: "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access"
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1722502299)
ACK 2be5e799ba623f969f5ffc59667a5bc6799df290 🥘

📝 maflcko opened a pull request: "test: Avoid intermittent failures in feature_init"
(https://github.com/bitcoin/bitcoin/pull/28831)
The code not only modifies block dat files, but also leveldb files, which may be of smaller size. Such corruption may not force leveldb to abort, according to the intermittent test failures.

Fix the intermittent test failures by reverting https://github.com/bitcoin/bitcoin/commit/5ab6419f380cc0a8cde78b125f3eeee5fcba43ae .

🤔 ryanofsky reviewed a pull request: "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly"
(https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-1722504219)
Thanks for the reviews.

Updated d7477b1c9cde1e795e006d1788c79b8985970eee -> 52ecb6547dd2b59643483e43531d1201941c0df0 ([`pr/noshut.16`](https://github.com/ryanofsky/bitcoin/commits/pr/noshut.16) -> [`pr/noshut.17`](https://github.com/ryanofsky/bitcoin/commits/pr/noshut.17), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/noshut.16..pr/noshut.17)) with suggested changes

💬 ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1387982956)
re: https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1377678893

> note for other reviewers (and future self): `NodeImpl::startShutDown()` does have [this additional code block](https://github.com/bitcoin/bitcoin/blob/d51fb9caa622add96c6b594e162da5584eb927fc/src/node/interfaces.cpp#L124-L128)

There is a change in behavior here but the new behavior should be more correct. RPC calls should be interrupted whenever the GUI is shut down, regardless of how it is shut down. Calls to `Star
...

💬 ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1387983151)
re: https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1364373784

> NIt: s/namespaces/namespace/

Thanks, fixed

💬 ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1387983370)
re: https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1379378726

> I don't think we need to expose those internal error details. Also, I know we exclude `server.cpp` from the CHECK_NONFATAL linter, but given that this is an RPC method I think it would be preferable here?

Thanks, replaced JSONRPCError exception with nonfatal exception

💬 ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1387983586)
re: https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1377746899

> nit: missing `#include <util/check.h>`

Thanks, fixed

💬 ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1387982522)
re: https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1377777492

> nit: long line

Thanks, added line break

✅ fanquake closed an issue: "TSAN: lock-order-inversion (potential deadlock) in ZapSelectTx test"
(https://github.com/bitcoin/bitcoin/issues/27582)

💬 fanquake commented on issue "TSAN: lock-order-inversion (potential deadlock) in ZapSelectTx test":
(https://github.com/bitcoin/bitcoin/issues/27582#issuecomment-1803871925)
Closing for now. Don't seem to be able to reproduce.

🤔 glozow reviewed a pull request: "mempool: Persist with XOR"
(https://github.com/bitcoin/bitcoin/pull/28207#pullrequestreview-1722565783)
ACK fa520848da
Code LGTM, tried a few roundtrips of stop/start/savemempool/importmempool with combinations of the options. I originally thought `-persistmempool=0` should imply something for `-persistmempoolv1` but makes sense that it applies to `savemempool` when we're not persisting.

💬 glozow commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#discussion_r1388024702)
nit: would be nice to name 1 as `MEMPOOL_DUMP_VERSION_V1` or `MEMPOOL_DUMP_VERSION_NO_XOR`

💬 glozow commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#discussion_r1388019621)
Should the mempool_compatibility.py test be updated? (the comment "200100 # Last release with previous mempool format" isn't accurate anymore)

💬 instagibbs commented on pull request "fuzz: Minor improvements to tx_package_eval target":
(https://github.com/bitcoin/bitcoin/pull/28825#discussion_r1388055793)
done

💬 furszy commented on issue "TSAN: lock-order-inversion (potential deadlock) in ZapSelectTx test":
(https://github.com/bitcoin/bitcoin/issues/27582#issuecomment-1803902266)
Saw this too late, but just in case this appears again; I have rewritten `ZapSelectTx()` entirely inside #28574.

📝 darosior opened a pull request: "fuzz: rule-out too deep derivation paths in descriptor parsing targets"
(https://github.com/bitcoin/bitcoin/pull/28832)
This fixes the `mocked_descriptor_parse` timeout reported in #28812 and direct the targets more toward what they are intended to fuzz: the descriptor syntax.

🤔 stickies-v reviewed a pull request: "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access"
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1624659971)
Code Review ACK 2be5e799ba623f969f5ffc59667a5bc6799df290

Left a number of suggestions: some about performance improvements, some about avoiding UB, and some general style/concise nits, but they can all be done in a follow-up too.

💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1380338654)
`infoAll()` holds its own `CTxMempool::cs` lock, and I think we don't need that to add stuff to `notifications`? I'm also not quite sure if we need to be holding `::cs_main` here?

💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387913882)
nit: I think a brief rationale behind why we need this change would be good to add to the commit message of "[refactor] rewrite vTxHashes as a vector of CTransactionRef"

💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387907915)
nit: one-liners seem appropriate? (+ [here](https://github.com/bitcoin/bitcoin/pull/28391/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R1487), [here](https://github.com/bitcoin/bitcoin/pull/28391/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R1498))
```suggestion
const auto entry{Assert(pool.GetEntry(tx.GetHash()))};
```