💬 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 :)

📝 sr-gi opened a pull request: "net: Attempts to connect to all resolved addresses on `addnode`"
(https://github.com/bitcoin/bitcoin/pull/28834)
This is a follow-up of #28155 motivated by https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1362677038

## Rationale

Prior to this, addnode will only try to connect to a single randomized address after IP resolution, resulting in potentially failing the connection if the picked address is unreachable for whatever reason (e.g. being IPV6 while we do not support it).

This patches it by going over all resolved IPs until a valid one is found, or until we exhaust them.

<!--
*** P
...

🤔 theStack reviewed a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1723019604)
Concept ACK

Did only a rough first review round so far, left some comments on the way, mostly nits.

It seems like the newly introduced functional test `p2p_v2_encrypted.py` could be merged with the already existing `p2p_v2_transport.py` (here or in a follow-up), or is it fundamentally different?