💬 achow101 commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2339623497)
Not really that much faster: https://cirrus-ci.com/build/5557255590903808, but possibly also misconfigured as everything was setup to run on that one machine simultaneously, although it should enough cores and memory.
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2339623497)
Not really that much faster: https://cirrus-ci.com/build/5557255590903808, but possibly also misconfigured as everything was setup to run on that one machine simultaneously, although it should enough cores and memory.
💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2339651980)
Hmm, you could also try to re-run the tasks individually to see the best possible performance (which is what was tested above as well).
Otherwise, my suggestion would be to go with one of the AMD CPUs mentioned above. (I can give this a try in the coming days)
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2339651980)
Hmm, you could also try to re-run the tasks individually to see the best possible performance (which is what was tested above as well).
Otherwise, my suggestion would be to go with one of the AMD CPUs mentioned above. (I can give this a try in the coming days)
💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1751290182)
Yes. I am thinking that it could make sense to have a wrapper-header around tinyformat, so that each relevant tfm function can be called with `ConstevalFormatString`.
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1751290182)
Yes. I am thinking that it could make sense to have a wrapper-header around tinyformat, so that each relevant tfm function can be called with `ConstevalFormatString`.
💬 maflcko commented on issue "Trying to run bitcoin qt on Windows and getting an AV":
(https://github.com/bitcoin/bitcoin/issues/30825#issuecomment-2339720856)
It would also be good to confirm the exact commit hash where this happened, because `master` may have changed when it was pulled and when the issue was filed (and certainly has changed now).
(https://github.com/bitcoin/bitcoin/issues/30825#issuecomment-2339720856)
It would also be good to confirm the exact commit hash where this happened, because `master` may have changed when it was pulled and when the issue was filed (and certainly has changed now).
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2339769505)
Trivial rebase after #30509.
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2339769505)
Trivial rebase after #30509.
💬 Sjors commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#issuecomment-2339814474)
Post-merge re-utACK 30073e6b3a24cbe417c45cd5df6a3a2de0251e9d
(https://github.com/bitcoin/bitcoin/pull/30509#issuecomment-2339814474)
Post-merge re-utACK 30073e6b3a24cbe417c45cd5df6a3a2de0251e9d
💬 stickies-v commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#issuecomment-2339825378)
rebase re-ACK 6da47f6ba5450e7f2b2ab2a64c7cfd354f67a1a9, CI timeout failure seems unrelated
(https://github.com/bitcoin/bitcoin/pull/30618#issuecomment-2339825378)
rebase re-ACK 6da47f6ba5450e7f2b2ab2a64c7cfd354f67a1a9, CI timeout failure seems unrelated
💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1751353778)
988dacead4a9a6850b767a8ced0c08b47fece56d: can you add a comment for why this isn't in `src/test/CMakeFile.txt`? A tl&dr of https://github.com/bitcoin/bitcoin/pull/30510/files#r1747656292
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1751353778)
988dacead4a9a6850b767a8ced0c08b47fece56d: can you add a comment for why this isn't in `src/test/CMakeFile.txt`? A tl&dr of https://github.com/bitcoin/bitcoin/pull/30510/files#r1747656292
💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1751359880)
On Intel macOS 14.6.1 this doesn't work for me.
```
git clean -dfx
cmake -B build -DWITH_MULTIPROCESS=true
...
-- Configuring done (29.3s)
CMake Error at src/CMakeLists.txt:335 (add_dependencies):
The dependency target "bitcoin_ipc_headers" of target "bitcoin_ipc_test"
does not exist.
```
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1751359880)
On Intel macOS 14.6.1 this doesn't work for me.
```
git clean -dfx
cmake -B build -DWITH_MULTIPROCESS=true
...
-- Configuring done (29.3s)
CMake Error at src/CMakeLists.txt:335 (add_dependencies):
The dependency target "bitcoin_ipc_headers" of target "bitcoin_ipc_test"
does not exist.
```
💬 willcl-ark commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2339988301)
> Hmm, you could also try to re-run the tasks individually to see the best possible performance (which is what was tested above as well).
Correct, I used default `cirrus-cli` concurrency (of 1) on the server during testing. Some of these were second runs but ccache was not hit in all of them. E.g. it hit 0% in `depends, debug`, but 100% in `nowallet, libbitcoinkernel`. The run is here: https://cirrus-ci.com/build/6496417768800256
IMO that @achow101 didn't see a great speedup further hints
...
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2339988301)
> Hmm, you could also try to re-run the tasks individually to see the best possible performance (which is what was tested above as well).
Correct, I used default `cirrus-cli` concurrency (of 1) on the server during testing. Some of these were second runs but ccache was not hit in all of them. E.g. it hit 0% in `depends, debug`, but 100% in `nowallet, libbitcoinkernel`. The run is here: https://cirrus-ci.com/build/6496417768800256
IMO that @achow101 didn't see a great speedup further hints
...
📝 BrandonOdiwuor opened a pull request: "test: autogenerate bash completion"
(https://github.com/bitcoin/bitcoin/pull/30860)
Fixes https://github.com/bitcoin/bitcoin/issues/17289, and follows up on https://github.com/bitcoin/bitcoin/pull/18606
Adds a functional test that parses available RPC commands, generates the associated bitcoin-cli autocomplete file and checks that the current autocomplete file matches.
An outdated autocomplete file can be updated via the --overwrite test parameter.
(https://github.com/bitcoin/bitcoin/pull/30860)
Fixes https://github.com/bitcoin/bitcoin/issues/17289, and follows up on https://github.com/bitcoin/bitcoin/pull/18606
Adds a functional test that parses available RPC commands, generates the associated bitcoin-cli autocomplete file and checks that the current autocomplete file matches.
An outdated autocomplete file can be updated via the --overwrite test parameter.
💬 willcl-ark commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2340006739)
Another potential (and cheap) avenue we could explore is having a single massive shared ccache dir. This of course wouldn't speed up running tests themselves, but we could try for even more cache hits during compilation.
Hetzner has a "storage box" https://www.hetzner.com/storage/storage-box/ which, for 4€ per month gets unlimited internal traffic and 1TB of disk space. I don't much like the look of "10 concurrent connections" max, and unsure if it's SSD or spinning disk, but supposing the be
...
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2340006739)
Another potential (and cheap) avenue we could explore is having a single massive shared ccache dir. This of course wouldn't speed up running tests themselves, but we could try for even more cache hits during compilation.
Hetzner has a "storage box" https://www.hetzner.com/storage/storage-box/ which, for 4€ per month gets unlimited internal traffic and 1TB of disk space. I don't much like the look of "10 concurrent connections" max, and unsure if it's SSD or spinning disk, but supposing the be
...
💬 hebasto commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340054600)
Shouldn't we check for the absence of unfortified versions of functions, rather than checking for the presence of fortified ones?
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340054600)
Shouldn't we check for the absence of unfortified versions of functions, rather than checking for the presence of fortified ones?
💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340063318)
> Shouldn't we check for the absence of unfortified versions of functions, rather than checking for the presence of fortified ones?
Can you explain how that test would work? Not all functions are gauranteed to be fortified.
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340063318)
> Shouldn't we check for the absence of unfortified versions of functions, rather than checking for the presence of fortified ones?
Can you explain how that test would work? Not all functions are gauranteed to be fortified.
💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#discussion_r1751550882)
Yea, it's a bit annoying. At least post-BDB, it'll just be "ignore the stack protector".
(https://github.com/bitcoin/bitcoin/pull/27038#discussion_r1751550882)
Yea, it's a bit annoying. At least post-BDB, it'll just be "ignore the stack protector".
💬 hebasto commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340081949)
> > Shouldn't we check for the absence of unfortified versions of functions, rather than checking for the presence of fortified ones?
>
> Can you explain how that test would work? Not all functions are guaranteed to be fortified.
1. Get the list of fortified functions from all binaries.
2. Create a list of the corresponding unfortified functions based on the list from step 1.
3. Check for the absence of symbols from the list in step 2.
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340081949)
> > Shouldn't we check for the absence of unfortified versions of functions, rather than checking for the presence of fortified ones?
>
> Can you explain how that test would work? Not all functions are guaranteed to be fortified.
1. Get the list of fortified functions from all binaries.
2. Create a list of the corresponding unfortified functions based on the list from step 1.
3. Check for the absence of symbols from the list in step 2.
💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340094083)
> Get the list of fortified functions from all binaries.
This assumes that fortification is already working correctly, otherwise you'll miss any (relevant) function that hasn't be fortified at least one time.
Again, it's not a bug to have unfortified functions. So I'm not sure what you are trying to acheive by turning that into a check failure. Can you explain further.
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340094083)
> Get the list of fortified functions from all binaries.
This assumes that fortification is already working correctly, otherwise you'll miss any (relevant) function that hasn't be fortified at least one time.
Again, it's not a bug to have unfortified functions. So I'm not sure what you are trying to acheive by turning that into a check failure. Can you explain further.
💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2340096700)
I am thinking it would be less hassle to set up local storage. Ensuring that a given task type only runs on a given machine should be enough to ensure a "single ccache dir". (ccache results can't be shared between tasks anyway)
This would also avoid additional work or downtime when something goes wrong in the remote "storage box" (connection management, maintenance, configuration, ...)
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2340096700)
I am thinking it would be less hassle to set up local storage. Ensuring that a given task type only runs on a given machine should be enough to ensure a "single ccache dir". (ccache results can't be shared between tasks anyway)
This would also avoid additional work or downtime when something goes wrong in the remote "storage box" (connection management, maintenance, configuration, ...)
💬 hebasto commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340112965)
> Again, it's not a bug to have unfortified functions.
Then how it can be classified?
> So I'm not sure what you are trying to acheive by turning that into a check failure.
Fortified functions can be statically linked into executables from a toolchain that was built with fortification enabled, while the rest of the code remains unfortified. In that case, the current PR branch will report a false positive result, won't it?
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340112965)
> Again, it's not a bug to have unfortified functions.
Then how it can be classified?
> So I'm not sure what you are trying to acheive by turning that into a check failure.
Fortified functions can be statically linked into executables from a toolchain that was built with fortification enabled, while the rest of the code remains unfortified. In that case, the current PR branch will report a false positive result, won't it?
💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340122396)
> Then how it can be classified?
As expected behaviour, given that whether fortification occurs is dependant on various (compiler & libc) heuristics.
> Fortified functions can be statically linked into executables from a toolchain that was built with fortification enabled, while the rest of the code remains unfortified. In that case, the current PR branch will report a false positive result, won't it?
I don't really understand what you mean. Can you give a specific example of a false po
...
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340122396)
> Then how it can be classified?
As expected behaviour, given that whether fortification occurs is dependant on various (compiler & libc) heuristics.
> Fortified functions can be statically linked into executables from a toolchain that was built with fortification enabled, while the rest of the code remains unfortified. In that case, the current PR branch will report a false positive result, won't it?
I don't really understand what you mean. Can you give a specific example of a false po
...