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

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