💬 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()))};
```

💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387904513)
nit: could simplify

```suggestion
const auto entry{CHECK_NONFATAL(pool.GetEntry(hash))};
```

💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387953875)
It's probably not too important, but this is a bit of a performance and memory regression to construct the vector of `TxMempoolInfo` when really we only need the underlying txref.

I think it could make sense to add `std::vector<const CTxMemPoolEntry*> entryAll() const EXCLUSIVE_LOCKS_REQUIRED(cs);` to reduce memory and computational overhead and as a bonus make the code a bit easier to read when we just want to iterate over the mempool entries?

<details>
<summary>git diff on 2be5e799ba</s
...

💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387920369)
nit: Given that we're doing not any meaninful operations with `i` I think using `auto` makes sense, and the ternary operator also seems appropriate here, simplifying this to:

```suggestion
const auto i{mapTx.find(txid)};
return i == mapTx.end() ? nullptr : &(*i);
```