💬 paplorinc commented on pull request "optimization: Speed up TryParseHex by 300%":
(https://github.com/bitcoin/bitcoin/pull/29458#discussion_r1496263552)
Ideally (when no spaces present) two characters will form a single byte - and when spaces are present, it will be less, so we can avoid reallocation by reserving `str.size() / 2`

👍 theuni approved a pull request: "lint: Check for missing bitcoin-config.h includes"
(https://github.com/bitcoin/bitcoin/pull/29408#pullrequestreview-1891133183)
Weak ACK fa58ae74ea67485495dbc2d003adfbca68d6869b.

My python-fu is weak, but I think this is a necessary check and it seems to work as advertised.

💬 mzumsande commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1496276836)
Looks like the case of empty networks is no longer used outside of tests after this PR. In that case, should we drop the default args and simplify the implementation?

💬 sdaftuar commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1496342131)
I had the same question -- as far as I can tell, it's fine to leave this Assume() in?

💬 sr-gi commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1496355683)
I did some digging on the topic. Looks like the way TCP handshake timeout works depends on the OS, but is managed by the application who's trying to create/accept the connection. In our case, this is governed by `nConnectTimeout`, which ranges from 0-5000ms (being the later the default: `DEFAULT_CONNECT_TIMEOUT`).

So, the longest this could take to resolve, assuming 256 addresses that all timeout, is around ~21 min (`5*256/60`). As mentioned before, this is done by the user under normal condi
...

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1954946598)
Updated 8ab4e07a1a54c9f4818a557fd1182bbb59e1cf9b -> 8afcd99435b693f69b8c3a918b0573ef12559b22 ([`pr/nofake.8`](https://github.com/ryanofsky/bitcoin/commits/pr/nofake.8) -> [`pr/nofake.9`](https://github.com/ryanofsky/bitcoin/commits/pr/nofake.9), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/nofake.8..pr/nofake.9)) adding suppression to avoid getchaintxstats ubsan integer overflow errors exposed by new test coverage

💬 luke-jr commented on pull request "Add a `-permitbarepubkey` option":
(https://github.com/bitcoin/bitcoin/pull/29309#issuecomment-1954959472)
@shaavan Also note ChatGPT output is not necessary clear of copyright issues, so should never be included directly.

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1954993497)
@cbergqvist thanks so much for your thorough review!

----

re: Implicit version

Yes the default 1.1 parameter in cpp code is to keep default behavior from bitcoin-cli. In python it's to match existing behavior from the RPC client in the test framework, i.e.:

https://github.com/bitcoin/bitcoin/blob/207220ce8b767d8efdb5abf042ecf23d846ded73/test/functional/test_framework/authproxy.py#L120

I look forward to upgrading both cli and the tests to v2.0 as a follow-up PR

----

re: named
...

💬 cbergqvist commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1955168519)
ACK 58cb22c

All reasonable arguments! Would be a scary to see new default values for parameters in consensus critical code - but this is not, and it's very unlikely it would cause any harm.

💬 pinheadmz commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1496566849)
Kinda funny that in/out maps to in/manual ... perhaps `"manual"` should've been the permission flag?

💬 pinheadmz commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1496520271)
nit, I think these should get the `m_` prefix. When I was reviewing a7240eb99dc792fdf3a6f5aeb93c40c42a8cdc16 I thought `whitelist_relay` was a local variable and couldn't tell where it was defined. Maybe just me though 🤷

👍 pinheadmz approved a pull request: "p2p: Allow whitelisting manual connections"
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1891576785)
ACK c10f528350152ca9248e8c167b67993fc7321ca3

I left comments on a few nits but none are breaking for me. Happy to re-ACK if you update.

Built and ran all functional and unit tests locally on macOS/arm

Reviewed specific changes since my last ACK:
```
git range-diff abfc8c901d..d69747ab65 5883a8911a..c10f528350
```
- Move relay flags to netpermissions from net
- Only apply new logic to manual connections
- Apply flag to new tests added since last review
```


<details><summary>S
...

💬 pinheadmz commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1496499176)
Now that we're not modifying `nLocalServices` I don't **think** we need the local variable `nodeServices` any more.

💬 luke-jr commented on pull request "Update rpcauth.py":
(https://github.com/bitcoin/bitcoin/pull/29433#issuecomment-1955210936)
Not sure it makes sense to do this. If you're parsing JSON, surely you can do the trivial things inside rpcauth yourself?

🤔 ryanofsky reviewed a pull request: "serialization: Support for multiple parameters"
(https://github.com/bitcoin/bitcoin/pull/28929#pullrequestreview-1891677340)
Updated 3c311734d2fc6a4ca410f254ba3c8e923d58be70 -> e02ab1577f51a6c9af96b49e29ffaa3918ddae6c ([`pr/iparams.4`](https://github.com/ryanofsky/bitcoin/commits/pr/iparams.4) -> [`pr/iparams.5`](https://github.com/ryanofsky/bitcoin/commits/pr/iparams.5), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/iparams.4..pr/iparams.5)) using && and std::forward to support rvalue substream arguments and adding tests and documentation

💬 ryanofsky commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1496589417)
re: https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1484436443

> Yes, an example would be in net.cpp, but feel free to ignore, if it is too much hassle.
>
> ```c++
> src/net.cpp- DataStream underlying_stream{vSeedsIn};
> src/net.cpp: ParamsStream s{underlying_stream, CAddress::V2_NETWORK};
> ```

Since it didn't complicate implementation of this PR too much, I implemented support for passing any type of substream as an rvalue to ParamsStream, instead of only passing ne
...

🤔 fjahr reviewed a pull request: "p2p: Don't process mutated blocks"
(https://github.com/bitcoin/bitcoin/pull/29412#pullrequestreview-1891184515)
Concept ACK

"`CBlock::GetHash()` is a foot-gun without a prior mutation check", I hadn't felt this strongly about but I get it. I think it makes sense to rename it. I have drafted it here: https://github.com/fjahr/bitcoin/commit/8e11e9c9ec9d52b2c5c04a33f33da0b22ae5e28b If it sounds valuable I will open a PR.

💬 fjahr commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1496312078)
Given the recent changes around the mailing list could also name what's behind this link? So in case it disappears people know what to search for. I know they promised to keep the links but who knows...

💬 fjahr commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1496365733)
It would also be a little bit easier to reason about if when `block.vtx.empty()` is true then we just `return false;` because in that case `std::any_of` will always return false.

💬 luke-jr commented on pull request "doc: Fix gen-manpages to check build options":
(https://github.com/bitcoin/bitcoin/pull/29457#issuecomment-1955225976)
Probably should be a way to override this, in case someone actually wants a custom-fit manpage?