💬 yancyribbens commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#discussion_r1995938540)
```suggestion
// (with N being the number of peers from which we're downloading validated blocks), disconnect due to timeout
```
(https://github.com/bitcoin/bitcoin/pull/32051#discussion_r1995938540)
```suggestion
// (with N being the number of peers from which we're downloading validated blocks), disconnect due to timeout
```
💬 ryanofsky commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2725260200)
> I can get behind that, can you point me to other usages where we could use this?
I don't know, but if I were looking I'd just grep for places where AutoFile class is used. I don't have any real concerns here and I'm fine with the current approach. To the extent I do have concerns they are about readability and maintainability of code not performance, so I like an approach that makes it trivial to turn buffering on and off, which is I why thought BufferedReader / BufferedWriter stream adapte
...
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2725260200)
> I can get behind that, can you point me to other usages where we could use this?
I don't know, but if I were looking I'd just grep for places where AutoFile class is used. I don't have any real concerns here and I'm fine with the current approach. To the extent I do have concerns they are about readability and maintainability of code not performance, so I like an approach that makes it trivial to turn buffering on and off, which is I why thought BufferedReader / BufferedWriter stream adapte
...
💬 yancyribbens commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#issuecomment-2725268217)
Concept ACK. I'm curious if there should be any doc updates accompanying this change.
(https://github.com/bitcoin/bitcoin/pull/31870#issuecomment-2725268217)
Concept ACK. I'm curious if there should be any doc updates accompanying this change.
💬 janb84 commented on pull request "contrib: Update coverage.cpp to drop linux restriction":
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1995978301)
### Main problem:
> "macOS’s linker (ld64) doesn’t silently resolve unreferenced weak symbols to NULL when they’re called—it demands a definition".
Combined with calling `__gcov_reset` and `__llvm_profile_reset_counters` directly, forcing the linker to find a definition, will result in that the linker will break on trying to link the following code:
```C
extern "C" void __llvm_profile_reset_counters(void) __attribute__((weak));
extern "C" void __gcov_reset(void) __attribute__((weak)
...
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1995978301)
### Main problem:
> "macOS’s linker (ld64) doesn’t silently resolve unreferenced weak symbols to NULL when they’re called—it demands a definition".
Combined with calling `__gcov_reset` and `__llvm_profile_reset_counters` directly, forcing the linker to find a definition, will result in that the linker will break on trying to link the following code:
```C
extern "C" void __llvm_profile_reset_counters(void) __attribute__((weak));
extern "C" void __gcov_reset(void) __attribute__((weak)
...
💬 janb84 commented on pull request "contrib: Update coverage.cpp to drop linux restriction":
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2725326675)
In my write-up of the explanation to hodlinator I discovered one edge case that needed fixing, sorry for the new commit.
Reason:
Weak fallbacks (__attribute__((weak))) were added instead of strong ones to ensure the profiling runtime’s implementations are used when profiling is enabled, **_avoiding linker conflicts or silent overrides_**. This guarantees correct counter resets while keeping safe, empty fallbacks when profiling is off, improving portability and reliability across Clang and G
...
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2725326675)
In my write-up of the explanation to hodlinator I discovered one edge case that needed fixing, sorry for the new commit.
Reason:
Weak fallbacks (__attribute__((weak))) were added instead of strong ones to ensure the profiling runtime’s implementations are used when profiling is enabled, **_avoiding linker conflicts or silent overrides_**. This guarantees correct counter resets while keeping safe, empty fallbacks when profiling is off, improving portability and reliability across Clang and G
...
💬 VolodymyrBg commented on pull request "test: Add support for mainnet addresses in address_to_scriptpubkey":
(https://github.com/bitcoin/bitcoin/pull/32060#issuecomment-2725346210)
@maflcko Done
(https://github.com/bitcoin/bitcoin/pull/32060#issuecomment-2725346210)
@maflcko Done
💬 VolodymyrBg commented on pull request "test: Add support for mainnet addresses in address_to_scriptpubkey":
(https://github.com/bitcoin/bitcoin/pull/32060#issuecomment-2725350636)
@davidgumberg I applied myself. Thank you
(https://github.com/bitcoin/bitcoin/pull/32060#issuecomment-2725350636)
@davidgumberg I applied myself. Thank you
💬 zzzi2p commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#issuecomment-2725369358)
Thanks for CC'ing me. I also asked eyedeekay and orignal from i2pd to take a look.
10 standby sessions is a lot, except maybe at startup. If your I2P connections limit is 10, then you're doubling your network resource usage. 2 or 3 is probably enough, depending on how fast your connections churn and how long is long enough for a unused session to be up to give you the anonymity you're looking for.
A couple of alternatives:
```c
int max = MAX_UNUSED_I2P_SESSIONS_SIZE;
if (uptime() > 15
...
(https://github.com/bitcoin/bitcoin/pull/32065#issuecomment-2725369358)
Thanks for CC'ing me. I also asked eyedeekay and orignal from i2pd to take a look.
10 standby sessions is a lot, except maybe at startup. If your I2P connections limit is 10, then you're doubling your network resource usage. 2 or 3 is probably enough, depending on how fast your connections churn and how long is long enough for a unused session to be up to give you the anonymity you're looking for.
A couple of alternatives:
```c
int max = MAX_UNUSED_I2P_SESSIONS_SIZE;
if (uptime() > 15
...
💬 yancyribbens commented on pull request "wallet: refactor: various master key encryption cleanups":
(https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1996031367)
```suggestion
static constexpr unsigned int DEFAULT_DERIVE_ITERATIONS = 25'000;
```
(https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1996031367)
```suggestion
static constexpr unsigned int DEFAULT_DERIVE_ITERATIONS = 25'000;
```
💬 yancyribbens commented on pull request "wallet: refactor: various master key encryption cleanups":
(https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1996031577)
```suggestion
// 25,000 rounds is just under 0.1 seconds on a 1.86 GHz Pentium M
```
(https://github.com/bitcoin/bitcoin/pull/31398#discussion_r1996031577)
```suggestion
// 25,000 rounds is just under 0.1 seconds on a 1.86 GHz Pentium M
```
💬 yancyribbens commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2725437236)
rust-bitcoin maintains a file of [constants](https://github.com/tcharding/rust-bitcoin/blob/0ca9fcfd0ea81a7bb0d781bdc07a136cea9d0796/bitcoin/src/policy.rs) which are meant to mirror values in core. We've discussed trying to find an automated solution to keep these consts synchronized since right now, these are manually maintained. This is a point of annoyance since these values are constantly becoming stale (pun intended). Would it be possible to use these C headers to automatically build a r
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2725437236)
rust-bitcoin maintains a file of [constants](https://github.com/tcharding/rust-bitcoin/blob/0ca9fcfd0ea81a7bb0d781bdc07a136cea9d0796/bitcoin/src/policy.rs) which are meant to mirror values in core. We've discussed trying to find an automated solution to keep these consts synchronized since right now, these are manually maintained. This is a point of annoyance since these values are constantly becoming stale (pun intended). Would it be possible to use these C headers to automatically build a r
...
💬 yancyribbens commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2725441239)
Furthermore, besides keeping values stale, an automated solution which would generate all available consts would be ideal.
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2725441239)
Furthermore, besides keeping values stale, an automated solution which would generate all available consts would be ideal.
💬 darosior commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1996080156)
I'm not talking about the state but the stats. My comment was about how in this commit you replace `if (ThresholdState::LOCKED_IN != current_state)` by `if (info.stats->threshold > 0 || info.stats->possible) {` to show the `threshold` and `possible` fields in the RPC, and this relies on these specific lines. I thought the only reason you had this logic here was to convey to the RPC whether to show these fields, which is a bit convoluted. I don't think this is worth touching at this point.
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1996080156)
I'm not talking about the state but the stats. My comment was about how in this commit you replace `if (ThresholdState::LOCKED_IN != current_state)` by `if (info.stats->threshold > 0 || info.stats->possible) {` to show the `threshold` and `possible` fields in the RPC, and this relies on these specific lines. I thought the only reason you had this logic here was to convey to the RPC whether to show these fields, which is a bit convoluted. I don't think this is worth touching at this point.
💬 yancyribbens commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r1996089043)
The [wiki](https://github.com/bitcoin/bips/blob/master/bip-0094.mediawiki) says the default port is `48333`. Is this a typo?
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r1996089043)
The [wiki](https://github.com/bitcoin/bips/blob/master/bip-0094.mediawiki) says the default port is `48333`. Is this a typo?
💬 maflcko commented on pull request "build: enable libc++ hardening in debug builds":
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2725472077)
I tried this with GCC in debug mode, but it passed without error. What are the exact steps to reproduce your claim?
> Note that `libstdc++`'s `_GLIBCXX_ASSERTIONS` (aka light debug mode) is [enabled by default](https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros.html) when compiling without optimizations), that is in our debug builds.
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2725472077)
I tried this with GCC in debug mode, but it passed without error. What are the exact steps to reproduce your claim?
> Note that `libstdc++`'s `_GLIBCXX_ASSERTIONS` (aka light debug mode) is [enabled by default](https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros.html) when compiling without optimizations), that is in our debug builds.
💬 Sjors commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r1996094358)
48333 is the default p2p port
48332 is used for RPC, which is not defined in the BIP because it's specific to Bitcoin Core (as is ZMQ).
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r1996094358)
48333 is the default p2p port
48332 is used for RPC, which is not defined in the BIP because it's specific to Bitcoin Core (as is ZMQ).
🤔 zaidmstrr reviewed a pull request: "fuzz: split `coinselection` harness"
(https://github.com/bitcoin/bitcoin/pull/31870#pullrequestreview-2686505153)
reACK [ba82240](https://github.com/bitcoin/bitcoin/pull/31870/commits/ba82240553ddd534287845e10bc76b46b45329fe)
As the previous issue raised by maflcko is resolved.
(https://github.com/bitcoin/bitcoin/pull/31870#pullrequestreview-2686505153)
reACK [ba82240](https://github.com/bitcoin/bitcoin/pull/31870/commits/ba82240553ddd534287845e10bc76b46b45329fe)
As the previous issue raised by maflcko is resolved.
🤔 stickies-v reviewed a pull request: "kernel: Introduce initial C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-2685229851)
A few comments regarding logging. It's a bit awkward to have LoggingConnection instances, but only global setters to update their granularity, so #30342 looks like a welcome improvement.
Besides that, hooking up a downstream log viewer in py-bitcoinkernel was fairly straightforward. Having a struct `kernel_Log` callback instead of having to parse a string for various fields (time, threadname, level, ...) would be nice, and I think not even a huge left (can be done without upstream changes, ev
...
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-2685229851)
A few comments regarding logging. It's a bit awkward to have LoggingConnection instances, but only global setters to update their granularity, so #30342 looks like a welcome improvement.
Besides that, hooking up a downstream log viewer in py-bitcoinkernel was fairly straightforward. Having a struct `kernel_Log` callback instead of having to parse a string for various fields (time, threadname, level, ...) would be nice, and I think not even a huge left (can be done without upstream changes, ev
...
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1995880696)
Since log levels are ordered, would it be prudent to reserve space for intermediate levels? E.g. if we decide we do want to add WARNING/ERROR later, we'd have to change existing log levels.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1995880696)
Since log levels are ordered, would it be prudent to reserve space for intermediate levels? E.g. if we decide we do want to add WARNING/ERROR later, we'd have to change existing log levels.
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1995386796)
This (+ in `log_category_to_string()` leads to runtime assertion errors for interpreted languages. It also means that `add_log_level_category()`, `enable_log_category()` and `disable_log_category()` are basically `void` instead of `bool` because they can only return `true` (or crash).
E.g. in python:
```
>>> pbk.add_log_level_category(99, 20)
Assertion failed: (false), function log_level_to_string, file bitcoinkernel.cpp, line 87.
zsh: abort python
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1995386796)
This (+ in `log_category_to_string()` leads to runtime assertion errors for interpreted languages. It also means that `add_log_level_category()`, `enable_log_category()` and `disable_log_category()` are basically `void` instead of `bool` because they can only return `true` (or crash).
E.g. in python:
```
>>> pbk.add_log_level_category(99, 20)
Assertion failed: (false), function log_level_to_string, file bitcoinkernel.cpp, line 87.
zsh: abort python
```