💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1785885728)
Changed, thanks!
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1785885728)
Changed, thanks!
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1785886009)
Removed.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1785886009)
Removed.
🤔 hodlinator reviewed a pull request: "bench: add support for custom data directory"
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2344877863)
Concept ACK 1ba225c6ce689defb7063dd68f62e1e90fcdda5e
Good to see these globals implemented for bench. As already mentioned, I've been working on a partially overlapping change in #30737.
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2344877863)
Concept ACK 1ba225c6ce689defb7063dd68f62e1e90fcdda5e
Good to see these globals implemented for bench. As already mentioned, I've been working on a partially overlapping change in #30737.
💬 hodlinator commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1785805254)
nit: Redundant comment.
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1785805254)
nit: Redundant comment.
💬 hodlinator commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1785776060)
[Braces on new lines for classes, functions, methods.](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c)
Can see you are following style of function above, but function below follows the dev-notes.
Don't feel as strongly about capitalization.
```suggestion
static std::vector<std::string> ParseTestSetupArgs(const ArgsManager& argsman)
{
```
If you want to make `parsePriorityLevel` conform to the dev-notes as well, I support it.
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1785776060)
[Braces on new lines for classes, functions, methods.](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c)
Can see you are following style of function above, but function below follows the dev-notes.
Don't feel as strongly about capitalization.
```suggestion
static std::vector<std::string> ParseTestSetupArgs(const ArgsManager& argsman)
{
```
If you want to make `parsePriorityLevel` conform to the dev-notes as well, I support it.
💬 hodlinator commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1785821577)
Why not make `SetupUnitTestArgs` available and call it here instead? Delegates the responsibility of documenting the default value to that part of the code.
(Could maybe rename `SetupUnitTestArgs` -> `SetupTestArgs` or `static BasicTestingSetup::RegisterArgs` to make it less unit test-specific and communicate how broadly it is used).
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1785821577)
Why not make `SetupUnitTestArgs` available and call it here instead? Delegates the responsibility of documenting the default value to that part of the code.
(Could maybe rename `SetupUnitTestArgs` -> `SetupTestArgs` or `static BasicTestingSetup::RegisterArgs` to make it less unit test-specific and communicate how broadly it is used).
💬 hodlinator commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1785827487)
(Please either remove empty line after local global here or add an empty line after `static std::string g_running_benchmark_name`).
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1785827487)
(Please either remove empty line after local global here or add an empty line after `static std::string g_running_benchmark_name`).
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1785892026)
> it would be better not to do change addrman based on what happens in private connections
I agree! Changed to avoid the `Good()` and `Connected()` calls for private broadcast.
On the second problem - I agree that more tried peers (which happens naturally over time) and more people using private broadcast would help because then the recipient just sees multiple broadcasts and has no way to relate them.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1785892026)
> it would be better not to do change addrman based on what happens in private connections
I agree! Changed to avoid the `Good()` and `Connected()` calls for private broadcast.
On the second problem - I agree that more tried peers (which happens naturally over time) and more people using private broadcast would help because then the recipient just sees multiple broadcasts and has no way to relate them.
💬 fanquake commented on issue "win64-cross CI timeout after 2h ":
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2390910998)
https://cirrus-ci.com/task/5934269598531584?logs=ci#L2683:
```bash
[14:05:20.035] 131/136 Test #132: wallet_tests ......................... Passed 16.54 sec
[14:05:27.163] 132/136 Test #122: coinselector_tests ................... Passed 35.31 sec
[14:07:02.612] 133/136 Test #6: tests ................................ Passed 262.37 sec
[14:07:04.392] 134/136 Test #9: addrman_tests ........................ Passed 264.14 sec
[14:10:59.874] 135/136 Test #1: util_test_runner
...
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2390910998)
https://cirrus-ci.com/task/5934269598531584?logs=ci#L2683:
```bash
[14:05:20.035] 131/136 Test #132: wallet_tests ......................... Passed 16.54 sec
[14:05:27.163] 132/136 Test #122: coinselector_tests ................... Passed 35.31 sec
[14:07:02.612] 133/136 Test #6: tests ................................ Passed 262.37 sec
[14:07:04.392] 134/136 Test #9: addrman_tests ........................ Passed 264.14 sec
[14:10:59.874] 135/136 Test #1: util_test_runner
...
💬 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