💬 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_r1496193271)
fixed

💬 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_r1496193353)
removed

💬 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#issuecomment-1954669587)
> I commented https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1493358857 that I think we should drop the Assume(size != 0 || fee == 0) calls in feefrac.h, so that we don't have to police all the places we invoke operator- to make sure we can never accidentally violate that.
Another nit: in the commit message for commit https://github.com/bitcoin/bitcoin/commit/860823fb93212fa78ab928589bd403da462be222, it should say "FeeFrac" and not "FeeFrace"

Addressed these by removing Assumes an
...

🤔 instagibbs reviewed a pull request: "policy: enable sibling eviction for v3 transactions"
(https://github.com/bitcoin/bitcoin/pull/29306#pullrequestreview-1891040322)
looking good, will do some testing

💬 instagibbs commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1496216091)
was this always erroneous?

💬 instagibbs commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1496208017)
let's get a bit of test coverage of `->second`

💬 instagibbs commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1496221417)
log/description of this test should get updated or maybe merged into test_v3_sibling_eviction

💬 instagibbs commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1496235168)
```Suggestion
# However, an RBF (with conflicting inputs) is possible even if the resulting cluster exceeds size 2
```

🤔 mzumsande reviewed a pull request: "net: call `Select` with reachable networks in `ThreadOpenConnections`"
(https://github.com/bitcoin/bitcoin/pull/29436#pullrequestreview-1891096814)
I tried to test the performance impact on the "normal" case of an addrman that only has addrs for reachable nets:
If I change the `AddrManSelect` benchmark to use `addrman.Select(true, g_reachable_nets.All()); ` the performance goes down by a factor of ~5 for me (from `~160ns/op` to `~770 ns/op`). Do you see a similar effect?

💬 paplorinc commented on pull request "optimization: Speed up TryParseHex by 300%":
(https://github.com/bitcoin/bitcoin/pull/29458#discussion_r1496260138)
we don't expect most hex strings to contain spaces so let's fall back to this when we can't parse the string instead

💬 paplorinc commented on pull request "optimization: Speed up TryParseHex by 300%":
(https://github.com/bitcoin/bitcoin/pull/29458#discussion_r1496260453)
c1 validation can be done sooner

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