💬 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
💬 danielabrozzoni commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2281794387)
Ooops, not sure what happened there, thanks for noticing!
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2281794387)
Ooops, not sure what happened there, thanks for noticing!
💬 danielabrozzoni commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2281924397)
Thanks, I didn't like the initial formatting either, but I thought it was common (looking at other constructors in the repository, no, it's not...)
I updated the formatting both in `HeadersSyncState` and `CompressedHeaders`!
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2281924397)
Thanks, I didn't like the initial formatting either, but I thought it was common (looking at other constructors in the repository, no, it's not...)
I updated the formatting both in `HeadersSyncState` and `CompressedHeaders`!
💬 danielabrozzoni commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2279448653)
I think it fits slightly better with the span change... since in c++20 span doesn't have a `cbegin` method, we needed to change the initial code, and update to `ranges::all_of`
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2279448653)
I think it fits slightly better with the span change... since in c++20 span doesn't have a `cbegin` method, we needed to change the initial code, and update to `ranges::all_of`
🤔 janb84 reviewed a pull request: "cmake: Drop python dependency for translate"
(https://github.com/bitcoin/bitcoin/pull/33209#pullrequestreview-3128722396)
re ACK df07b07b2d8b338b9af60ec8ffb82c20b9901f86
changes since last ACK:
- dropped shebang
- script optimizations
- ifdef style change (thanks for including my nit)
(https://github.com/bitcoin/bitcoin/pull/33209#pullrequestreview-3128722396)
re ACK df07b07b2d8b338b9af60ec8ffb82c20b9901f86
changes since last ACK:
- dropped shebang
- script optimizations
- ifdef style change (thanks for including my nit)
💬 alexanderwiederin commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2282651470)
If `0` is success, we should invert this, right?
```suggestion
return result ? 0 : 1;
```
The earlier error returns should also be updated from `return 0;` to `return 1;` if I am not wrong.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2282651470)
If `0` is success, we should invert this, right?
```suggestion
return result ? 0 : 1;
```
The earlier error returns should also be updated from `return 0;` to `return 1;` if I am not wrong.
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2282652539)
I am asking myself why need this function at all now. You can retrieve the position of an entry by getting its height, and then just retrieving the next one in the chain. How about just removing it instead?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2282652539)
I am asking myself why need this function at all now. You can retrieve the position of an entry by getting its height, and then just retrieving the next one in the chain. How about just removing it instead?
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2282658564)
I wanted to mirror the existing behaviour of the deprecated consensus library here. There we return `1` on success of this function.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2282658564)
I wanted to mirror the existing behaviour of the deprecated consensus library here. There we return `1` on success of this function.