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

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

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

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

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

💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1996244623)
Why is this not re-used for `ProcessNewBlock`?

💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1995939302)
Nit: `s/this a/this is a/`

💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1995921074)
Nit: Are we still doing `-present` now? I thought I saw some discussion about that recently, but can't find it anymore.

💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1996249639)
I'm surprised this is not just taking the `m_chainstates` size. Why is that?

💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1996263354)
I'm a bit confused when it is appropriate to use `::cs_main` and when `chainman.GetMutex()`. Is there a rule to that?

📝 hodlinator opened a pull request: "net: Only try downgrade v2->v1 transport if online"
(https://github.com/bitcoin/bitcoin/pull/32073)
We might have just set `fDisconnect` in the preceding loop because of being offline.

Attempting to reconnect using v1 transport just because `fNetworkActive` was set to `false` at the "right" stage in the v2 handshake does not make sense.

Issue [discovered](https://github.com/bitcoin/bitcoin/pull/31633#discussion_r1930908304) by davidgumberg.

💬 hodlinator commented on pull request "net: Only try downgrade v2->v1 transport if online":
(https://github.com/bitcoin/bitcoin/pull/32073#issuecomment-2725820358)
#### How I tested

Run node on mainnet:
```shell
bitcoind -debug=net -daemonwait
```
Tail log for relevant info:
```shell
tail -f ~/.bitcoin/debug.log | grep -E "SetNetworkActive|Network not active|retrying"
```
In another console, spam:
```shell
build/bin/bitcoin-cli setnetworkactive false && sleep 0.5 && build/bin/bitcoin-cli setnetworkactive true
```

Expected grep results:
```
2025-03-14T21:12:17Z SetNetworkActive: false
2025-03-14T21:12:17Z [net] Network not active, discon
...

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2725832595)
Re https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2725437236 and https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2725441239

Thank you for your suggestions!

> Would it be possible to use these C headers to automatically build a rust crate of constants from a C header API? Or would that be overkill..

Looking at the constants in the linked file they all seem to be policy-related, which is out of scope for now. I don't think we'll add a header for that in the near fut
...