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

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1388276157)
nit: there's already a `MAGIC_BYTES` dictionary in p2p.py, could use that? (though it seems there's some circular inclusion going on, not sure how to properly resolve that....)

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1388307652)
nit: as `received_garbage` is ensured to have a maximum size of 16 bytes a few lines above, the slicing isn't needed here.
```suggestion
if received_garbage == self.peer['recv_garbage_terminator']:
# Receive, decode, and ignore version packet.
# This includes skipping decoys and authenticating the received garbage.
aad = received_garbage
```

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1388268045)
nit: could avoid `kwargs` here (unless there is a good reason to also allow other keyword arguments)
```suggestion
def __init__(self, *, initiating):
self.initiating = initiating # True if initiator
```