💬 hebasto commented on issue "callgrind_annotate broken after cmake migration?":
(https://github.com/bitcoin/bitcoin/issues/31957#issuecomment-3567810672)
> Maybe the prefix-map can be disabled by default or an option could be added to disable it, or it could be disabled on `-DWITH_CCACHE=NO`, as there is no need for it then?
https://github.com/bitcoin/bitcoin/commit/788c1324f3d840f7a39b8bc3537dcff26ca0b552 could be considered for reverting. However, doing so would resurface https://github.com/bitcoin/bitcoin/issues/30799.
(https://github.com/bitcoin/bitcoin/issues/31957#issuecomment-3567810672)
> Maybe the prefix-map can be disabled by default or an option could be added to disable it, or it could be disabled on `-DWITH_CCACHE=NO`, as there is no need for it then?
https://github.com/bitcoin/bitcoin/commit/788c1324f3d840f7a39b8bc3537dcff26ca0b552 could be considered for reverting. However, doing so would resurface https://github.com/bitcoin/bitcoin/issues/30799.
💬 hebasto commented on issue "callgrind_annotate broken after cmake migration?":
(https://github.com/bitcoin/bitcoin/issues/31957#issuecomment-3567833333)
> > Maybe the prefix-map can be disabled by default or an option could be added to disable it, or it could be disabled on `-DWITH_CCACHE=NO`, as there is no need for it then?
>
> [788c132](https://github.com/bitcoin/bitcoin/commit/788c1324f3d840f7a39b8bc3537dcff26ca0b552) could be considered for reverting. However, doing so would resurface [#30799](https://github.com/bitcoin/bitcoin/issues/30799).
And the latter could be addressed using another approach:
```diff
--- a/src/logging.cpp
+++ b/src
...
(https://github.com/bitcoin/bitcoin/issues/31957#issuecomment-3567833333)
> > Maybe the prefix-map can be disabled by default or an option could be added to disable it, or it could be disabled on `-DWITH_CCACHE=NO`, as there is no need for it then?
>
> [788c132](https://github.com/bitcoin/bitcoin/commit/788c1324f3d840f7a39b8bc3537dcff26ca0b552) could be considered for reverting. However, doing so would resurface [#30799](https://github.com/bitcoin/bitcoin/issues/30799).
And the latter could be addressed using another approach:
```diff
--- a/src/logging.cpp
+++ b/src
...
💬 hebasto commented on issue "Source code mapping for debugger has changed since cmake":
(https://github.com/bitcoin/bitcoin/issues/31204#issuecomment-3567837050)
@pinheadmz
Does the approach proposed in both https://github.com/bitcoin/bitcoin/issues/31957#issuecomment-3567810672 and https://github.com/bitcoin/bitcoin/issues/31957#issuecomment-3567833333 make any difference to this issue?
(https://github.com/bitcoin/bitcoin/issues/31204#issuecomment-3567837050)
@pinheadmz
Does the approach proposed in both https://github.com/bitcoin/bitcoin/issues/31957#issuecomment-3567810672 and https://github.com/bitcoin/bitcoin/issues/31957#issuecomment-3567833333 make any difference to this issue?
💬 hebasto commented on pull request "build: Set AUTHOR_WARNING on warnings":
(https://github.com/bitcoin/bitcoin/pull/33144#issuecomment-3567848817)
cc @purpleKarrot
(https://github.com/bitcoin/bitcoin/pull/33144#issuecomment-3567848817)
cc @purpleKarrot
✅ l0rinc closed an issue: "malloc: Failed to allocate segment from range group - out of space"
(https://github.com/bitcoin/bitcoin/issues/33806)
(https://github.com/bitcoin/bitcoin/issues/33806)
💬 l0rinc commented on issue "malloc: Failed to allocate segment from range group - out of space":
(https://github.com/bitcoin/bitcoin/issues/33806#issuecomment-3567870300)
No idea what changed since, but I cannot reproduce it with `master` or with `27.2`.
Has anyone else tried it? Closing for now, we can reopen if it reproduces later.
(https://github.com/bitcoin/bitcoin/issues/33806#issuecomment-3567870300)
No idea what changed since, but I cannot reproduce it with `master` or with `27.2`.
Has anyone else tried it? Closing for now, we can reopen if it reproduces later.
💬 hebasto commented on pull request "cmake: fatal error when PIE not supported":
(https://github.com/bitcoin/bitcoin/pull/33282#issuecomment-3567921587)
> Where is it defined or documented why `CMAKE_POSITION_INDEPENDENT_CODE` is set to `ON` in the first place?
>
> A better approach would be to not override user preferences at all and set the `POSITION_INDEPENDENT_CODE` target property explicitly on targets that need it.
Using only PIC objects by default was introduced in https://github.com/bitcoin/bitcoin/commit/19df238a7ba7a6cdfbf48bc663c39ddbf9f6c7d0.
(https://github.com/bitcoin/bitcoin/pull/33282#issuecomment-3567921587)
> Where is it defined or documented why `CMAKE_POSITION_INDEPENDENT_CODE` is set to `ON` in the first place?
>
> A better approach would be to not override user preferences at all and set the `POSITION_INDEPENDENT_CODE` target property explicitly on targets that need it.
Using only PIC objects by default was introduced in https://github.com/bitcoin/bitcoin/commit/19df238a7ba7a6cdfbf48bc663c39ddbf9f6c7d0.
💬 TheCharlatan commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2554084325)
We should include post-condition checks here. For the `TestBlockValidity` refactor, we added
```
if (state.IsValid()) NONFATAL_UNREACHABLE();
```
I think such a check should be added wherever a boolean was returned previously.
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2554084325)
We should include post-condition checks here. For the `TestBlockValidity` refactor, we added
```
if (state.IsValid()) NONFATAL_UNREACHABLE();
```
I think such a check should be added wherever a boolean was returned previously.
💬 TheCharlatan commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2554092568)
I don't think this change is correct. Previously false was returned when the block failed to be accepted or on a fatal error condition, not when it failed to validate. If you look into `ActivateBestChainStep`, you'll see that we break on a validation failure, reset the state again and don't return false. Can you drop this change again?
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2554092568)
I don't think this change is correct. Previously false was returned when the block failed to be accepted or on a fatal error condition, not when it failed to validate. If you look into `ActivateBestChainStep`, you'll see that we break on a validation failure, reset the state again and don't return false. Can you drop this change again?
💬 hebasto commented on pull request "cmake: fatal error when PIE not supported":
(https://github.com/bitcoin/bitcoin/pull/33282#issuecomment-3568067474)
> This introduces two behaviour changes:
>
> 1. a user-defined `CMAKE_POSITION_INDEPENDENT_CODE=OFF` will now be respected by our build system, instead of silently overriding it like before
Concept ACK on that.
(https://github.com/bitcoin/bitcoin/pull/33282#issuecomment-3568067474)
> This introduces two behaviour changes:
>
> 1. a user-defined `CMAKE_POSITION_INDEPENDENT_CODE=OFF` will now be respected by our build system, instead of silently overriding it like before
Concept ACK on that.
💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-3568091456)
Rebased on top of #33629.
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-3568091456)
Rebased on top of #33629.
👋 andrewtoth's pull request is ready for review: "validation: fetch block inputs on parallel threads >20% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/31132)
(https://github.com/bitcoin/bitcoin/pull/31132)
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3568164009)
I've updated the PR to make the `InputFetcher` a subclass of `CCoinsViewCache`. Instead of waiting on the MPSC queue to be finished before connecting the block, the queue can be processed during `CCoinsViewCache::FetchCoin` during `ConnectBlock`. This makes the fetching completely non-blocking, which is a significant performance improvement. It's been renamed to `CoinsViewCacheAsync`.
@l0rinc thank you for your very thorough benchmarks. I've updated this to use 4 worker threads, which yields
...
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3568164009)
I've updated the PR to make the `InputFetcher` a subclass of `CCoinsViewCache`. Instead of waiting on the MPSC queue to be finished before connecting the block, the queue can be processed during `CCoinsViewCache::FetchCoin` during `ConnectBlock`. This makes the fetching completely non-blocking, which is a significant performance improvement. It's been renamed to `CoinsViewCacheAsync`.
@l0rinc thank you for your very thorough benchmarks. I've updated this to use 4 worker threads, which yields
...
💬 151henry151 commented on pull request "Defer transaction signing until user clicks Send":
(https://github.com/bitcoin-core/gui/pull/915#issuecomment-3568201828)
It looks like the one failing check is a CI issue with disk space too low. I'm not sure how to address that. I think the code of the PR is correct.
(https://github.com/bitcoin-core/gui/pull/915#issuecomment-3568201828)
It looks like the one failing check is a CI issue with disk space too low. I'm not sure how to address that. I think the code of the PR is correct.
💬 151henry151 commented on pull request "Align legacy script policy with P2SH policy in AreInputsStandard":
(https://github.com/bitcoin/bitcoin/pull/33926#issuecomment-3568224698)
I updated the tests to match the new policy. The NONSTANDARD script in p2p_segwit.py now passes AreInputsStandard since it has zero sigops, so I kept it as-is. For test_segwit_versions, I switched the input from the NONSTANDARD UTXO to the standard UTXOs created earlier, so the transaction now passes all checks and gets accepted. For feature_taproot.py, I adjusted the is_standard flag for the compat/nocsa spender because bare NONSTANDARD scripts with low sigop counts now pass policy checks.
...
(https://github.com/bitcoin/bitcoin/pull/33926#issuecomment-3568224698)
I updated the tests to match the new policy. The NONSTANDARD script in p2p_segwit.py now passes AreInputsStandard since it has zero sigops, so I kept it as-is. For test_segwit_versions, I switched the input from the NONSTANDARD UTXO to the standard UTXOs created earlier, so the transaction now passes all checks and gets accepted. For feature_taproot.py, I adjusted the is_standard flag for the compat/nocsa spender because bare NONSTANDARD scripts with low sigop counts now pass policy checks.
...
💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#issuecomment-3568292879)
reACK 17cf9ff7efdbab07644fc2f9017fcac1b0757c38
Just the `POST_CHANGE_WORK` change. I ran with the changes manually a bit and logged when things were non-optimal as a sanity check (it never happened). As a follow-up, may be nice to have non-optimality logged in some sensible way for diagnostics.
`git range-diff master 3fb3c230c93e39706b813f67006ae86c9e34e803 17cf9ff7efdbab07644fc2f9017fcac1b0757c38`
(https://github.com/bitcoin/bitcoin/pull/33629#issuecomment-3568292879)
reACK 17cf9ff7efdbab07644fc2f9017fcac1b0757c38
Just the `POST_CHANGE_WORK` change. I ran with the changes manually a bit and logged when things were non-optimal as a sanity check (it never happened). As a follow-up, may be nice to have non-optimality logged in some sensible way for diagnostics.
`git range-diff master 3fb3c230c93e39706b813f67006ae86c9e34e803 17cf9ff7efdbab07644fc2f9017fcac1b0757c38`
🤔 furszy reviewed a pull request: "index: Deduplicate HashKey / HeightKey handling"
(https://github.com/bitcoin/bitcoin/pull/32997#pullrequestreview-3498048964)
what do you think about wrapping all the code in `db_key.h` under a namespace like "util", "common", "index::util", or "util::index" (just threw some names), so it's clear when reading the code that these functions are defined elsewhere and shared, without needing to search around for the definition.
(https://github.com/bitcoin/bitcoin/pull/32997#pullrequestreview-3498048964)
what do you think about wrapping all the code in `db_key.h` under a namespace like "util", "common", "index::util", or "util::index" (just threw some names), so it's clear when reading the code that these functions are defined elsewhere and shared, without needing to search around for the definition.
📝 hebasto opened a pull request: "test: Remove `system_tests/run_command` runtime dependencies"
(https://github.com/bitcoin/bitcoin/pull/33929)
`system_tests` currently rely on the presence of `cat`, `echo`, `false` and `sh` in `PATH` at runtime.
This PR:
1. Removes these dependencies.
2. Eliminates all but one platform-specific code paths.
(https://github.com/bitcoin/bitcoin/pull/33929)
`system_tests` currently rely on the presence of `cat`, `echo`, `false` and `sh` in `PATH` at runtime.
This PR:
1. Removes these dependencies.
2. Eliminates all but one platform-specific code paths.
⚠️ taoteh1221 opened an issue: "Documentation on 'services' parsing in RPC response for getnodeaddresses"
(https://github.com/bitcoin/bitcoin/issues/33930)
### Please describe the feature you'd like to see added.
I can't find any docs on how you parse `services` results for `getnodeaddresses` RPC data requests:
https://bitcoincore.org/en/doc/30.0.0/rpc/network/getnodeaddresses/
I'm hoping to have a more user friendly interface, other than the raw services number for a geolocation search filter:
<img width="1836" height="1206" alt="Image" src="https://github.com/user-attachments/assets/4b845c48-10b4-4098-86c2-fcf22ce49cff" />
### Is your featu
...
(https://github.com/bitcoin/bitcoin/issues/33930)
### Please describe the feature you'd like to see added.
I can't find any docs on how you parse `services` results for `getnodeaddresses` RPC data requests:
https://bitcoincore.org/en/doc/30.0.0/rpc/network/getnodeaddresses/
I'm hoping to have a more user friendly interface, other than the raw services number for a geolocation search filter:
<img width="1836" height="1206" alt="Image" src="https://github.com/user-attachments/assets/4b845c48-10b4-4098-86c2-fcf22ce49cff" />
### Is your featu
...
💬 pinheadmz commented on issue "Documentation on 'services' parsing in RPC response for getnodeaddresses":
(https://github.com/bitcoin/bitcoin/issues/33930#issuecomment-3568571196)
It's a bitfield, defined here:
https://github.com/bitcoin/bitcoin/blob/0690514d4f72aac251ee0b876cded9187d42c63e/src/protocol.h#L308-L339
You're right that it's not super well documented but you can find some information:
https://en.bitcoin.it/wiki/Protocol_documentation#version
https://bitcoin.stackexchange.com/search?q=service+bits
(https://github.com/bitcoin/bitcoin/issues/33930#issuecomment-3568571196)
It's a bitfield, defined here:
https://github.com/bitcoin/bitcoin/blob/0690514d4f72aac251ee0b876cded9187d42c63e/src/protocol.h#L308-L339
You're right that it's not super well documented but you can find some information:
https://en.bitcoin.it/wiki/Protocol_documentation#version
https://bitcoin.stackexchange.com/search?q=service+bits