💬 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
(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.
(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.
(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.
(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]]`.
(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)?
(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.
(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
(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.
(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 !
(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)
(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?
(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
...
(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
>
> —
>
...
(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.
(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.
(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.
(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)
(https://github.com/bitcoin/bitcoin/pull/27483)
💬 darosior commented on pull request "Wallet: estimate the size of signed inputs using descriptors":
(https://github.com/bitcoin/bitcoin/pull/26567#discussion_r1180304523)
Pushed a much cleaner version of this: introducing a `MultiSigningProvider` to wrap around both the wallet and external providers. Still as a commit on top, let me know what you think @achow101.
(https://github.com/bitcoin/bitcoin/pull/26567#discussion_r1180304523)
Pushed a much cleaner version of this: introducing a `MultiSigningProvider` to wrap around both the wallet and external providers. Still as a commit on top, let me know what you think @achow101.
📝 fjahr opened a pull request: "test: Remove modinv python util helper function"
(https://github.com/bitcoin/bitcoin/pull/27538)
Since #27483 was merged the `modinv()` body is just one line calling pythons own implementation of `pow()`. We can just remove the function as it doesn't seem to add any value. Additionally the comment in the function is now outdated and the test is only testing two ways of doing modular inverse but both using python's `pow()` function.
(https://github.com/bitcoin/bitcoin/pull/27538)
Since #27483 was merged the `modinv()` body is just one line calling pythons own implementation of `pow()`. We can just remove the function as it doesn't seem to add any value. Additionally the comment in the function is now outdated and the test is only testing two ways of doing modular inverse but both using python's `pow()` function.