💬 hebasto commented on pull request "build: remove ENABLE_HARDENING condition from check-security":
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665454487)
> > In case when "hardening was inadvertently disabled".
>
> I don't know what you mean. If hardening was accidently disabled then the build failing is a good thing.

Correct.However, this PR does the opposite: the `check-security` custom target is available unconditionally, and building it won't fail (at least at the build system level).

💬 fanquake commented on pull request "build: remove ENABLE_HARDENING condition from check-security":
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665459821)
> the check-security custom target is available unconditionally

This is what we want.

> and building it won't fail (at least at the build system level).

It will fail, because the checks will fail.

💬 hebasto commented on pull request "build: remove ENABLE_HARDENING condition from check-security":
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665463899)
> > the check-security custom target is available unconditionally
>
> This is what we want.

What reasons for?

>
> > and building it won't fail (at least at the build system level).
>
> It will fail, because the checks will fail.

It should fail earlier, at the build system level.

💬 l0rinc commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2665467283)
Tend to agree with @maflcko, I vote to keep it as is, given that it's already pushed

💬 TheCharlatan commented on pull request "build: remove ENABLE_HARDENING condition from check-security":
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665496209)
> It should fail earlier, at the build system level.

I'm not sure about making it fail on the build system level (i.e. keeping as is). Isn't that just adding brittle assumptions to the target's configuration?

💬 maflcko commented on issue "test: feature_assumeutxo.py Windows timeout":
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2665500269)
> Or maybe investigate why the `net` thread appears to not get the interrupt signal?

My impression was that the `net` thread is stopped, along with the http workers, and the http thread. See:

```
node1 ...T14:10:53.440186Z (mocktime...) [net] [D:\a\bitcoin\bitcoin\src\util\thread.cpp:22] [TraceThread] net thread exit
```

There seem to be two issues here:

* The `stop` RPC timing out
* The node not stopping

💬 fanquake commented on pull request "build: remove ENABLE_HARDENING condition from check-security":
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665503440)
> Isn't that just adding brittle assumptions to the target's configuration?

Yea. I don't see why we need to add more logic to try and preemptively catch some issue, rather than just unconditionally running the tests that soley exist to catch that issue.

💬 pinheadmz commented on issue "test: feature_assumeutxo.py Windows timeout":
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2665506921)
Hm yeah, but then why does `enabling extra block-relay-only peers` happen after that?

✅ fanquake closed a pull request: "cmake: add optional source files to bitcoin_crypto and crc32c directly"
(https://github.com/bitcoin/bitcoin/pull/31268)

✅ fanquake closed an issue: "ci: tidy task doesn't run on aarch64"
(https://github.com/bitcoin/bitcoin/issues/31697)

👋 fanquake's pull request is ready for review: "build: align debugging flags to `-O0`"
(https://github.com/bitcoin/bitcoin/pull/29796)

🤔 vasild reviewed a pull request: "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods"
(https://github.com/bitcoin/bitcoin/pull/31785#pullrequestreview-2623295004)
Approach ACK 2042ed105578a71234295b01be39ad8607722abe

💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1959586025)
The `Assume()` can be triggered during normal circumstances: if the first wait quits with `TipBlock()` null and `chainman().m_interrupt` being `true`. The easiest way to work around this would be to swap the order in the second condition:

```diff
- Assume(notifications().TipBlock()) != current_tip || chainman().m_interrupt
+ chainman().m_interrupt || Assume(notifications().TipBlock()) != current_tip
```
that might be a bit fragile though, wrt future changes.

💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1959608374)
Same here, if the user provides `height=0`, and `getTip()` returns `nullopt`, then this will be `while (0 < 0)`, will never enter and `block` will remain `nullopt` and be dereferenced at the end.

💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1959617648)
nit: maybe use `uint256::ZERO`, also used in commit b954af72e4d6a83b3ba701b7ee52d97f2af3254b `Have createNewBlock() wait for a tip`:

```suggestion
uint256 tip_hash{block ? block->hash : uint256::ZERO};
```

💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1959604318)
This could crash in the following (stupid) scenario:
* the user provides `0` as desired block hash to the `waitforblock`
* `getTip()` returns `nullopt`, which is assigned to `block`
* the `while` loop is never entered, not even once so `block` remains `nullopt`
* at the end we try to dereference the `nullopt` `block` :rescue_worker_helmet:

We shouldn't crash even on such "idiotic" input. Maybe change the `while` condition:

```diff
- while (tip_hash != hash) {
+ while (tip_hash != ha
...

👍 hebasto approved a pull request: "build: remove ENABLE_HARDENING condition from check-security"
(https://github.com/bitcoin/bitcoin/pull/31892#pullrequestreview-2623386934)
ACK 113a7a363faf1ace6b08a28cf4075dbce05f8f04.

> This check is only used in release builds...

Not directly related to this PR, but It seems reasonable to consider removing this condition as well:https://github.com/bitcoin/bitcoin/blob/28dec6c5f8bd35ef4e6cb8d7aa3f21b6879acf98/cmake/module/Maintenance.cmake#L27

👋 hebasto's pull request is ready for review: "cmake: Make implicit `libbitcoinkernel` dependencies explicit"
(https://github.com/bitcoin/bitcoin/pull/31884)

💬 hebasto commented on pull request "cmake: Make implicit `libbitcoinkernel` dependencies explicit":
(https://github.com/bitcoin/bitcoin/pull/31884#issuecomment-2665584719)
cc @TheCharlatan @theuni

🤔 rkrux reviewed a pull request: "descriptor: check whitespace in pubkeys within fragments"
(https://github.com/bitcoin/bitcoin/pull/31603#pullrequestreview-2623382128)
Somewhere either in the PR title or description, it should be mentioned that this works for descriptors containing private keys as well as observed in the tests and in the `ParsePubKeyInner` code: https://github.com/bitcoin/bitcoin/blob/28dec6c5f8bd35ef4e6cb8d7aa3f21b6879acf98/src/script/descriptor.cpp#L1540-L1550