💬 hodlinator commented on issue "test: feature_assumeutxo.py Windows timeout":
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2665304532)
@pinheadmz since you are working on the HTTP rewrite... have you noticed anything fishy regarding whether the stop-RPC will actually always get the response sent off back to the client or not?
Current stop-RPC implementation implies it should always work:
https://github.com/bitcoin/bitcoin/blob/790c509a4796ec47be2275d86b35370ff25a599a/src/rpc/server.cpp#L170-L172
But it's from 2015 and the log above makes me very suspicious.
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2665304532)
@pinheadmz since you are working on the HTTP rewrite... have you noticed anything fishy regarding whether the stop-RPC will actually always get the response sent off back to the client or not?
Current stop-RPC implementation implies it should always work:
https://github.com/bitcoin/bitcoin/blob/790c509a4796ec47be2275d86b35370ff25a599a/src/rpc/server.cpp#L170-L172
But it's from 2015 and the log above makes me very suspicious.
💬 maflcko commented on issue "test: feature_assumeutxo.py Windows timeout":
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2665368484)
It looks like the scheduler thread never exited? It may be related to the comment:
```
// After everything has been shut down, but before things get flushed, stop the
// the scheduler. After this point, SyncWithValidationInterfaceQueue() should not be called anymore
// as this would prevent the shutdown from completing.
if (node.scheduler) node.scheduler->stop();
```
However, I don't see which thread could have called `SyncWithValidationInterfaceQueue` for it to block.
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2665368484)
It looks like the scheduler thread never exited? It may be related to the comment:
```
// After everything has been shut down, but before things get flushed, stop the
// the scheduler. After this point, SyncWithValidationInterfaceQueue() should not be called anymore
// as this would prevent the shutdown from completing.
if (node.scheduler) node.scheduler->stop();
```
However, I don't see which thread could have called `SyncWithValidationInterfaceQueue` for it to block.
🤔 hebasto reviewed a pull request: "build: remove ENABLE_HARDENING condition from check-security"
(https://github.com/bitcoin/bitcoin/pull/31892#pullrequestreview-2623248603)
For historical context, the current CMake's implementation mirrors the Autotools' implementation introduced in f3d3eaf78eb51238d799d8f20a585550d1567719:https://github.com/bitcoin/bitcoin/blob/32efe850438ef22e2de39e562af557872a402c31/src/Makefile.am#L1057-L1061
(https://github.com/bitcoin/bitcoin/pull/31892#pullrequestreview-2623248603)
For historical context, the current CMake's implementation mirrors the Autotools' implementation introduced in f3d3eaf78eb51238d799d8f20a585550d1567719:https://github.com/bitcoin/bitcoin/blob/32efe850438ef22e2de39e562af557872a402c31/src/Makefile.am#L1057-L1061
💬 hebasto commented on pull request "build: remove ENABLE_HARDENING condition from check-security":
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665425211)
> This check is only used in release builds, where hardening should always be enabled. I can't think of a reason we'd want to silently skip these checks if hardening was inadvertently disabled.
In that case, removing the dummy target is the correct solution, but the `if(ENABLE_HARDENING)` condition should remain:
```
$ cmake -B build -G "Ninja" -DENABLE_HARDENING=OFF
$ cmake --build build -t check-security
ninja: error: unknown target 'check-security'
```
or
```
$ cmake -B build -G "U
...
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665425211)
> This check is only used in release builds, where hardening should always be enabled. I can't think of a reason we'd want to silently skip these checks if hardening was inadvertently disabled.
In that case, removing the dummy target is the correct solution, but the `if(ENABLE_HARDENING)` condition should remain:
```
$ cmake -B build -G "Ninja" -DENABLE_HARDENING=OFF
$ cmake --build build -t check-security
ninja: error: unknown target 'check-security'
```
or
```
$ cmake -B build -G "U
...
💬 pinheadmz commented on issue "test: feature_assumeutxo.py Windows timeout":
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2665433174)
> [@pinheadmz](https://github.com/pinheadmz) since you are working on the HTTP rewrite...
I hadn't noticed anything about the current implementation but managing the RPC `stop` race condition was definitely a tricky part of the rewrite.
One thing to note about the actual total time elapsed in your case is that on CI the timeout factors are multiplied by large factors in some cases to avoid timeout errors on low-resource runners:
https://github.com/bitcoin/bitcoin/blob/78c19ddd7c56a275d2ae835
...
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2665433174)
> [@pinheadmz](https://github.com/pinheadmz) since you are working on the HTTP rewrite...
I hadn't noticed anything about the current implementation but managing the RPC `stop` race condition was definitely a tricky part of the rewrite.
One thing to note about the actual total time elapsed in your case is that on CI the timeout factors are multiplied by large factors in some cases to avoid timeout errors on low-resource runners:
https://github.com/bitcoin/bitcoin/blob/78c19ddd7c56a275d2ae835
...
💬 fanquake commented on pull request "build: remove ENABLE_HARDENING condition from check-security":
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665434797)
> but the if(ENABLE_HARDENING) condition should remain:
Why? Those aren't release builds.
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665434797)
> but the if(ENABLE_HARDENING) condition should remain:
Why? Those aren't release builds.
💬 hebasto commented on pull request "build: remove ENABLE_HARDENING condition from check-security":
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665439339)
> > but the if(ENABLE_HARDENING) condition should remain:
>
> Why? Those aren't release builds.
In case when "hardening was inadvertently disabled".
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665439339)
> > but the if(ENABLE_HARDENING) condition should remain:
>
> Why? Those aren't release builds.
In case when "hardening was inadvertently disabled".
💬 fanquake commented on pull request "build: remove ENABLE_HARDENING condition from check-security":
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665441973)
> 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.
(https://github.com/bitcoin/bitcoin/pull/31892#issuecomment-2665441973)
> 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.
💬 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).
(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.
(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.
(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
(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?
(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
(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.
(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?
(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)
(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)
(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)
(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
(https://github.com/bitcoin/bitcoin/pull/31785#pullrequestreview-2623295004)
Approach ACK 2042ed105578a71234295b01be39ad8607722abe