💬 dergoegge commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1785885817)
This won't define the c++ macro `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`, as this only sets a cmake build flag (cmake probably complains that this flag doesn't exist).
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).
You will also need to add this to the correct section in the docs (i.e. the net
...
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1785885817)
This won't define the c++ macro `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`, as this only sets a cmake build flag (cmake probably complains that this flag doesn't exist).
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).
You will also need to add this to the correct section in the docs (i.e. the net
...
💬 dergoegge commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1785875497)
I think this could be de-duplicated and made a little cleaner (same for the magic byte check):
```c++
bool checksum_ok{memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) == 0};
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
checksum_ok = checksum_ok || (hdr.pchChecksum[0] & 1) != 0;
#endif
if (!checksum_ok) {
...
```
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1785875497)
I think this could be de-duplicated and made a little cleaner (same for the magic byte check):
```c++
bool checksum_ok{memcmp(hash.begin(), hdr.pchChecksum, CMessageHeader::CHECKSUM_SIZE) == 0};
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
checksum_ok = checksum_ok || (hdr.pchChecksum[0] & 1) != 0;
#endif
if (!checksum_ok) {
...
```
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2390929867)
Tested and mostly happy with the first two preparation commits. Note to other reviewers: see discussion in https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1567767610 for why the prefixes are hardcoded instead of derived.
The main commit fd02e7650f6410824a64f1403a98583b7ec6b0dd might be easier to review if you split it up. For example one commit that improves the base58 vs bech32 detection, another commit that that improves base58 error handling and finally one for bech32.
Re last
...
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2390929867)
Tested and mostly happy with the first two preparation commits. Note to other reviewers: see discussion in https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1567767610 for why the prefixes are hardcoded instead of derived.
The main commit fd02e7650f6410824a64f1403a98583b7ec6b0dd might be easier to review if you split it up. For example one commit that improves the base58 vs bech32 detection, another commit that that improves base58 error handling and finally one for bech32.
Re last
...
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1785858113)
7478ac6af922c302e292ace7f61e6eaa461da727: this change belongs in the previous commit 07f9733e6022d77d755321914a2a2a1dd7ce0d59
(One way to move it there is by doing `git rebase -i HEAD~3`, setting "e" for the first commit, and then add these lines. Then you do a `git commit -a --amend` to update the first commit. Then `git rebase --continue`, which should automatically adjust the second commit to omit this change.)
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1785858113)
7478ac6af922c302e292ace7f61e6eaa461da727: this change belongs in the previous commit 07f9733e6022d77d755321914a2a2a1dd7ce0d59
(One way to move it there is by doing `git rebase -i HEAD~3`, setting "e" for the first commit, and then add these lines. Then you do a `git commit -a --amend` to update the first commit. Then `git rebase --continue`, which should automatically adjust the second commit to omit this change.)
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1785860610)
@maflcko I'm puzzled why the `test each commit` job didn't catch this. No test coverage? Shouldn't it use `-Werror`?
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1785860610)
@maflcko I'm puzzled why the `test each commit` job didn't catch this. No test coverage? Shouldn't it use `-Werror`?
💬 laanwj commented on pull request "log: Enforce trailing newline":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1785925915)
thanks for doing it in this way instead of making it an error/warning, i like that we can now just do LogPrintfs without typing `\n` every time
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1785925915)
thanks for doing it in this way instead of making it an error/warning, i like that we can now just do LogPrintfs without typing `\n` every time
💬 laanwj commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2390962613)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2390962613)
Concept ACK
💬 hodlinator commented on pull request "Windows bitcoind stall debugging [WIP, NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2390985894)
I finally read the documentation for [schedule](https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#schedule) a bit more closely and realized it only applies to the master branch, so I'm making the "every 3 hours"-schedule happen on my personal master branch for now.
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2390985894)
I finally read the documentation for [schedule](https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#schedule) a bit more closely and realized it only applies to the master branch, so I'm making the "every 3 hours"-schedule happen on my personal master branch for now.
🤔 dergoegge reviewed a pull request: "test: Add mockable steady clock, tests for PCP and NATPMP implementations"
(https://github.com/bitcoin/bitcoin/pull/31022#pullrequestreview-2345125043)
Concept ACK
Left some comments on the approach for the steady mock time.
(https://github.com/bitcoin/bitcoin/pull/31022#pullrequestreview-2345125043)
Concept ACK
Left some comments on the approach for the steady mock time.
💬 dergoegge commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1785930990)
Could we turn `SteadyClock` into this struct instead? Having both types could be confusing.
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1785930990)
Could we turn `SteadyClock` into this struct instead? Having both types could be confusing.
💬 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?
(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
(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.
(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
...
(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 😄
(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)
(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.
(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
(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.
(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.
(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
(https://github.com/bitcoin/bitcoin/pull/30992#pullrequestreview-2345306765)
ACK 36a6d4b0078ebb39ed082c866bf49214a2a01241