💬 maflcko commented on issue "OpenBSD, NetBSD: `-reindex` is broken":
(https://github.com/bitcoin/bitcoin/issues/33128#issuecomment-3150231458)
Would be nice to see a test run with additional logging added to `ChainstateManager::LoadExternalBlockFile`, and a log showing the size of the blk dat file.
(https://github.com/bitcoin/bitcoin/issues/33128#issuecomment-3150231458)
Would be nice to see a test run with additional logging added to `ChainstateManager::LoadExternalBlockFile`, and a log showing the size of the blk dat file.
💬 cedwies commented on pull request "tests: cover abortrescan() in-flight True path with dynamic-tail retry":
(https://github.com/bitcoin/bitcoin/pull/33131#discussion_r2251205963)
When I first wrote the test, I had repeated issues on MacOS. the wallet’s scanning flag sometimes didn’t turn true quickly enough when I fired a full rescan. Too quickly, I convinced myself that the issue is with the tail length (I honestly never questioned it anymore until your question).
Upon inspection: Yes, I will fix it (which simplifies the code a lot).
(https://github.com/bitcoin/bitcoin/pull/33131#discussion_r2251205963)
When I first wrote the test, I had repeated issues on MacOS. the wallet’s scanning flag sometimes didn’t turn true quickly enough when I fired a full rescan. Too quickly, I convinced myself that the issue is with the tail length (I honestly never questioned it anymore until your question).
Upon inspection: Yes, I will fix it (which simplifies the code a lot).
💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2251210391)
Definitely - should be fixed now:
```diff
$ git diff 784459ac79 104b40c8c9
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index c8d824bb6c..05ec73697c 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -1080,8 +1080,6 @@ bool BlockManager::ReadRawBlock(std::vector<std::byte>& block, const FlatFilePos
return true;
}
-static const uint32_t HEADER_SIZE = ::GetSerializeSize(CBlockHeader{});
-
bool BlockManager::ReadTxFromBlock(CTransac
...
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2251210391)
Definitely - should be fixed now:
```diff
$ git diff 784459ac79 104b40c8c9
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index c8d824bb6c..05ec73697c 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -1080,8 +1080,6 @@ bool BlockManager::ReadRawBlock(std::vector<std::byte>& block, const FlatFilePos
return true;
}
-static const uint32_t HEADER_SIZE = ::GetSerializeSize(CBlockHeader{});
-
bool BlockManager::ReadTxFromBlock(CTransac
...
💬 cedwies commented on pull request "tests: cover abortrescan() in-flight True path with dynamic-tail retry":
(https://github.com/bitcoin/bitcoin/pull/33131#issuecomment-3150239992)
Also, I will block the test on t.join(). (no timeout), so the
background rescan thread is guaranteed to exit before the test returns (which I think causes the CI
cleanup step from failing with “OSError: directory not empty”).
(https://github.com/bitcoin/bitcoin/pull/33131#issuecomment-3150239992)
Also, I will block the test on t.join(). (no timeout), so the
background rescan thread is guaranteed to exit before the test returns (which I think causes the CI
cleanup step from failing with “OSError: directory not empty”).
💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2251213333)
Oops, sorry -> should be fixed now:
```diff
$ git diff b2a22ce33d 4441827ef4
diff --git a/doc/REST-interface.md b/doc/REST-interface.md
index ffb3c09af1..8f8431c29a 100644
--- a/doc/REST-interface.md
+++ b/doc/REST-interface.md
@@ -42,7 +42,7 @@ Given a block hash and transaction offset within it: returns a transaction in bi
Responds with 404 if the transaction doesn't exist.
By default, this endpoint will also deserialize the leading transactions, before reading and returning the
...
(https://github.com/bitcoin/bitcoin/pull/32541#discussion_r2251213333)
Oops, sorry -> should be fixed now:
```diff
$ git diff b2a22ce33d 4441827ef4
diff --git a/doc/REST-interface.md b/doc/REST-interface.md
index ffb3c09af1..8f8431c29a 100644
--- a/doc/REST-interface.md
+++ b/doc/REST-interface.md
@@ -42,7 +42,7 @@ Given a block hash and transaction offset within it: returns a transaction in bi
Responds with 404 if the transaction doesn't exist.
By default, this endpoint will also deserialize the leading transactions, before reading and returning the
...
💬 hebasto commented on pull request "cmake: Switch to generated `ts_files.cmake` file":
(https://github.com/bitcoin/bitcoin/pull/33115#discussion_r2251227924)
> transifex updated
I've pulled the most recent translations.
(https://github.com/bitcoin/bitcoin/pull/33115#discussion_r2251227924)
> transifex updated
I've pulled the most recent translations.
💬 hebasto commented on pull request "cmake: Switch to generated `ts_files.cmake` file":
(https://github.com/bitcoin/bitcoin/pull/33115#discussion_r2251239067)
> > Are we going to pull in the updates, or leave this as-is?
>
> I've also created a preliminary review for all languages: https://github.com/maflcko/b-c-gui-translations-review/tree/main/reviews
>
> There is some obvious erroneous translations, like https://github.com/maflcko/b-c-gui-translations-review/blob/05100fec918fc44787d43004ea3b02749f5a16ee/reviews/te.md#L160-L166, but I am not familiar with those languages, so I can't evaluate if the LLM generated translation is correct.
Than
...
(https://github.com/bitcoin/bitcoin/pull/33115#discussion_r2251239067)
> > Are we going to pull in the updates, or leave this as-is?
>
> I've also created a preliminary review for all languages: https://github.com/maflcko/b-c-gui-translations-review/tree/main/reviews
>
> There is some obvious erroneous translations, like https://github.com/maflcko/b-c-gui-translations-review/blob/05100fec918fc44787d43004ea3b02749f5a16ee/reviews/te.md#L160-L166, but I am not familiar with those languages, so I can't evaluate if the LLM generated translation is correct.
Than
...
💬 janb84 commented on pull request "Allowing multi client support in guix-build":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3150314952)
Concept NACK
What is the usecase for this ? forks would also fork the GUIX build script and if this is intended as multi version build script than move it to an independent repo.
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3150314952)
Concept NACK
What is the usecase for this ? forks would also fork the GUIX build script and if this is intended as multi version build script than move it to an independent repo.
💬 hebasto commented on issue "OpenBSD, NetBSD: `-reindex` is broken":
(https://github.com/bitcoin/bitcoin/issues/33128#issuecomment-3150347633)
> ... showing the size of the blk dat file.
You mean `blkdat.GetPos()`?
(https://github.com/bitcoin/bitcoin/issues/33128#issuecomment-3150347633)
> ... showing the size of the blk dat file.
You mean `blkdat.GetPos()`?
👍 Sjors approved a pull request: "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key"
(https://github.com/bitcoin/bitcoin/pull/32471#pullrequestreview-3083871225)
ACK 45e06e05e7f8d638ecc91a1ba62e1b38a5133a2b
I find that the separation of 23431cde206bd6e18f274773f77bd914553bf2cc makes it easier to follow. The separation of https://github.com/bitcoin/bitcoin/commit/d2b84af58073630f87df32a7b785971fe32cb9be from https://github.com/bitcoin/bitcoin/commit/49f41141c674f83d0b66262a5b21aad623704264 is less useful, but fine by me.
(https://github.com/bitcoin/bitcoin/pull/32471#pullrequestreview-3083871225)
ACK 45e06e05e7f8d638ecc91a1ba62e1b38a5133a2b
I find that the separation of 23431cde206bd6e18f274773f77bd914553bf2cc makes it easier to follow. The separation of https://github.com/bitcoin/bitcoin/commit/d2b84af58073630f87df32a7b785971fe32cb9be from https://github.com/bitcoin/bitcoin/commit/49f41141c674f83d0b66262a5b21aad623704264 is less useful, but fine by me.
💬 Sjors commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2251304473)
In 49f41141c674f83d0b66262a5b21aad623704264 _descriptor: ToPrivateString() pass if at least 1 priv key exists_: maybe for good measure:
```cpp
*has_priv_key = false;
```
In both places it seems better to check that `priv_key` exists, even if that's always the case in this fuzzer. IIUC in that case you can leave out `tmp{false}` and just pass `nullptr` below.
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2251304473)
In 49f41141c674f83d0b66262a5b21aad623704264 _descriptor: ToPrivateString() pass if at least 1 priv key exists_: maybe for good measure:
```cpp
*has_priv_key = false;
```
In both places it seems better to check that `priv_key` exists, even if that's always the case in this fuzzer. IIUC in that case you can leave out `tmp{false}` and just pass `nullptr` below.
💬 Sjors commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2251274240)
In 64213213b3691c9d93de95f880f820a640cd3c35 _descriptors: add IsWatchOnly()_: nit: you don't need `output_type.has_value()`
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2251274240)
In 64213213b3691c9d93de95f880f820a640cd3c35 _descriptors: add IsWatchOnly()_: nit: you don't need `output_type.has_value()`
💬 maflcko commented on pull request "cmake: Switch to generated `ts_files.cmake` file":
(https://github.com/bitcoin/bitcoin/pull/33115#discussion_r2251381743)
> As for this PR, the last commit only serves only to demonstrate that the modified build system handles translation files properly. So I don't think that any translation-specific issue should block it.
Agree. Discussion can be closed.
(https://github.com/bitcoin/bitcoin/pull/33115#discussion_r2251381743)
> As for this PR, the last commit only serves only to demonstrate that the modified build system handles translation files properly. So I don't think that any translation-specific issue should block it.
Agree. Discussion can be closed.
💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3150541294)
Applied a few more fixes: [`b2a22ce -> 4441827`](https://github.com/bitcoin/bitcoin/compare/b2a22ce33dc69697181547fa1e83bd0ed3321565..4441827ef4e9b5fe306c5f0a81a52b5d2b5e0b69)
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3150541294)
Applied a few more fixes: [`b2a22ce -> 4441827`](https://github.com/bitcoin/bitcoin/compare/b2a22ce33dc69697181547fa1e83bd0ed3321565..4441827ef4e9b5fe306c5f0a81a52b5d2b5e0b69)
💬 hodlinator commented on pull request "Allowing multi client support in guix-build":
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2251460522)
Might be able to parse `CLIENT_NAME` from *build/src/bitcoin-build-config.h* and replace ` ` with `-`?
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2251460522)
Might be able to parse `CLIENT_NAME` from *build/src/bitcoin-build-config.h* and replace ` ` with `-`?
🤔 glozow reviewed a pull request: "fuzz: txgraph: fix `real_is_optimal` flag propagation in `CommitStaging`"
(https://github.com/bitcoin/bitcoin/pull/33132#pullrequestreview-3084265420)
ACK 444dcb2f9944ad5208bf00c9f197da7b2c98063c
The fix works and makes sense to me. I suppose a concrete scenario is:
1. we have main and staging, neither optimally linearized
2. we create an observer for the main level (alternatively, it could be oversized)
3. we call DoWork() which returns true after optimally linearizing the staging level and skipping the main level
4. we delete the main level observer
5. we call CommitStaging. sims staging gets copied over.
6. even though staging was
...
(https://github.com/bitcoin/bitcoin/pull/33132#pullrequestreview-3084265420)
ACK 444dcb2f9944ad5208bf00c9f197da7b2c98063c
The fix works and makes sense to me. I suppose a concrete scenario is:
1. we have main and staging, neither optimally linearized
2. we create an observer for the main level (alternatively, it could be oversized)
3. we call DoWork() which returns true after optimally linearizing the staging level and skipping the main level
4. we delete the main level observer
5. we call CommitStaging. sims staging gets copied over.
6. even though staging was
...
👍 willcl-ark approved a pull request: "cmake: Install internal binaries to <prefix>/libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-3084294348)
utACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
> Sorry about that, the PR description was confusing ...
No that was my bad, I should have read the PR comments before reviewing (perhaps, although I don't like to).
I am in favour of moving binaries to libexec/, including `test_bitcoin`, as per this change.
I've tried to find other bitcoin packages which may break on this, but can't find many, and the few I have found are already outdated in other ways, so require a "manual update" alre
...
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-3084294348)
utACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
> Sorry about that, the PR description was confusing ...
No that was my bad, I should have read the PR comments before reviewing (perhaps, although I don't like to).
I am in favour of moving binaries to libexec/, including `test_bitcoin`, as per this change.
I've tried to find other bitcoin packages which may break on this, but can't find many, and the few I have found are already outdated in other ways, so require a "manual update" alre
...
💬 willcl-ark commented on pull request "Allowing multi client support in guix-build":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3150797545)
Concept NACK
Unless this value was picked up from elsewhere, I don't see how this makes it easier to support "multi clients""? A two-line downstream patch... remains a two-line downstream patch. Only the lines being patched have changed?
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3150797545)
Concept NACK
Unless this value was picked up from elsewhere, I don't see how this makes it easier to support "multi clients""? A two-line downstream patch... remains a two-line downstream patch. Only the lines being patched have changed?
🤔 furszy reviewed a pull request: "wallet: Avoid potentially writing incorrect best block locator"
(https://github.com/bitcoin/bitcoin/pull/29652#pullrequestreview-3084332144)
Since #30221 got merged, I think this one can be closed
(https://github.com/bitcoin/bitcoin/pull/29652#pullrequestreview-3084332144)
Since #30221 got merged, I think this one can be closed
🤔 marcofleon reviewed a pull request: "p2p: TxOrphanage revamp cleanups"
(https://github.com/bitcoin/bitcoin/pull/32941#pullrequestreview-3084354759)
Code review ACK 3d4d4f0d92d42809e74377e4380abdc70f74de5d
Fuzzed all three targets over the weekend, no crashes. Only small thing I caught while reading the `txorphan_protected` target:
https://github.com/bitcoin/bitcoin/blob/3d4d4f0d92d42809e74377e4380abdc70f74de5d/src/test/fuzz/txorphan.cpp#L331
That line should be `(tx->vin.size() / 10) + 1` but it doesn't result in a crash because it's overestimating the latency score for an additional announcer.
(https://github.com/bitcoin/bitcoin/pull/32941#pullrequestreview-3084354759)
Code review ACK 3d4d4f0d92d42809e74377e4380abdc70f74de5d
Fuzzed all three targets over the weekend, no crashes. Only small thing I caught while reading the `txorphan_protected` target:
https://github.com/bitcoin/bitcoin/blob/3d4d4f0d92d42809e74377e4380abdc70f74de5d/src/test/fuzz/txorphan.cpp#L331
That line should be `(tx->vin.size() / 10) + 1` but it doesn't result in a crash because it's overestimating the latency score for an additional announcer.