💬 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
```
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1995405051)
This back-and-forth string conversion feels suboptimal. Perhaps an alternative approach would be to keep the integer values between `kernel_LogCategory` the same as `BCLog::LogFlags` and just define a kernel-specific bitfield that defines which `BCLog` flags are valid? I don't think the `kernel_LogCategory` enum values being non-continuous is a problem, since this might happen in the future anyway e.g. if certain components are moved out of kernel scope?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1995405051)
This back-and-forth string conversion feels suboptimal. Perhaps an alternative approach would be to keep the integer values between `kernel_LogCategory` the same as `BCLog::LogFlags` and just define a kernel-specific bitfield that defines which `BCLog` flags are valid? I don't think the `kernel_LogCategory` enum values being non-continuous is a problem, since this might happen in the future anyway e.g. if certain components are moved out of kernel scope?
💬 brunoerg commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#issuecomment-2725492948)
> I'm curious if there should be any doc updates accompanying this change.
No, it's a test-only change.
(https://github.com/bitcoin/bitcoin/pull/31870#issuecomment-2725492948)
> I'm curious if there should be any doc updates accompanying this change.
No, it's a test-only change.
📝 davidgumberg opened a pull request: "build: Drop option to disable hardening."
(https://github.com/bitcoin/bitcoin/pull/32071)
Follow up to #32038 which dropped `NO_HARDEN` from depends builds, this PR drops the `ENABLE_HARDENING` build option since disabling hardening of binaries should not be a supported or maintained use case.
Individual hardening flags and options can still be disabled by appending flags, e.g.:
```bash
cmake -B build \
-DAPPEND_CPPFLAGS='-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=0 -fno-stack-protector -fcf-protection=none -fno-stack-clash-protection' \
-DAPPEND_LDFLAGS='-Wl,-z,lazy -Wl,-z,nor
...
(https://github.com/bitcoin/bitcoin/pull/32071)
Follow up to #32038 which dropped `NO_HARDEN` from depends builds, this PR drops the `ENABLE_HARDENING` build option since disabling hardening of binaries should not be a supported or maintained use case.
Individual hardening flags and options can still be disabled by appending flags, e.g.:
```bash
cmake -B build \
-DAPPEND_CPPFLAGS='-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=0 -fno-stack-protector -fcf-protection=none -fno-stack-clash-protection' \
-DAPPEND_LDFLAGS='-Wl,-z,lazy -Wl,-z,nor
...
💬 davidgumberg commented on pull request "depends: remove `NO_HARDEN` option":
(https://github.com/bitcoin/bitcoin/pull/32038#issuecomment-2725534093)
I've opened https://github.com/bitcoin/bitcoin/pull/32071 to drop the `ENABLE_HARDENING` option in Bitcoin Core builds.
(https://github.com/bitcoin/bitcoin/pull/32038#issuecomment-2725534093)
I've opened https://github.com/bitcoin/bitcoin/pull/32071 to drop the `ENABLE_HARDENING` option in Bitcoin Core builds.
💬 fjahr commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2725629205)
re-ACK 8ceaecc89e20129f9c11727e4c7795633cfcdd2e
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2725629205)
re-ACK 8ceaecc89e20129f9c11727e4c7795633cfcdd2e
💬 fjahr commented on pull request "[draft] Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-2725685034)
Concept ACK
My understanding from the low-level networking discussion at CoreDev was that this wouldn't build on top of sockman. I guess the devil is in the details but can you address that in what sense the current approach follows what was discussed there? Thanks!
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-2725685034)
Concept ACK
My understanding from the low-level networking discussion at CoreDev was that this wouldn't build on top of sockman. I guess the devil is in the details but can you address that in what sense the current approach follows what was discussed there? Thanks!
💬 fjahr commented on pull request "test: testnet4 could log a disk space warning":
(https://github.com/bitcoin/bitcoin/pull/32057#issuecomment-2725697618)
utACK c09e2a72ff3b851d4c47d1e715b8bdcc54027364
Change looks good to me.
(https://github.com/bitcoin/bitcoin/pull/32057#issuecomment-2725697618)
utACK c09e2a72ff3b851d4c47d1e715b8bdcc54027364
Change looks good to me.
💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1995511334)
I think this `SnapshotcompletionResult` variant can be removed, no?
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1995511334)
I think this `SnapshotcompletionResult` variant can be removed, no?
🤔 TheCharlatan reviewed a pull request: "refactor: Improve assumeutxo state representation"
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-2685423528)
Approach ACK
I am not yet onvinced of the last three commits changing the code (5e22fdcdb2665c0bb5e47c53719796096afe2dd1, 58265b955a3d8622f22958943f79409129d58b36, ca48478fbfcc83cc6f8c928e57800707fe0c3b51), maybe they could do with some more motivation in the commit message? I guess the last of the commits ca48478fbfcc83cc6f8c928e57800707fe0c3b51 is kind of the result of the preceding two and it is nice not to have to construct the vector on the fly for every `GetAll()` call anymore.
There
...
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-2685423528)
Approach ACK
I am not yet onvinced of the last three commits changing the code (5e22fdcdb2665c0bb5e47c53719796096afe2dd1, 58265b955a3d8622f22958943f79409129d58b36, ca48478fbfcc83cc6f8c928e57800707fe0c3b51), maybe they could do with some more motivation in the commit message? I guess the last of the commits ca48478fbfcc83cc6f8c928e57800707fe0c3b51 is kind of the result of the preceding two and it is nice not to have to construct the vector on the fly for every `GetAll()` call anymore.
There
...
💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1996247059)
Shouldn't this skip over the historical chainstate if `m_target_utxohash` is set?
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1996247059)
Shouldn't this skip over the historical chainstate if `m_target_utxohash` is set?
💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1995924122)
Nit: `s/if is/if it is/`. I think there are some missing articles around here too.
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1995924122)
Nit: `s/if is/if it is/`. I think there are some missing articles around here too.
💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1996212305)
I don't quite follow this change. What is wrong with using `CurrentChainstate` here?
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1996212305)
I don't quite follow this change. What is wrong with using `CurrentChainstate` here?