💬 fjahr commented on pull request "Bump python minimum version to 3.8":
(https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1526080724)
ACK fac395e5eb2cd3210ba6345f777a586a9bec84e3

This is ok to merge but when I wrote that comment I meant to remove the `modinv()` function completely, e.g. https://github.com/fjahr/bitcoin/commit/75b8ba524d3dc957910bc8c0a4d1dd2b8ceaa426. You can pull this commit in here or we merge this now and I will open it as a follow-up. I think the comment in `modinv()` isn't accurate anymore after this change because python seems to use Exponentiation by Squaring and we don't really need a one-line functi
...

👋 satsie's pull request is ready for review: "rpc: add 'getnetmsgstats', new rpc to view network message statistics"
(https://github.com/bitcoin/bitcoin/pull/27534)

💬 satsie commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1179500435)
Would it be better to pass a reference of NetStats to every CNode upon initialization?

Example: https://github.com/bitcoin/bitcoin/commit/cd0c8eeb0940790b6ba83786d1c9e362d4dc4829

💬 satsie commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1179509345)
This was moved over from net.cpp. Should it be in the NetMsgType namespace? If so, what about the allNetMessageTypes array? There are places in this PR where I have to add +1 to account for the missing 'other' message type, and I'm not sure if it's appropriate for this to be a special case.

💬 satsie commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1179508199)
Now that an effort has been made to remove the friendship between CConnman and CNode (see this PR: https://github.com/bitcoin/bitcoin/pull/27257) I'm unsure if this (recording network stats on a CConnman object) belongs here.

🤔 jonatack reviewed a pull request: "rpc: add 'getnetmsgstats', new rpc to view network message statistics"
(https://github.com/bitcoin/bitcoin/pull/27534#pullrequestreview-1404629427)
A few initial thoughts, mainly about code organization.

💬 jonatack commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1179538880)
Not sure if these to/from util helper methods are needed or if this is the right place for them rather than in their respective domains where available, like `node/connection_type` and `node/network` (the latter proposed in #27385).

Unrelated, can also make all new methods that return a value `[[nodiscard]]`.

💬 jonatack commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1179532400)
Would it be a good idea for this class to be in its own file/compilation unit, rather than added into `net.h` (which is included in 40+ other files)?

💬 jonatack commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1179540855)
Perhaps consider putting all this new code in its own `rpc/netstats` files.

💬 jonatack commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1179543384)
nit unless I'm missing something, https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f16-for-in-parameters-pass-cheaply-copied-types-by-value-and-others-by-reference-to-const

💬 jonatack commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#discussion_r1179553879)
Perhaps better to calculate this programmatically, so it doesn't need to be updated.

💬 ilbarillo2014 commented on issue "Bitcoin Core v22.0.0 crashes while syncronizing the first local wallet transaction":
(https://github.com/bitcoin/bitcoin/issues/27533#issuecomment-1526233524)
Problem solved installing 24.0.1.
THANKS !

✅ ilbarillo2014 closed an issue: "Bitcoin Core v22.0.0 crashes while syncronizing the first local wallet transaction"
(https://github.com/bitcoin/bitcoin/issues/27533)

💬 HenrikJannsen commented on pull request "Remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1526869942)
@schildbach - this would be problematic for BitcoinJ, no?

💬 George43566 commented on pull request "Remove now-unnecessary poll, fcntl includes from net(base).cpp":
(https://github.com/bitcoin/bitcoin/pull/27530#issuecomment-1527049237)
I can help you out

On Thu, 27 Apr 2023, 4:11 pm Jon Atack, ***@***.***> wrote:

> ***@***.**** commented on this pull request.
>
> utACK b095ac5
> <https://github.com/bitcoin/bitcoin/commit/b095ac573b89d6898168bb9fda2abf7622ddec09>
>
> Noticed these while doing #27385
> <https://github.com/bitcoin/bitcoin/pull/27385>. Per
> https://cirrus-ci.com/task/6749578737745920:
>
> net.cpp should remove these lines:
> - #include <clientversion.h> // lines 15-15
> - #include <crypto/sha256.h> // lines 1
...

💬 George43566 commented on pull request "Remove now-unnecessary poll, fcntl includes from net(base).cpp":
(https://github.com/bitcoin/bitcoin/pull/27530#issuecomment-1527049841)
I can handle it

On Thu, 27 Apr 2023, 4:00 pm Christopher Coverdale, <
***@***.***> wrote:

> tACK
> I also ran netbase.cpp through the iwyu CI and seems like there maybe a
> few more unused imports, but I haven't thoroughly tested removing the other
> ones
>
> netbase.cpp should remove these lines:
> - #include <fcntl.h> // lines 25-25
> - #include <poll.h> // lines 29-29
> - #include <chrono> // lines 18-18
> - #include <cstdint> // lines 19-19
> - #include <limits> // lines 21-21
>
> —
>
...

💬 Empact commented on pull request "Remove now-unnecessary poll, fcntl includes from net(base).cpp":
(https://github.com/bitcoin/bitcoin/pull/27530#issuecomment-1527141145)
I'll update this PR on Sunday, just busy with http://btcpp.dev at the moment.

💬 MarcoFalke commented on pull request "Bump python minimum version to 3.8":
(https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1527228543)
Thanks. I think it is best for you to open a follow-up, unless people want me to push the commit here.

💬 fanquake commented on pull request "Bump python minimum version to 3.8":
(https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1527240485)
> I think it is best for you to open a follow-up,

Lets do that.

🚀 fanquake merged a pull request: "Bump python minimum version to 3.8"
(https://github.com/bitcoin/bitcoin/pull/27483)