💬 stickies-v commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1463055516)
thx, fixed

💬 brunoerg commented on pull request "fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses":
(https://github.com/bitcoin/bitcoin/pull/26859#issuecomment-1905749723)
> @brunoerg?

Sounds good to me. Is the CJDNS address from `ConsumeNetAddr` always valid?

💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1905760878)
Ok, fixed that issue by first getting the file size and then resizing the `std::vector` / `std::string`.

That doesn't generalise nicely to known-size things like `CKey`. The `maxsize` argument also makes sense for that. So I might make a separate method for that. Which in turn makes this PR just a refactor with unused, but tested, support for `std::vector<unsigned char>`.

👋 Sjors's pull request is ready for review: "Make (Read/Write)BinaryFile work with char vector"
(https://github.com/bitcoin/bitcoin/pull/29229)

🤔 naumenkogs reviewed a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1838102663)
Some low-level feedback for now, mostly nits. I will do high-level review against the BIP shortly.

💬 naumenkogs commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1462877634)
c950386eaaf41ec096606de81a3a60b278b156f0
nit: since `v2transport` is named exactly the same in addconnection and as an init flag, perhaps either specify where exactly to see, or have extended description in both places ("adding v2transport connections requires v2transport init flag to be set."). Current version is kinda confusing.

💬 naumenkogs commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1462899288)
9b6425ce0fb74b40e12f92b704a41feec23cd754
nit: "at least one byte" is confusing... what if it's half-a-byte? Sounds like that would be ignored, but i assume it won't be.

💬 naumenkogs commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1462920200)
9b6425ce0fb74b40e12f92b704a41feec23cd754

nit: might be cleaner this way
```
received_prefix_len = 0
while received_prefix_len < 16:
byte = response.read(1)
# return b"" if we need to receive more bytes
if not byte:
return received_prefix_len, b""
received_prefix_len += len(byte)
if byte != v1_prefix[received_prefix_len - 1]:
return received_prefix_len, self.generate_keypair_an
...

💬 naumenkogs commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1462949568)
9a2050275573eae31c5dc14fefedb612e951d0b8

do you mind adding a comment for what happened here?

💬 naumenkogs commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1462953624)
what does this mean?

💬 naumenkogs commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1462964155)
why `on_connection_send_msg` is not sent here on reconnect (the new condition)?

💬 vasild commented on pull request "fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses":
(https://github.com/bitcoin/bitcoin/pull/26859#issuecomment-1905788444)
`790f0cb60d...b851c5385d`: rebase and adjust `fuzz/banman.cpp` due to https://github.com/bitcoin/bitcoin/pull/26859#pullrequestreview-1833881229.

> is the CJDNS address from ConsumeNetAddr always valid

Yes, I think so. Anyway, for safety wrt future changes I added `IsValid()` to the check. That is - omit lookup for valid CJDNS addresses.

💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1905800964)
I tried find some existing code that use the `std::vector<unsigned char>` variant. Didn't find it at first glance. It seems we almost always know what size to expect.

So that might be a good reason to kill that variant and only support loading an std::string of unknown size for now.

💬 maflcko commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1463138328)
```suggestion
try {
AutoFile{fsbridge::fopen(filename, "wb")} << Span{data};
```

nit: Can be written shorter

🤔 shaavan reviewed a pull request: "init: handle empty settings file gracefully"
(https://github.com/bitcoin/bitcoin/pull/29144#pullrequestreview-1838611173)
Code review ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766

After our discussions in [this](https://github.com/bitcoin/bitcoin/pull/29144#discussion_r1450404094) comment thread, I believe the changes in this PR are complete and are implemented adequately.

💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1463183042)
Took me a while to wrap my head around the various calls involved, but I guess the nullptr check is handled in `read()`, which is called by the various `ser_read...` functions in `seralize.h`, which is called by the `Unserialize` implementations, which is called by `<<`.

💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1463199231)
On the read side I'm keeping the explicit nullptr check for now, so I don't have to catch `fs::file_size` failure.

💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1463205250)
At the point `WriteBinaryFile` is so small you might as well have the called do the try - catch. But might as well keep if around as long as `ReadBinaryFile` can't be made smaller.

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1463274709)
> > CConMan cannot access the chain state, the sync progress, in-flight block requests
>
> I don't see how the connection opening logic needs access to any of those things.

I believe that we are not in sync regarding the distinction between (1) the logic for deciding whether to connect to a specific address and (2) the thread/process responsible for opening such a connection. The former requires access to that contextual information; its usage is behind the `extra_block_relay` and `try_ano
...

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1463281056)
direct_conflicts and all_conflicts. Does that help?