💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2741259247)
`72ff6d2b50...696b6671da`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2741259247)
`72ff6d2b50...696b6671da`: rebase due to conflicts
🤔 BrandonOdiwuor reviewed a pull request: "test: avoid treating hash results as integers"
(https://github.com/bitcoin/bitcoin/pull/32050#pullrequestreview-2703676924)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32050#pullrequestreview-2703676924)
Concept ACK
💬 sr-gi commented on pull request "refactor: Enforces Txid and Wtxid types in RelayTransaction":
(https://github.com/bitcoin/bitcoin/pull/32104#discussion_r2006210663)
I've updated the approach to enforce RelayTransaction types, and extended it to all other calls to the method
(https://github.com/bitcoin/bitcoin/pull/32104#discussion_r2006210663)
I've updated the approach to enforce RelayTransaction types, and extended it to all other calls to the method
💬 yancyribbens commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2006228794)
I tested however the benchmarks don't seem to be sensitive enough for this to make a difference.
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2006228794)
I tested however the benchmarks don't seem to be sensitive enough for this to make a difference.
🤔 Jassor909 reviewed a pull request: "refactor: Enforces Txid and Wtxid types in RelayTransaction"
(https://github.com/bitcoin/bitcoin/pull/32104#pullrequestreview-2703737664)
#
(https://github.com/bitcoin/bitcoin/pull/32104#pullrequestreview-2703737664)
#
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2006247029)
... or a poor design of Qt's internal find modules.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2006247029)
... or a poor design of Qt's internal find modules.
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2741372636)
My Guix build:
```
aarch64
8948298e720d0d9077a1f57c6e44fff86a234830547f5d57278d37c672236541 guix-build-7b51bf1d4c19/output/aarch64-linux-gnu/SHA256SUMS.part
43cec316ed0a89274e9b805b845e5be6b164b9ba5b58750fee470335ed4f7904 guix-build-7b51bf1d4c19/output/aarch64-linux-gnu/bitcoin-7b51bf1d4c19-aarch64-linux-gnu-debug.tar.gz
023042d946d06d6a89e5a611bd5eb9e73680c541e676e4b4c0c28ea780e9fc71 guix-build-7b51bf1d4c19/output/aarch64-linux-gnu/bitcoin-7b51bf1d4c19-aarch64-linux-gnu.tar.gz
513e04e1
...
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2741372636)
My Guix build:
```
aarch64
8948298e720d0d9077a1f57c6e44fff86a234830547f5d57278d37c672236541 guix-build-7b51bf1d4c19/output/aarch64-linux-gnu/SHA256SUMS.part
43cec316ed0a89274e9b805b845e5be6b164b9ba5b58750fee470335ed4f7904 guix-build-7b51bf1d4c19/output/aarch64-linux-gnu/bitcoin-7b51bf1d4c19-aarch64-linux-gnu-debug.tar.gz
023042d946d06d6a89e5a611bd5eb9e73680c541e676e4b4c0c28ea780e9fc71 guix-build-7b51bf1d4c19/output/aarch64-linux-gnu/bitcoin-7b51bf1d4c19-aarch64-linux-gnu.tar.gz
513e04e1
...
💬 Sjors commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2741407835)
re-ACK 418236c106e32abd7357551d309f8e6d1e494f72
Same as my last review, but the two commits shared with #31866 are merged.
Also tested (ad hoc signed) guix builds on macOS (M4 with 15.3.2, Intel with 13.7.4).
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2741407835)
re-ACK 418236c106e32abd7357551d309f8e6d1e494f72
Same as my last review, but the two commits shared with #31866 are merged.
Also tested (ad hoc signed) guix builds on macOS (M4 with 15.3.2, Intel with 13.7.4).
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2006272875)
The [latest force-push](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_29..kernelApi_30) broke this logic by leaving `m_chainparams` and `m_notifications` uninitialized if `options` is non-nullptr, but the respective options members are nullptr.
Suggested fix:
<details>
<summary>git diff on 2dc27e2860</summary>
```diff
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index 0cb2d69cec..1e6c582357 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2006272875)
The [latest force-push](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_29..kernelApi_30) broke this logic by leaving `m_chainparams` and `m_notifications` uninitialized if `options` is non-nullptr, but the respective options members are nullptr.
Suggested fix:
<details>
<summary>git diff on 2dc27e2860</summary>
```diff
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index 0cb2d69cec..1e6c582357 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/
...
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2000864918)
typo nit (+ in 2 other log functions)
```suggestion
* all existing `kernel_LoggingConnection instances.
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2000864918)
typo nit (+ in 2 other log functions)
```suggestion
* all existing `kernel_LoggingConnection instances.
```
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2006290487)
What is the rationale behind requiring to first add, and then {enable,disable} the category? An alternative would be a single `kernel_set_log_level_category`, which immediately "enables" (I think it's a strange term anyway) the category at the given level. "Disabling" would be achieved by calling `kernel_set_log_level_category` again with a higher (i.e. less granular) level, again taking effect immediately.
I can't think of any use cases that require separating this in 3 functions? I think it
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2006290487)
What is the rationale behind requiring to first add, and then {enable,disable} the category? An alternative would be a single `kernel_set_log_level_category`, which immediately "enables" (I think it's a strange term anyway) the category at the given level. "Disabling" would be achieved by calling `kernel_set_log_level_category` again with a higher (i.e. less granular) level, again taking effect immediately.
I can't think of any use cases that require separating this in 3 functions? I think it
...
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2006309594)
> I don't think we need to rely on the order inside the enumeration here
I don't think it's required, but it is slightly convenient when they are? E.g. when implementing `py-bitcoinkernel`'s logging, being able to rely on the order of `kernel_LogLevel` helps the implementation a bit (and it is also how these level enums are usually implemented in most logging libraries, I think). As one example, I think using the same values as the python `logging` library could be sensible: https://docs.pyth
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2006309594)
> I don't think we need to rely on the order inside the enumeration here
I don't think it's required, but it is slightly convenient when they are? E.g. when implementing `py-bitcoinkernel`'s logging, being able to rely on the order of `kernel_LogLevel` helps the implementation a bit (and it is also how these level enums are usually implemented in most logging libraries, I think). As one example, I think using the same values as the python `logging` library could be sensible: https://docs.pyth
...
💬 willcl-ark commented on issue "b-msghand invoked oom-killer: Master (v28.99) crashing during IBD":
(https://github.com/bitcoin/bitcoin/issues/31561#issuecomment-2741665911)
I may take a stab at trying to reproduce this to see if it's still a problem with master today (unless you fancy doing so).
Just a few questions first:
> Configuration is just -txindex=1
- Their (basic) 8GB droplets appear to have 160 GiB (SSD), so I presume you also had pruning on?
- Your logs show at least `bench` and `validation` debug levels, did you use `-debug=1`, or only these (or only these on a subsequent run to debug a crash)?
- Am I likely to need to sync to ~ `height=814565` to se
...
(https://github.com/bitcoin/bitcoin/issues/31561#issuecomment-2741665911)
I may take a stab at trying to reproduce this to see if it's still a problem with master today (unless you fancy doing so).
Just a few questions first:
> Configuration is just -txindex=1
- Their (basic) 8GB droplets appear to have 160 GiB (SSD), so I presume you also had pruning on?
- Your logs show at least `bench` and `validation` debug levels, did you use `-debug=1`, or only these (or only these on a subsequent run to debug a crash)?
- Am I likely to need to sync to ~ `height=814565` to se
...
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2006471733)
Thanks! I also added a regression test. Sorry for not catching this earlier!
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2006471733)
Thanks! I also added a regression test. Sorry for not catching this earlier!
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2741688322)
Updated 2dc27e2860b97c2bffa5f18706917b21858e5594 -> 9fc6accf89ed001f70e107a8e9936f6dc3a35f41 ([kernelApi_30](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_30) -> [kernelApi_31](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_31), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_30..kernelApi_31))
* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2000864918), fixed naming in docstring for `kernel_LoggingConnection`.
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2741688322)
Updated 2dc27e2860b97c2bffa5f18706917b21858e5594 -> 9fc6accf89ed001f70e107a8e9936f6dc3a35f41 ([kernelApi_30](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_30) -> [kernelApi_31](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_31), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_30..kernelApi_31))
* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2000864918), fixed naming in docstring for `kernel_LoggingConnection`.
...
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2006477122)
This should just mirror the internal code at the moment, but I agree that it is not really useful to split this up. Will see if I can consolidate this.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2006477122)
This should just mirror the internal code at the moment, but I agree that it is not really useful to split this up. Will see if I can consolidate this.
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2006504759)
It seems there is an issue with older CMake versions. I was able to reproduce it on Ubuntu `x86_64` while compiling natively:
```
$ cmake --version
cmake version 3.22.6
CMake suite maintained and supported by Kitware (kitware.com/cmake).
$ gmake -C depends -j 16 qt
<snip>
Building qt...
ninja: error: 'qtbase/src/platformsupport/input/CMakeFiles/InputSupportPrivate.dir/cmake_pch.hxx.gch', needed by 'qtbase/src/platformsupport/input/CMakeFiles/InputSupportPrivate.dir/InputSupportPrivate_
...
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2006504759)
It seems there is an issue with older CMake versions. I was able to reproduce it on Ubuntu `x86_64` while compiling natively:
```
$ cmake --version
cmake version 3.22.6
CMake suite maintained and supported by Kitware (kitware.com/cmake).
$ gmake -C depends -j 16 qt
<snip>
Building qt...
ninja: error: 'qtbase/src/platformsupport/input/CMakeFiles/InputSupportPrivate.dir/cmake_pch.hxx.gch', needed by 'qtbase/src/platformsupport/input/CMakeFiles/InputSupportPrivate.dir/InputSupportPrivate_
...
⚠️ yellowred opened an issue: "Add a script for Signet or Regtest to unlock fee estimator"
(https://github.com/bitcoin/bitcoin/issues/32105)
### Please describe the feature you'd like to see added.
Currently, the fee estimator won't work in a private signet or regtest network, because there are usually not enough transactions being added. This forces clients to hardcode a default fee or have some custom logic for these networks.
The proposal is to add a script to /contrib/signet that would simulate a transactions volume just enough to allow `estimatesmartfee` to work.
It would be useful to have it in the bitcoin core repo, because:
...
(https://github.com/bitcoin/bitcoin/issues/32105)
### Please describe the feature you'd like to see added.
Currently, the fee estimator won't work in a private signet or regtest network, because there are usually not enough transactions being added. This forces clients to hardcode a default fee or have some custom logic for these networks.
The proposal is to add a script to /contrib/signet that would simulate a transactions volume just enough to allow `estimatesmartfee` to work.
It would be useful to have it in the bitcoin core repo, because:
...
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2741785465)
Rebased 9fc6accf89ed001f70e107a8e9936f6dc3a35f41 -> 29f05b91cf8a479e403b0322afeb5ff1133da221 ([kernelApi_31](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_31) -> [kernelApi_32](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_32), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_31..kernelApi_32))
* Fixed silent merge conflict with #31519
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2741785465)
Rebased 9fc6accf89ed001f70e107a8e9936f6dc3a35f41 -> 29f05b91cf8a479e403b0322afeb5ff1133da221 ([kernelApi_31](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_31) -> [kernelApi_32](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_32), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_31..kernelApi_32))
* Fixed silent merge conflict with #31519
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2006536581)
The problem occurs with CMake versions older than 3.25.0.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2006536581)
The problem occurs with CMake versions older than 3.25.0.