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

💬 willcl-ark commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1804150932)
@ajtowns @MatthewLM thank for your comments.

I agree that after these changes some ASM fields will be less human-readable-friendly in LE hex.

Prefixing with `0d` was another possibility discussed previously, but there still exists a chance that someone will interpret these as hex-encoded values, as `d` is a valid hex char (whereas `x` is clearly not decimal or hex). e.g this is not necessarily _that_ much clearer, but does offer some improvement:

```sh
$ bitcoin-cli -regtest decodescri
...

💬 MatthewLM commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1804161011)
I agree that `0d` is not a good idea, much better to go with `0x` for hex.

💬 pinheadmz commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1388273116)
ok

💬 pinheadmz commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1388273335)
done, thanks

💬 pinheadmz commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1388274061)
hmmm I'm gonna keep it actually because later in this function is a boolean called `forced` so, I think this is legible

💬 pinheadmz commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1388274229)
good idea, added a new commit for this

💬 pinheadmz commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1804201429)
Thanks for the review Gleb, I also rebased on master to fix conflicts

💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1388299672)
Helpers are good, so I'll add this patch as a separate commit.

💬 theuni commented on pull request "build: remove `-bind_at_load` usage":
(https://github.com/bitcoin/bitcoin/pull/28783#issuecomment-1804217435)
utACK.

Whoops, I butchered that commit message. I forgot to paste in the commit hash I was referencing. Mind fixing it up?

💬 fanquake commented on pull request "build: remove `-bind_at_load` usage":
(https://github.com/bitcoin/bitcoin/pull/28783#issuecomment-1804224177)
> I forgot to paste in the commit hash I was referencing. Mind fixing it up?

Pretty sure it is there? "Similar to a98356fee8a44d7d1cb37f22c876fff8f244365e."

💬 theuni commented on pull request "build: remove `-bind_at_load` usage":
(https://github.com/bitcoin/bitcoin/pull/28783#issuecomment-1804234360)
> > I forgot to paste in the commit hash I was referencing. Mind fixing it up?
>
> Pretty sure it is there? "Similar to [a98356f](https://github.com/bitcoin/bitcoin/commit/a98356fee8a44d7d1cb37f22c876fff8f244365e)."

"build: Add an old hack similar to remove bind_at_load from libtool." ->
"build: Add an old hack to remove bind_at_load from libtool."

I clearly copied "similar to" from one place to another and forgot to delete the first :)