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

💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387927635)
This seems like a slight regression: `tx3_iter.value()` would throw if the hash was not found, whereas `tx3_entry->GetSizeWithAncestors()` is UB. I think it would be good to add inline `Assert`s to the entry initialization (and add list initialization while you're at it)?

```cpp
const auto tx3_entry{Assert(pool.GetEntry(tx3->GetHash()))};
```

Similar concerns [here](https://github.com/bitcoin/bitcoin/pull/28391/files#diff-e9d32ba5cee0e31252a337dda9c2272152be7e9798601e62c2d042dccf79f7c1R9
...

💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1388021604)
nit: not a regression, but using `.value()` seems safer (also for `tx7`, `tx8`, `tx9` further down)
```suggestion
setAncestors.insert(pool.GetIter(tx6.GetHash()).value());
```

💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1388039482)
nit: I think we can just directly use `GetIter`?

```suggestion
GetIter(txns_randomized.back()->GetHash()).value()->idx_randomized = it->idx_randomized;
```

📝 theStack opened a pull request: "wallet: refactor: remove unused `SignatureData` instances in spkm's `FillPSBT` methods"
(https://github.com/bitcoin/bitcoin/pull/28833)
These are filled with signature data from a PSBT input, but not used anywhere after, hence they can be removed. Note that the same code is in the `SignPSBTInput` function where the `sigdata` result is indeed used.