📝 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
```

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1388285003)
Since we are only in test land here, it's probably fine to also use seed-determined randomness for garbage as well (i.e. `random.randrange`)? (I've used `os.urandom` in the past for a PR and got critical feedback about that: https://github.com/bitcoin/bitcoin/pull/25625#discussion_r924540431, which I agree with)
```suggestion
self.sent_garbage = random.randrange(garbage_len)
```

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1388279451)
nit: could deduplicate code by using a helper method like `generate_garbage` (or even `generate_keypair_and_garbage`) that is used both in `initiate_v2_handshake` and `respond_v2_handshake`? Could also put the magic number in a `MAX_GARBAGE_LEN` constant (that's the naming we use in the C++ codebase), though that's one less than the number passed to randrange here.

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1388353134)
nit: could import this constant from test_framework.crypto

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1388344334)
nit:
```suggestion
if is_decoy: # since decoy messages are ignored by the recipient - no need to wait for response
```
(or "don't get processed" or something like that?)

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1388337105)
readability nit:
```suggestion
kwargs['services'] = (kwargs['services']|NODE_P2P_V2) if 'services' in kwargs else (P2P_SERVICES|NODE_P2P_V2)
```

💬 maflcko commented on pull request "fuzz: call lookup functions before calling `Ban`":
(https://github.com/bitcoin/bitcoin/pull/27935#issuecomment-1804300886)
lgtm ACK fca0a8938e34cb4f6c400e1d1d0be02f027d80c5

I did not check the performance difference against 7e644308805229ab64455c01a22531756644fe69

💬 pablomartin4btc commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-1804327607)
I've addressed feedback from @andrewtoth - thank you!

💬 maflcko commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#discussion_r1388449752)
Thx, done

💬 maflcko commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#discussion_r1388449815)
Thx, done

💬 maflcko commented on pull request "multiprocess compatibility updates":
(https://github.com/bitcoin/bitcoin/pull/28721#issuecomment-1804394759)
re-ACK 3b70f7b6156cb110c47a6e482791cf337bb6ad6d 🎆

<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 3b70f7b6156cb110c47
...

💬 fjahr commented on pull request "Embedding ASMap files as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-1804437895)
> Would it make sense to only check in the asmap.dat file into the repository, and then have the build system convert it to a .h at compile time (similar to the approach used in the `src/test/data` directory)? That would mean there is also a directly-manipulable file in the repository (that can be used with `asmap-tool` etc).

Yeah, that would work as well. I have drafted that approach here: https://github.com/fjahr/bitcoin/commit/0f9109fc49b8add9c057dcacd537250d4539ae57 Details can change of
...

💬 fjahr commented on pull request "Embedding ASMap files as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-1804443164)
> Why is the option of loading it from a file even in dev builds, not considered?

It is considered. Only loading from a file is the only option for dev builds if we go the other route mentioned in the description: "The alternative approach is not having the data in the source code but only adding it during the build phase of a release." But this also comes with the aforementioned downsides.

💬 maaku commented on pull request "snapshots: don't core dump when running -checkblockindex after `loadtxoutset`":
(https://github.com/bitcoin/bitcoin/pull/28791#issuecomment-1804450152)
No, that would be likely to cause problems in various workflows. It is entirely reasonable that -checkblockindex is set in a bitcoin.conf file, just as it is done by default in the RPC tests. In that case restarting the daemon after loading a snapshot but before validation of the historical blockchain is complete will cause it to be in an unstartable state, even though there is nothing wrong with the block index.-checkblockindex is useful for checking the integrity of the database, even before t
...

💬 maaku commented on pull request "snapshots: don't core dump when running -checkblockindex after `loadtxoutset`":
(https://github.com/bitcoin/bitcoin/pull/28791#discussion_r1388479896)
Yes, because the genesis block.

💬 kevkevinpal commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#discussion_r1388491212)
running this command
`grep -nr --text "\-present" ./src`

I only see these files using `present`
```
./src/streams.cpp:1:// Copyright (c) 2009-present The Bitcoin Core developers
./src/kernel/mempool_removal_reason.cpp:1:// Copyright (c) 2016-present The Bitcoin Core developers
./src/kernel/mempool_removal_reason.h:1:// Copyright (c) 2016-present The Bitcoin Core developers
```

if this is something we're going towards would we want to update the copy right of all files whenever there
...