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

💬 furszy commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1388101787)
In 5809c90fee6:

Need to remove this flag. Otherwise the migrated watch-only and solvables wallets will have the global hd key flag set while newly created private key disabled wallets will not.

This is because the wallets are created calling `CWallet::Create()` here, which takes this variable and set it calling `CWallet ::InitWalletFlags()`.

It would be nice to add a test verifying that we do not create a private key disabled wallet with the global hd key flag set.

💬 brunoerg commented on pull request "fuzz: call lookup functions before calling `Ban`":
(https://github.com/bitcoin/bitcoin/pull/27935#issuecomment-1804028996)
> Ah ok. And about the efficiency, is it possible to keep the fuzz input format unchanged instead of using a string representation (Maybe via a string-lookup roundtrip)? I wonder if that'd be more efficient.

Yes, force-pushed addressing it. Also, I think this way our seeds remain great.

💬 Riahiamirreza commented on pull request "rpc: show P2(W)SH redeemScript in getrawtransaction #27637":
(https://github.com/bitcoin/bitcoin/pull/27638#issuecomment-1804074931)
@Sjors For adding new field to a transaction (in my case, adding `redeemScript` to `vin`) is it correct to add new fields to `CTxIn` class in `test/functional/test_framework/messages.py`? While adding it will cause mismatch in weight calculation of transactions, I think it's not the correct way. But I'm not sure what's the correct way.

💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1804077020)
re-ACK d0d4308750c14c44ed2a9b60bb44d6ec5423bd5a 💇

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK d0d4308750c14c44ed2a
...

🤔 pablomartin4btc reviewed a pull request: "snapshots: don't core dump when running -checkblockindex after `loadtxoutset`"
(https://github.com/bitcoin/bitcoin/pull/28791#pullrequestreview-1722890569)
tACK cdc6ac4126b31426261605a757c52ea2dbfb2a81

<details><summary>I've managed to reproduce the issue</summary><ul>

<li><details><summary><code>getblockchaininfo</code> - <code>IBD</code> not completed</summary><ul>

```
./src/bitcoin-cli -datadir=${AU_DATADIR} getblockchaininfo
{
"chain": "main",
"blocks": 800000,
"headers": 815917,
"bestblockhash": "00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054",
"difficulty": 53911173001054.59,
"time": 1690168629,
...

💬 pablomartin4btc commented on pull request "snapshots: don't core dump when running -checkblockindex after `loadtxoutset`":
(https://github.com/bitcoin/bitcoin/pull/28791#discussion_r1388214419)
Removing the testing assert conditions, as explained by @maflcko in the comment link provided above, even with this fix, `bitcoind` still crashes.
```
2023-11-09T15:54:40Z [dnsseed] dnsseed thread exit
bitcoind: validation.cpp:4880: void ChainstateManager::CheckBlockIndex(): Assertion `(pindex->nChainTx == pindex->nTx + prev_chain_tx) || pindex->IsAssumedValid()' failed.
Aborted (core dumped)

```

💬 mzumsande commented on pull request "p2p: Increase inbound capacity for block-relay only connections":
(https://github.com/bitcoin/bitcoin/pull/28463#issuecomment-1804150092)
Rebased after #28464 was merged.