💬 dergoegge commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1785941780)
My approach for this would have been to reuse `SetMockTime`, which would set `g_mock_time` to the requested mocktime and `g_mock_steady_time` to `max(mock_time_in, g_mock_steady_time)` (to ensure it only ever increases).

The benefit of that approach would be that all existing tests that make use of mock time would now also mock steady time (assuming `NodeSteadyClock` is used throughout).

I never implemented and tested this but I was curious if you considered this as well?

💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1785957916)
sure, fine with me, agree it's less confusing though i wasn't sure which of the current usages should be mockable and didn't want to decide that here

💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1785962075)
i don't know.

i like being able to mock them seperately, and millisecond resolultion instead of second resolution is kind of nice here because it tends to be used for deadlines and timeouts, seconds are kind of clunky there.

also conceptually i think "`max(mock_time_in, g_mock_steady_time)`" is a wrong representation of how monotonic time works, it's basically a CPU cycle counter, it isn't linked to wall time at all.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2391022009)
@pinheadmz, I think that the functionality of "execute this code after some time", is not much related to the sockets handling and better be implemented at some higher level, not inside `SockMan`. Maybe the scheduler, like @maflcko suggested, or in the `EventIOLoopCompletedForAllPeers()` method which will be caller periodically by `SockMan`:

```cpp
/**
* SockMan has completed send+recv for all nodes.
* Can be used to execute periodic tasks for all nodes.
* The implement
...

💬 laanwj commented on pull request "cmake: Avoid hardcoding Qt's major version in Find module / variable names":
(https://github.com/bitcoin/bitcoin/pull/31010#issuecomment-2391045186)
Seems fine with me, we could update the Qt5 to Qt6 as well, but i imagine removing the version number saves us work when we move to Qt7 in the future 😄

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2391079413)
`b98320ceff...54f4f3b05e`: address suggestions and blindly try to fix a bogus warning about uninitialized optional (didn't work)

💬 brunoerg commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1786005133)
Nice, I will address it.

🤔 ismaelsadeeq reviewed a pull request: "test: Treat exclude list warning as failure in CI"
(https://github.com/bitcoin/bitcoin/pull/31018#pullrequestreview-2345259658)
utACK fa6d14eacb2a8c1c3243e3075254dfdebaa9290e

💬 brunoerg commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1786018407)
> You should be able to use -DAPPEND_CPPFLAGS="-DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION" (I see where the confusion comes from, as -D is accepted by compilers to define macros and by cmake to set build flags but they are different things).

Ah yes, thank you. Just saw some examples from the CI scripts.

💬 brunoerg commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2391109001)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1785875497 and https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1785885817. Thanks, @dergoegge.

🤔 ismaelsadeeq reviewed a pull request: "doc: update IBD requirements in doc/README.md"
(https://github.com/bitcoin/bitcoin/pull/30992#pullrequestreview-2345306765)
ACK 36a6d4b0078ebb39ed082c866bf49214a2a01241

💬 ismaelsadeeq commented on pull request "doc: update IBD requirements in doc/README.md":
(https://github.com/bitcoin/bitcoin/pull/30992#discussion_r1786039900)
nit: maybe just say hundreds
```suggestion
Bitcoin Core is the original Bitcoin client and it builds the backbone of the network. It downloads and, by default, stores the entire history of Bitcoin transactions, which requires hundreds of gigabytes of disk space. Depending on the speed of your computer and network connection, the synchronization process can take anywhere from a few hours to several days or more.
```

💬 ismaelsadeeq commented on issue "ci: macOS 14 CI failure `Invalid value detected for '-wallet' or '-nowallet'`":
(https://github.com/bitcoin/bitcoin/issues/31019#issuecomment-2391146953)
@tdb3 it might be helpful to share link to the full C.I logs?

📝 Sjors opened a pull request: "Add -pausebackgroundsync startup option"
(https://github.com/bitcoin/bitcoin/pull/31023)
When a UTXO snapshot is loaded, this option lets the user pause the verification of historical blocks in the background.

💬 Sjors commented on pull request "Add -pausebackgroundsync startup option":
(https://github.com/bitcoin/bitcoin/pull/31023#issuecomment-2391203396)
It might make sense to change this into an RPC method.

🤔 danielabrozzoni reviewed a pull request: "rpc: add getorphantxs"
(https://github.com/bitcoin/bitcoin/pull/30793#pullrequestreview-2345399717)
ACK 98c1536852d1de9a978b11046e7414e79ed40b46

Code looks good to me, I have a few nits, but feel free to ignore them.

💬 danielabrozzoni commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786093549)
nit: we could also test that verbosity > 2 is treated as 2

💬 danielabrozzoni commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786092641)
nit: this blank line seems unnecessary

💬 danielabrozzoni commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786107786)
nit: we could test that it's hidden in the functional test, similarly to `addpeeraddress`/`getrawaddrman` (https://github.com/bitcoin/bitcoin/blob/cfb59da4b3bb34afae467691a3e901f2b5a186f3/test/functional/rpc_net.py#L338-L341)

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1786141210)
The `QMAKE_*` variables are used by the legacy `qmake` build tools, which we neither build nor use.

From https://doc.qt.io/qt-6/qt6-buildsystem.html:
> The Qt 5 build system was built on top of [qmake](https://doc.qt.io/qt-6/qmake-manual.html). In Qt 6, we ported the build system to CMake.
>
> **Note:** This only affects users that want to build Qt from sources. You can still use qmake as a build tool for your applications.