💬 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
...
💬 maflcko commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1751591719)
style nit for `LogPrintf("Error` here and below:
For new code, it would be good to use a non-deprecated log function (like `LogInfo`, `LogWarning`, or `LogError`). Otherwise, there may be confusion whether the `info` severity was picked on purpose or if something more suitable should be used instead.
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1751591719)
style nit for `LogPrintf("Error` here and below:
For new code, it would be good to use a non-deprecated log function (like `LogInfo`, `LogWarning`, or `LogError`). Otherwise, there may be confusion whether the `info` severity was picked on purpose or if something more suitable should be used instead.
💬 maflcko commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2340166976)
Apart from the benchmark, it may also be useful to create a flamegraph/heatmap, so that it is easy to see where the time is spent.
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2340166976)
Apart from the benchmark, it may also be useful to create a flamegraph/heatmap, so that it is easy to see where the time is spent.
💬 hebasto commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340179006)
> Can you give a specific example of a false positive/issue, in the context of our Guix environment/build.
Sure. This [branch](https://github.com/hebasto/bitcoin/commits/pr27038/0910-DEMO.2/) clearly demonstrates false positive results for `bitcoind` and `bitcoin-qt`:
```
b07d7f1b7e5f5eaf8649685e7f8e031e4ac078f87dbd27e0d732c2a578ef3c4c guix-build-423fc912bca9/output/dist-archive/bitcoin-423fc912bca9.tar.gz
e7cce3c0bdf87e4583067fdf39aea50fb4c12fb0d52cbb78e3c7c0c967a9b215 guix-build-423fc9
...
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340179006)
> Can you give a specific example of a false positive/issue, in the context of our Guix environment/build.
Sure. This [branch](https://github.com/hebasto/bitcoin/commits/pr27038/0910-DEMO.2/) clearly demonstrates false positive results for `bitcoind` and `bitcoin-qt`:
```
b07d7f1b7e5f5eaf8649685e7f8e031e4ac078f87dbd27e0d732c2a578ef3c4c guix-build-423fc912bca9/output/dist-archive/bitcoin-423fc912bca9.tar.gz
e7cce3c0bdf87e4583067fdf39aea50fb4c12fb0d52cbb78e3c7c0c967a9b215 guix-build-423fc9
...
💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340212685)
> Sure. This branch clearly demonstrates false positive results for bitcoind and bitcoin-qt:
Thanks. I built this branch and inspected `bitcoind`, and it contains calls to fortified functions. i.e:
```bash
objdump -D /root/bitcoin/guix-build-423fc912bca9/output/x86_64-linux-gnu/bitcoin-423fc912bca9/bin/bitcoind
<snip>
6f9afe: e8 cd dd 95 ff call 578d0 <__vsnprintf_chk@plt>
6fd0a1: e8 ba b1 95 ff call 58260 <__fprintf_chk@plt>
6fe135: e8 66 a0 95 ff call
...
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340212685)
> Sure. This branch clearly demonstrates false positive results for bitcoind and bitcoin-qt:
Thanks. I built this branch and inspected `bitcoind`, and it contains calls to fortified functions. i.e:
```bash
objdump -D /root/bitcoin/guix-build-423fc912bca9/output/x86_64-linux-gnu/bitcoin-423fc912bca9/bin/bitcoind
<snip>
6f9afe: e8 cd dd 95 ff call 578d0 <__vsnprintf_chk@plt>
6fd0a1: e8 ba b1 95 ff call 58260 <__fprintf_chk@plt>
6fe135: e8 66 a0 95 ff call
...
💬 hebasto commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340221792)
> So how is this a false positive?
This [commit](https://github.com/hebasto/bitcoin/commit/a4b627a27867b274cc7541336e0995ffad58c632) deletes the source fortification logic from the build system altogether. Nevertheless, the check passes.
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340221792)
> So how is this a false positive?
This [commit](https://github.com/hebasto/bitcoin/commit/a4b627a27867b274cc7541336e0995ffad58c632) deletes the source fortification logic from the build system altogether. Nevertheless, the check passes.
💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1751658436)
Got the error again, trying to create a godbolt reproducer to understand the apparent pointer arithmetic misunderstanding
<details>
<summary>Error</summary>
```
In file included from /ci_container_base/src/script/script.h:11,
from /ci_container_base/src/primitives/transaction.h:11,
from /ci_container_base/src/primitives/block.h:9,
from /ci_container_base/src/kernel/chainparams.h:11,
from /ci_container_base/src/kernel
...
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1751658436)
Got the error again, trying to create a godbolt reproducer to understand the apparent pointer arithmetic misunderstanding
<details>
<summary>Error</summary>
```
In file included from /ci_container_base/src/script/script.h:11,
from /ci_container_base/src/primitives/transaction.h:11,
from /ci_container_base/src/primitives/block.h:9,
from /ci_container_base/src/kernel/chainparams.h:11,
from /ci_container_base/src/kernel
...
💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340225004)
> This [commit](https://github.com/hebasto/bitcoin/commit/a4b627a27867b274cc7541336e0995ffad58c632) deletes the source fortification logic from the build system altogether. Nevertheless, the check passes.
Yes, the check as implemented, which checks for (any) usage of foritfied function calls in the binary, passes, because the binary contains calls to fortified functions.
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2340225004)
> This [commit](https://github.com/hebasto/bitcoin/commit/a4b627a27867b274cc7541336e0995ffad58c632) deletes the source fortification logic from the build system altogether. Nevertheless, the check passes.
Yes, the check as implemented, which checks for (any) usage of foritfied function calls in the binary, passes, because the binary contains calls to fortified functions.
💬 dergoegge commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1751663364)
Reusing the same data from the fuzz input for two different things (e.g. decode/encode and encode/decode) is usually not a good idea.
How about adding a decode step after the encoding above (i.e. decode -> encode -> decode)? Or alternatively split this second round-trip into its own harness as well?
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1751663364)
Reusing the same data from the fuzz input for two different things (e.g. decode/encode and encode/decode) is usually not a good idea.
How about adding a decode step after the encoding above (i.e. decode -> encode -> decode)? Or alternatively split this second round-trip into its own harness as well?