💬 hebasto commented on pull request "cmake: Do not require Python to build GUI":
(https://github.com/bitcoin/bitcoin/pull/33156#issuecomment-3196425727)
Closing in favour of https://github.com/bitcoin/bitcoin/pull/33209.
(https://github.com/bitcoin/bitcoin/pull/33156#issuecomment-3196425727)
Closing in favour of https://github.com/bitcoin/bitcoin/pull/33209.
💬 purpleKarrot commented on pull request "cmake: Drop python dependency for translate":
(https://github.com/bitcoin/bitcoin/pull/33209#issuecomment-3196426884)
> CI linter complains:
>
> ```
> [11:39:00.061] File "share/qt/translate.cmake" contains a shebang line, but has the file permission 644 instead of the expected executable permission 755. Do "chmod 755 share/qt/translate.cmake" (or remove the shebang line).
> ```
What would you prefer:
a) Set executable bit, or
b) Remove shebang.
(https://github.com/bitcoin/bitcoin/pull/33209#issuecomment-3196426884)
> CI linter complains:
>
> ```
> [11:39:00.061] File "share/qt/translate.cmake" contains a shebang line, but has the file permission 644 instead of the expected executable permission 755. Do "chmod 755 share/qt/translate.cmake" (or remove the shebang line).
> ```
What would you prefer:
a) Set executable bit, or
b) Remove shebang.
💬 macgyver13 commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3196441400)
@Eunovo Nice work on addressing the issues identified in previous tests. All of those are working as expected now.
I am including a patch that adds 2 more failing tests when adding silent-payments address in the transaction output:
self.test_sendrawtransaction()
self.test_mixed_output_types()
[test_sp_receive.patch](https://github.com/user-attachments/files/21834964/test_sp_receive.patch)
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3196441400)
@Eunovo Nice work on addressing the issues identified in previous tests. All of those are working as expected now.
I am including a patch that adds 2 more failing tests when adding silent-payments address in the transaction output:
self.test_sendrawtransaction()
self.test_mixed_output_types()
[test_sp_receive.patch](https://github.com/user-attachments/files/21834964/test_sp_receive.patch)
💬 hebasto commented on pull request "cmake: Drop python dependency for translate":
(https://github.com/bitcoin/bitcoin/pull/33209#issuecomment-3196449095)
> > CI linter complains:
> > ```
> > [11:39:00.061] File "share/qt/translate.cmake" contains a shebang line, but has the file permission 644 instead of the expected executable permission 755. Do "chmod 755 share/qt/translate.cmake" (or remove the shebang line).
> > ```
>
> What would you prefer:
>
> a) Set executable bit, or
> b) Remove shebang.
(b), for consistency with other CMake scripts in the repository,
(https://github.com/bitcoin/bitcoin/pull/33209#issuecomment-3196449095)
> > CI linter complains:
> > ```
> > [11:39:00.061] File "share/qt/translate.cmake" contains a shebang line, but has the file permission 644 instead of the expected executable permission 755. Do "chmod 755 share/qt/translate.cmake" (or remove the shebang line).
> > ```
>
> What would you prefer:
>
> a) Set executable bit, or
> b) Remove shebang.
(b), for consistency with other CMake scripts in the repository,
📝 brunoerg opened a pull request: "fuzz: enhance wallet_fees by mocking mempool stuff"
(https://github.com/bitcoin/bitcoin/pull/33210)
Some functions in `wallet/fees.cpp` (fuzzed by the wallet_fees target) depends on some mempool stuff - e.g. relay current min fee, smart fee and max blocks estimation, relay dust fee and other ones. For better fuzzing of it, it would be great to have these values/interactions. That said, this PR enhances the `wallet_fees` target by:
- Setting mempool options - `min_relay_feerate`, `dust_relay_feerate` and `incremental_relay_feerate` - when creating the `CTxMemPool`.
- Creates a `ConsumeMemp
...
(https://github.com/bitcoin/bitcoin/pull/33210)
Some functions in `wallet/fees.cpp` (fuzzed by the wallet_fees target) depends on some mempool stuff - e.g. relay current min fee, smart fee and max blocks estimation, relay dust fee and other ones. For better fuzzing of it, it would be great to have these values/interactions. That said, this PR enhances the `wallet_fees` target by:
- Setting mempool options - `min_relay_feerate`, `dust_relay_feerate` and `incremental_relay_feerate` - when creating the `CTxMemPool`.
- Creates a `ConsumeMemp
...
🤔 janb84 reviewed a pull request: "Release: Prepare "Translation string freeze" step"
(https://github.com/bitcoin/bitcoin/pull/33193#pullrequestreview-3128184088)
ACK d32fcd3df22671b633cae605ed06c6d0a353e2bf
PR updates translation strings, for 30.0 freeze.
Ran the commands against this PR; got 0 changes (reproduced the results) ✅
Ran the commands against Master; got the changes as provided in this PR ✅
(https://github.com/bitcoin/bitcoin/pull/33193#pullrequestreview-3128184088)
ACK d32fcd3df22671b633cae605ed06c6d0a353e2bf
PR updates translation strings, for 30.0 freeze.
Ran the commands against this PR; got 0 changes (reproduced the results) ✅
Ran the commands against Master; got the changes as provided in this PR ✅
🤔 hebasto reviewed a pull request: "cmake: Drop python dependency for translate"
(https://github.com/bitcoin/bitcoin/pull/33209#pullrequestreview-3128236494)
It seems no need to track `in_msgstr`:
```diff
--- a/share/qt/translate.cmake
+++ b/share/qt/translate.cmake
@@ -34,23 +34,20 @@ function(extract_strings output)
set(messages)
set(msgid)
set(in_msgid OFF)
- set(in_msgstr OFF)
file(STRINGS "bitcoinstrings.text" text ENCODING "UTF-8")
foreach(line IN LISTS text)
if(line MATCHES "^msgid (.*)$")
- if(in_msgstr AND NOT msgid STREQUAL "\"\"")
+ set(in_msgid ON)
+ set(msgid "${CMAKE_MATCH_1}")
+
...
(https://github.com/bitcoin/bitcoin/pull/33209#pullrequestreview-3128236494)
It seems no need to track `in_msgstr`:
```diff
--- a/share/qt/translate.cmake
+++ b/share/qt/translate.cmake
@@ -34,23 +34,20 @@ function(extract_strings output)
set(messages)
set(msgid)
set(in_msgid OFF)
- set(in_msgstr OFF)
file(STRINGS "bitcoinstrings.text" text ENCODING "UTF-8")
foreach(line IN LISTS text)
if(line MATCHES "^msgid (.*)$")
- if(in_msgstr AND NOT msgid STREQUAL "\"\"")
+ set(in_msgid ON)
+ set(msgid "${CMAKE_MATCH_1}")
+
...
💬 alexanderwiederin commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2282357987)
I think we need to return a `nullptr` here instead of returning a `btck_BlockTreeEntry` wrapper with a null `m_block_index`. The current behavior causes segfaults when callers try to access the returned entry. What do you think?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2282357987)
I think we need to return a `nullptr` here instead of returning a `btck_BlockTreeEntry` wrapper with a null `m_block_index`. The current behavior causes segfaults when callers try to access the returned entry. What do you think?
💬 fanquake commented on pull request "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2":
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3196844620)
Guix Build:
```bash
410caa60435c79c8c862c95acac2e9d29d08ecce8540f8b999dff2c1bac595d6 guix-build-4fd05dadbf23/output/aarch64-linux-gnu/SHA256SUMS.part
177315e236e00d9e8d3d36e2b6fff0ce2d8cfcdd554f35f97547721617782822 guix-build-4fd05dadbf23/output/aarch64-linux-gnu/bitcoin-4fd05dadbf23-aarch64-linux-gnu-debug.tar.gz
a140b60a3e77d6bfcf2ca8a0808f5ca10ef1318bc5480b8401a82727076ac94d guix-build-4fd05dadbf23/output/aarch64-linux-gnu/bitcoin-4fd05dadbf23-aarch64-linux-gnu.tar.gz
ed981094dbe36e3e
...
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3196844620)
Guix Build:
```bash
410caa60435c79c8c862c95acac2e9d29d08ecce8540f8b999dff2c1bac595d6 guix-build-4fd05dadbf23/output/aarch64-linux-gnu/SHA256SUMS.part
177315e236e00d9e8d3d36e2b6fff0ce2d8cfcdd554f35f97547721617782822 guix-build-4fd05dadbf23/output/aarch64-linux-gnu/bitcoin-4fd05dadbf23-aarch64-linux-gnu-debug.tar.gz
a140b60a3e77d6bfcf2ca8a0808f5ca10ef1318bc5480b8401a82727076ac94d guix-build-4fd05dadbf23/output/aarch64-linux-gnu/bitcoin-4fd05dadbf23-aarch64-linux-gnu.tar.gz
ed981094dbe36e3e
...
💬 maflcko commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2282385073)
> I ran some benchmarks comparing `.5s` timeout to no timeout and this commit was always sligthly faster.
The reason why `.5` is picked here is to preserve the observable behavior when `print('.', end='', flush=True)` is hit: During the sleep/wait you'll only want a dot every half second, not in a busy-loop without delay. Using a busy-loop also sounds like a plausible explanation for your observed slow down, when decreasing it to `0`.
Other than that, I don't think the exact value here sho
...
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2282385073)
> I ran some benchmarks comparing `.5s` timeout to no timeout and this commit was always sligthly faster.
The reason why `.5` is picked here is to preserve the observable behavior when `print('.', end='', flush=True)` is hit: During the sleep/wait you'll only want a dot every half second, not in a busy-loop without delay. Using a busy-loop also sounds like a plausible explanation for your observed slow down, when decreasing it to `0`.
Other than that, I don't think the exact value here sho
...
💬 sipa commented on pull request "build: Enable ENABLE_IPC option by default":
(https://github.com/bitcoin/bitcoin/pull/33190#issuecomment-3196869655)
ACK 732c134bfc1208377f449e5956e01dd8b78d73d9. See also tests added in #33201.
I think #31802 should be seen as a successor to this PR, regardless of which release it goes into.
(https://github.com/bitcoin/bitcoin/pull/33190#issuecomment-3196869655)
ACK 732c134bfc1208377f449e5956e01dd8b78d73d9. See also tests added in #33201.
I think #31802 should be seen as a successor to this PR, regardless of which release it goes into.
💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2282400346)
Is the 256-byte value used at all? If not, seems easier to just hardcode a value (like 0 or 1 or 2), or use a deterministic random comtext?
Also, it may be good to document what the goal of this function is. Maybe with an `Assert(mempool.GetMinFee()==min_fee)` or similar?
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2282400346)
Is the 256-byte value used at all? If not, seems easier to just hardcode a value (like 0 or 1 or 2), or use a deterministic random comtext?
Also, it may be good to document what the goal of this function is. Maybe with an `Assert(mempool.GetMinFee()==min_fee)` or similar?
💬 Eunovo commented on pull request "Silent Payments: sending":
(https://github.com/bitcoin/bitcoin/pull/28201#discussion_r2282414301)
`GetPrivKeyForSilentPayment` returns `{}` which is a default initialized object if the coin's keys are not available. `std::holds_alternative<CKey>` will pass for the retuen object and add the default intialized CKey to the `plain_keys` array.
(https://github.com/bitcoin/bitcoin/pull/28201#discussion_r2282414301)
`GetPrivKeyForSilentPayment` returns `{}` which is a default initialized object if the coin's keys are not available. `std::holds_alternative<CKey>` will pass for the retuen object and add the default intialized CKey to the `plain_keys` array.
💬 Eunovo commented on pull request "Silent Payments: sending":
(https://github.com/bitcoin/bitcoin/pull/28201#discussion_r2282367304)
https://github.com/bitcoin/bitcoin/pull/28201/commits/7c1093b08c0363a6d341967659f7b49e0f6d0266: We get a segfault later if the keys are not valid. We should check that keys are valid before adding them to `taproot_keys` or `plain_keys`
(https://github.com/bitcoin/bitcoin/pull/28201#discussion_r2282367304)
https://github.com/bitcoin/bitcoin/pull/28201/commits/7c1093b08c0363a6d341967659f7b49e0f6d0266: We get a segfault later if the keys are not valid. We should check that keys are valid before adding them to `taproot_keys` or `plain_keys`
💬 fanquake commented on pull request "build: Enable ENABLE_IPC option by default":
(https://github.com/bitcoin/bitcoin/pull/33190#issuecomment-3197036114)
> You'll probably want to include https://github.com/bitcoin/bitcoin/commit/71f29d4fa90aaeb6472b3ce9d4f4e97f85ed487b from that PR here to update the build docs.
You'll also need to update the msan fuzz, valgrind and valgrind fuzz CI jobs (run in qa-assets / elsewhere), otherwise they wont work, after this is merged.
(https://github.com/bitcoin/bitcoin/pull/33190#issuecomment-3197036114)
> You'll probably want to include https://github.com/bitcoin/bitcoin/commit/71f29d4fa90aaeb6472b3ce9d4f4e97f85ed487b from that PR here to update the build docs.
You'll also need to update the msan fuzz, valgrind and valgrind fuzz CI jobs (run in qa-assets / elsewhere), otherwise they wont work, after this is merged.
💬 furszy commented on issue "Indexes stuck on unknown best block after unclean shutdown":
(https://github.com/bitcoin/bitcoin/issues/33208#issuecomment-3197067329)
> > Raw idea: I think we can still rewind the indexes even without the block data
>
> How would we reconstruct the `m_muhash` object of the coinstatsindex though? That part of the state is not stored for each block height, just the current one at `DB_MUHASH`.
It seems we are storing it for each block height, see [line 30](https://github.com/bitcoin/bitcoin/blob/d31dc8f8189e93c82dadc01df9a2dd9cfdb50bf9/src/index/coinstatsindex.cpp#L30) and [line 225](https://github.com/bitcoin/bitcoin/blob/d31d
...
(https://github.com/bitcoin/bitcoin/issues/33208#issuecomment-3197067329)
> > Raw idea: I think we can still rewind the indexes even without the block data
>
> How would we reconstruct the `m_muhash` object of the coinstatsindex though? That part of the state is not stored for each block height, just the current one at `DB_MUHASH`.
It seems we are storing it for each block height, see [line 30](https://github.com/bitcoin/bitcoin/blob/d31dc8f8189e93c82dadc01df9a2dd9cfdb50bf9/src/index/coinstatsindex.cpp#L30) and [line 225](https://github.com/bitcoin/bitcoin/blob/d31d
...
💬 fjahr commented on issue "Indexes stuck on unknown best block after unclean shutdown":
(https://github.com/bitcoin/bitcoin/issues/33208#issuecomment-3197122941)
> > > Raw idea: I think we can still rewind the indexes even without the block data
> >
> >
> > How would we reconstruct the `m_muhash` object of the coinstatsindex though? That part of the state is not stored for each block height, just the current one at `DB_MUHASH`.
>
> It seems we are storing it for each block height, see [line 30](https://github.com/bitcoin/bitcoin/blob/d31dc8f8189e93c82dadc01df9a2dd9cfdb50bf9/src/index/coinstatsindex.cpp#L30) and [line 225](https://github.com/bitcoin/bi
...
(https://github.com/bitcoin/bitcoin/issues/33208#issuecomment-3197122941)
> > > Raw idea: I think we can still rewind the indexes even without the block data
> >
> >
> > How would we reconstruct the `m_muhash` object of the coinstatsindex though? That part of the state is not stored for each block height, just the current one at `DB_MUHASH`.
>
> It seems we are storing it for each block height, see [line 30](https://github.com/bitcoin/bitcoin/blob/d31dc8f8189e93c82dadc01df9a2dd9cfdb50bf9/src/index/coinstatsindex.cpp#L30) and [line 225](https://github.com/bitcoin/bi
...
👍 hebasto approved a pull request: "cmake: Drop python dependency for translate"
(https://github.com/bitcoin/bitcoin/pull/33209#pullrequestreview-3128665447)
ACK df07b07b2d8b338b9af60ec8ffb82c20b9901f86.
Another line can be saved:
```diff
--- a/share/qt/translate.cmake
+++ b/share/qt/translate.cmake
@@ -33,7 +33,7 @@ function(extract_strings output)
file(STRINGS "bitcoinstrings.text" text ENCODING "UTF-8")
- set(messages)
+ set(messages "\"${COPYRIGHT_HOLDERS}\"")
foreach(line IN LISTS text)
if(line MATCHES "^msgid (.*)$")
set(msgid "${CMAKE_MATCH_1}")
@@ -63,8 +63,6 @@ static const char UNUSED *bitcoin_strings
...
(https://github.com/bitcoin/bitcoin/pull/33209#pullrequestreview-3128665447)
ACK df07b07b2d8b338b9af60ec8ffb82c20b9901f86.
Another line can be saved:
```diff
--- a/share/qt/translate.cmake
+++ b/share/qt/translate.cmake
@@ -33,7 +33,7 @@ function(extract_strings output)
file(STRINGS "bitcoinstrings.text" text ENCODING "UTF-8")
- set(messages)
+ set(messages "\"${COPYRIGHT_HOLDERS}\"")
foreach(line IN LISTS text)
if(line MATCHES "^msgid (.*)$")
set(msgid "${CMAKE_MATCH_1}")
@@ -63,8 +63,6 @@ static const char UNUSED *bitcoin_strings
...
💬 Crypt-iQ commented on issue "ci: failure in `logging_tests`":
(https://github.com/bitcoin/bitcoin/issues/33195#issuecomment-3197223283)
> This is however not what happened in https://cirrus-ci.com/task/5318274667249664?logs=ci#L721, as the test started and failed all in a span of a few milliseconds.
I think the CI logs are a bit weird and are printing the time from the boost test log -- the test actually took ~22.7 seconds: `[09:01:31.798] [1;34;49mtest/logging_tests.cpp(448): Leaving test case "logging_filesize_rate_limit"; testing time: 22705157us`. This would be enough to trigger the `Reset`. I am not sure why it took this
...
(https://github.com/bitcoin/bitcoin/issues/33195#issuecomment-3197223283)
> This is however not what happened in https://cirrus-ci.com/task/5318274667249664?logs=ci#L721, as the test started and failed all in a span of a few milliseconds.
I think the CI logs are a bit weird and are printing the time from the boost test log -- the test actually took ~22.7 seconds: `[09:01:31.798] [1;34;49mtest/logging_tests.cpp(448): Leaving test case "logging_filesize_rate_limit"; testing time: 22705157us`. This would be enough to trigger the `Reset`. I am not sure why it took this
...
🤔 danielabrozzoni reviewed a pull request: "refactor: Header sync optimisations & simplifications"
(https://github.com/bitcoin/bitcoin/pull/32740#pullrequestreview-3124489895)
Thanks for the reviews!
In my last push:
- Rebased onto the right commit :sweat_smile: (see https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2269846966)
- Addressed review comments
(https://github.com/bitcoin/bitcoin/pull/32740#pullrequestreview-3124489895)
Thanks for the reviews!
In my last push:
- Rebased onto the right commit :sweat_smile: (see https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2269846966)
- Addressed review comments