💬 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;
```

💬 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
```

💬 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
...

💬 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.

💬 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.

💬 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?

💬 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.

💬 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).

🤔 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.

🤔 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
...

💬 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.

💬 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
```

💬 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?

💬 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.

📝 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
...

💬 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.

💬 fjahr commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(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!

💬 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.

💬 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?