💬 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
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611456690)
Seems we could factor out `AddressFamilyFromNetwork` at least, this is repeated in literally every implementation 😄
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611456690)
Seems we could factor out `AddressFamilyFromNetwork` at least, this is repeated in literally every implementation 😄
💬 maflcko commented on pull request "fuzz: More accurate coverage reports":
(https://github.com/bitcoin/bitcoin/pull/30156#discussion_r1611462530)
Why not call this at the end of `initialize` inside the function body?
(https://github.com/bitcoin/bitcoin/pull/30156#discussion_r1611462530)
Why not call this at the end of `initialize` inside the function body?
💬 maflcko commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2126828731)
> @ryanofsky If you theory is correct, then having a way to invoke `shrink_to_fit` on `UniValue`'s internal vector might be useful.
Not sure if this is ideal. UniValue is a recursive structure, so calling it on the top level vector only shouldn't cause any difference? Similarly, if shirking is done recursively, the runtime overhead will be equal to that of a copy, so might as well just do a copy instead? Finally, whenever a `UniValue` would be ready to shrink, it is usually one step away from
...
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2126828731)
> @ryanofsky If you theory is correct, then having a way to invoke `shrink_to_fit` on `UniValue`'s internal vector might be useful.
Not sure if this is ideal. UniValue is a recursive structure, so calling it on the top level vector only shouldn't cause any difference? Similarly, if shirking is done recursively, the runtime overhead will be equal to that of a copy, so might as well just do a copy instead? Finally, whenever a `UniValue` would be ready to shrink, it is usually one step away from
...
🤔 glozow reviewed a pull request: "net: add ASMap info in `getrawaddrman` RPC"
(https://github.com/bitcoin/bitcoin/pull/30062#pullrequestreview-2073621757)
ACK 1e54d61c4698debf3329d1960e06078ccbf8063c
(https://github.com/bitcoin/bitcoin/pull/30062#pullrequestreview-2073621757)
ACK 1e54d61c4698debf3329d1960e06078ccbf8063c
💬 glozow commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1611476223)
nit: this could have been a set?
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1611476223)
nit: this could have been a set?
🚀 glozow merged a pull request: "net: add ASMap info in `getrawaddrman` RPC"
(https://github.com/bitcoin/bitcoin/pull/30062)
(https://github.com/bitcoin/bitcoin/pull/30062)
💬 BenWestgate commented on pull request "doc: Update mentions of generating bitcoin.conf":
(https://github.com/bitcoin/bitcoin/pull/30154#issuecomment-2126846372)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30154#issuecomment-2126846372)
Concept ACK
💬 dergoegge commented on pull request "fuzz: More accurate coverage reports":
(https://github.com/bitcoin/bitcoin/pull/30156#discussion_r1611489609)
No reason, done!
(https://github.com/bitcoin/bitcoin/pull/30156#discussion_r1611489609)
No reason, done!
💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1611498845)
Oh, I didn't know you could do both. Happy to change if people want, but generally prefer to follow convention of the existing annotations.
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1611498845)
Oh, I didn't know you could do both. Happy to change if people want, but generally prefer to follow convention of the existing annotations.
💬 stickies-v commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611500389)
I gave the `kernel::Warning` and `node::Warning` enums approach a go because it would be nice to have the possible warnings enumerated, and it ended up being less overhead than I expected. I'm leaning towards updating this PR with that approach. What do you think?
https://github.com/stickies-v/bitcoin/commit/e4ea7aba9f1426dc510dbdbff19788919bf9d3fb - PoC but compiles and tests pass, needs cleanup and doc updates etc
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611500389)
I gave the `kernel::Warning` and `node::Warning` enums approach a go because it would be nice to have the possible warnings enumerated, and it ended up being less overhead than I expected. I'm leaning towards updating this PR with that approach. What do you think?
https://github.com/stickies-v/bitcoin/commit/e4ea7aba9f1426dc510dbdbff19788919bf9d3fb - PoC but compiles and tests pass, needs cleanup and doc updates etc