💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-2126637345)
`9a158da46c...ecba2fbd20`: rebase due to conflicts, thanks, @jonatack!
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-2126637345)
`9a158da46c...ecba2fbd20`: rebase due to conflicts, thanks, @jonatack!
👋 vasild's pull request is ready for review: "test: add end-to-end tests for CConnman and PeerManager"
(https://github.com/bitcoin/bitcoin/pull/26812)
(https://github.com/bitcoin/bitcoin/pull/26812)
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611345455)
Maybe also check that the result is not 0.0.0.0 (::/0 for IPv6 below). It's not entirely clear to me from the documentation if that can realistically happen, but doesn't hurt to check either.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611345455)
Maybe also check that the result is not 0.0.0.0 (::/0 for IPv6 below). It's not entirely clear to me from the documentation if that can realistically happen, but doesn't hurt to check either.
💬 vasild commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1611353096)
Makes sense (also for session destroy).
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1611353096)
Makes sense (also for session destroy).
👍 fanquake approved a pull request: "contrib: Renew Windows code signing certificate"
(https://github.com/bitcoin/bitcoin/pull/30149#pullrequestreview-2073430081)
ACK 9f4ff1e9659597307f62510f1885ad8da3a1d9a3
(https://github.com/bitcoin/bitcoin/pull/30149#pullrequestreview-2073430081)
ACK 9f4ff1e9659597307f62510f1885ad8da3a1d9a3
💬 vasild commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1611363214)
Note that for transient I2P sessions (when `-i2pacceptincoming=0`) this would be printed for every I2P peer connect and disconnect even if I2P logging is switched off.
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1611363214)
Note that for transient I2P sessions (when `-i2pacceptincoming=0`) this would be printed for every I2P peer connect and disconnect even if I2P logging is switched off.
💬 vasild commented on pull request "rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'":
(https://github.com/bitcoin/bitcoin/pull/29418#issuecomment-2126693545)
`6bfda84b92...b5189f543c`: rebase due to conflicts (part of this PR was merged via https://github.com/bitcoin/bitcoin/pull/29421)
(https://github.com/bitcoin/bitcoin/pull/29418#issuecomment-2126693545)
`6bfda84b92...b5189f543c`: rebase due to conflicts (part of this PR was merged via https://github.com/bitcoin/bitcoin/pull/29421)
👋 vasild's pull request is ready for review: "rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'"
(https://github.com/bitcoin/bitcoin/pull/29418)
(https://github.com/bitcoin/bitcoin/pull/29418)
💬 maflcko commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#issuecomment-2126708166)
> (reposting using the "review changes" button to appease the bot, it sent me several "request review" notifications in the last ten minutes :)
Not sure why that would happen. The bot should not react on new review comments after someone else's ACK. (c.f. https://github.com/maflcko/DrahtBot/blob/main/webhook_features/src/features/summary_comment.rs#L308) Looking at other pull requests, this seems to be working correctly, so my guess is that this is another GitHub metatdata corruption bug, whi
...
(https://github.com/bitcoin/bitcoin/pull/29833#issuecomment-2126708166)
> (reposting using the "review changes" button to appease the bot, it sent me several "request review" notifications in the last ten minutes :)
Not sure why that would happen. The bot should not react on new review comments after someone else's ACK. (c.f. https://github.com/maflcko/DrahtBot/blob/main/webhook_features/src/features/summary_comment.rs#L308) Looking at other pull requests, this seems to be working correctly, so my guess is that this is another GitHub metatdata corruption bug, whi
...
💬 hebasto commented on pull request "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP":
(https://github.com/bitcoin/bitcoin/pull/29790#issuecomment-2126713499)
> I've noticed two issues in the "tidy" job so far:
>
> 1. `Cannot open mapping file '/ci_container_base/ci/scratch/build-/contrib/devtools/iwyu/bitcoin.core.imp': No such file or directory.`
The recent push includes commits from https://github.com/hebasto/bitcoin/pull/205, which fixes the point 1.
> 2. Missed disabling `EXPORT_COMPILE_COMMANDS` property for targets in the `crc32c` subtree.
The point 2 has been fixed already.
(https://github.com/bitcoin/bitcoin/pull/29790#issuecomment-2126713499)
> I've noticed two issues in the "tidy" job so far:
>
> 1. `Cannot open mapping file '/ci_container_base/ci/scratch/build-/contrib/devtools/iwyu/bitcoin.core.imp': No such file or directory.`
The recent push includes commits from https://github.com/hebasto/bitcoin/pull/205, which fixes the point 1.
> 2. Missed disabling `EXPORT_COMPILE_COMMANDS` property for targets in the `crc32c` subtree.
The point 2 has been fixed already.
💬 maflcko commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2126726113)
> It's unclear to me whether the `UniValue` type has a move constructor, actually. If not, then these `std::move`s have no effect.
Since this question keeps coming up (https://github.com/bitcoin/bitcoin/issues/24542#issuecomment-2107357612), what about adding `static_assert(std::is_move_constructible_v<UniValue>);` somewhere?
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2126726113)
> It's unclear to me whether the `UniValue` type has a move constructor, actually. If not, then these `std::move`s have no effect.
Since this question keeps coming up (https://github.com/bitcoin/bitcoin/issues/24542#issuecomment-2107357612), what about adding `static_assert(std::is_move_constructible_v<UniValue>);` somewhere?
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611410019)
Alternatively you could store `net.route.0.inet[4].flags.gateway` here and use `sysctlbyname` below, avoiding the need to construct `mib[]`. Not sure if that's better though, because it's nice to be able to lookup constants like `NET_RT_FLAGS` in headers.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611410019)
Alternatively you could store `net.route.0.inet[4].flags.gateway` here and use `sysctlbyname` below, avoiding the need to construct `mib[]`. Not sure if that's better though, because it's nice to be able to lookup constants like `NET_RT_FLAGS` in headers.
📝 dergoegge opened a pull request: "fuzz: More accurate coverage reports"
(https://github.com/bitcoin/bitcoin/pull/30156)
Our coverage reports include coverage of initialization code, which can be misleading when trying to evaluate the coverage a fuzz harness achieves through fuzzing alone.
This PR proposes to make fuzz coverage reports more accurate by resetting coverage counters after initialization code has been run. This makes it easier to evaluate which code was actually reached through fuzzing (e.g. to spot fuzz blockers).
(https://github.com/bitcoin/bitcoin/pull/30156)
Our coverage reports include coverage of initialization code, which can be misleading when trying to evaluate the coverage a fuzz harness achieves through fuzzing alone.
This PR proposes to make fuzz coverage reports more accurate by resetting coverage counters after initialization code has been run. This makes it easier to evaluate which code was actually reached through fuzzing (e.g. to spot fuzz blockers).
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611411388)
Maybe a general "is the default gateway address sane" check might sense? though, up to some point if the OS returns a weird address who are we to question it. Can't get too paranoid about that. The worst that could happen is sending to 0.0.0.0, which wouldn't result in any bad things beyond an error.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611411388)
Maybe a general "is the default gateway address sane" check might sense? though, up to some point if the OS returns a weird address who are we to question it. Can't get too paranoid about that. The worst that could happen is sending to 0.0.0.0, which wouldn't result in any bad things beyond an error.
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611416943)
i prefer using constants to string based APIs, if given the choice. This avoids say, typos.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611416943)
i prefer using constants to string based APIs, if given the choice. This avoids say, typos.
💬 TheCharlatan commented on pull request "fuzz: More accurate coverage reports":
(https://github.com/bitcoin/bitcoin/pull/30156#issuecomment-2126752510)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30156#issuecomment-2126752510)
Concept ACK
💬 fanquake commented on pull request "guix: use GCC 13 to builds releases":
(https://github.com/bitcoin/bitcoin/pull/29881#issuecomment-2126768337)
Looks like Windows still has false positives with GCC 13.2.0 (https://github.com/bitcoin/bitcoin/pull/29881/checks?check_run_id=25320716500):
```bash
torcontrol.cpp: In static member function ‘static void TorControlConnection::readcb(bufferevent*, void*)’:
torcontrol.cpp:94:28: error: ‘result’ may be used uninitialized [-Werror=maybe-uninitialized]
94 | self->message.code = ToIntegral<int>(s.substr(0, 3)).value_or(0);
| ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
...
(https://github.com/bitcoin/bitcoin/pull/29881#issuecomment-2126768337)
Looks like Windows still has false positives with GCC 13.2.0 (https://github.com/bitcoin/bitcoin/pull/29881/checks?check_run_id=25320716500):
```bash
torcontrol.cpp: In static member function ‘static void TorControlConnection::readcb(bufferevent*, void*)’:
torcontrol.cpp:94:28: error: ‘result’ may be used uninitialized [-Werror=maybe-uninitialized]
94 | self->message.code = ToIntegral<int>(s.substr(0, 3)).value_or(0);
| ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
...
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611430621)
OK, you're right, this could happen: https://learn.microsoft.com/en-us/windows/win32/api/netioapi/ns-netioapi-mib_ipforward_row2
> NextHop
>
> Type: [SOCKADDR_INET](https://learn.microsoft.com/en-us/windows/desktop/api/ws2ipdef/ns-ws2ipdef-sockaddr_inet)
>
> For a remote route, the IP address of the next system or gateway en route. If the route is to a local loopback address or an IP address on the local link, the next hop is unspecified (all zeros). For a local loopback route, this membe
...
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611430621)
OK, you're right, this could happen: https://learn.microsoft.com/en-us/windows/win32/api/netioapi/ns-netioapi-mib_ipforward_row2
> NextHop
>
> Type: [SOCKADDR_INET](https://learn.microsoft.com/en-us/windows/desktop/api/ws2ipdef/ns-ws2ipdef-sockaddr_inet)
>
> For a remote route, the IP address of the next system or gateway en route. If the route is to a local loopback address or an IP address on the local link, the next hop is unspecified (all zeros). For a local loopback route, this membe
...
💬 stickies-v commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611438018)
I have considered that approach, but see a couple of difficulties with it:
- We have kernel warnings ("unknown-new-rules-activated", "large-work-invalid-chain") and node warnings ("clock-out-of-sync", "pre-release-test-build", "fatal-internal-error"). I would rather not define the node warnings in the kernel namespace. We could have have `kernel::Warning` and `node::Warning` enums (and potentially more in the future), and have a `std::variant<kernel::Warning, node::Warning> GetAllWarnings()` to
...
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611438018)
I have considered that approach, but see a couple of difficulties with it:
- We have kernel warnings ("unknown-new-rules-activated", "large-work-invalid-chain") and node warnings ("clock-out-of-sync", "pre-release-test-build", "fatal-internal-error"). I would rather not define the node warnings in the kernel namespace. We could have have `kernel::Warning` and `node::Warning` enums (and potentially more in the future), and have a `std::variant<kernel::Warning, node::Warning> GetAllWarnings()` to
...
🤔 glozow reviewed a pull request: "contrib: Renew Windows code signing certificate"
(https://github.com/bitcoin/bitcoin/pull/30149#pullrequestreview-2073561075)
tested ACK 9f4ff1e96595
(https://github.com/bitcoin/bitcoin/pull/30149#pullrequestreview-2073561075)
tested ACK 9f4ff1e96595