💬 sipa commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3643378098)
Concept ACK, but disagree with the approach of the first commit.
I don't think it tests anything useful:
* If it wanted to test the exact behavior of the skip-height pointer building, it could test that directly. No need for averages or medians here; it could test that block 171 points to block 161 and no other, for example.
* If it wanted to test something related to the performance of the constructed index, without hardcoding the exact behavior, it would need to do something like pick ran
...
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3643378098)
Concept ACK, but disagree with the approach of the first commit.
I don't think it tests anything useful:
* If it wanted to test the exact behavior of the skip-height pointer building, it could test that directly. No need for averages or medians here; it could test that block 171 points to block 161 and no other, for example.
* If it wanted to test something related to the performance of the constructed index, without hardcoding the exact behavior, it would need to do something like pick ran
...
📝 maflcko opened a pull request: " log: Remove brittle and confusing LogPrintLevel "
(https://github.com/bitcoin/bitcoin/pull/34051)
`LogPrintLevel` has many issues:
* It encourages to log several levels in one source location. This is problematic, because all levels (even warnings and errors) will be rate limited equally for the same location.
* Its warning and error logs are specially formatted compared to all other warning and error logs in the codebase, making them harder to spot (both in the debug log and in the code).
* It is verbose to type and read.
* It is confusing, because the majority of code uses the `Log$L
...
(https://github.com/bitcoin/bitcoin/pull/34051)
`LogPrintLevel` has many issues:
* It encourages to log several levels in one source location. This is problematic, because all levels (even warnings and errors) will be rate limited equally for the same location.
* Its warning and error logs are specially formatted compared to all other warning and error logs in the codebase, making them harder to spot (both in the debug log and in the code).
* It is verbose to type and read.
* It is confusing, because the majority of code uses the `Log$L
...
💬 maflcko commented on pull request "scripted-diff: Unify error and warning log formatting":
(https://github.com/bitcoin/bitcoin/pull/34033#issuecomment-3643390479)
> something like https://github.com/stickies-v/bitcoin/commits/2025-12/separate-log-print-level/ seems worth doing. As a bonus, it prevents issues such as in #34008 where debug logs can lead to unconditional logs being ratelimited.
Thx, create a pull request with your bugfix commits in https://github.com/bitcoin/bitcoin/pull/34051
(https://github.com/bitcoin/bitcoin/pull/34033#issuecomment-3643390479)
> something like https://github.com/stickies-v/bitcoin/commits/2025-12/separate-log-print-level/ seems worth doing. As a bonus, it prevents issues such as in #34008 where debug logs can lead to unconditional logs being ratelimited.
Thx, create a pull request with your bugfix commits in https://github.com/bitcoin/bitcoin/pull/34051
💬 fanquake commented on issue "29.x depends: fallback server missing capnp downloads":
(https://github.com/bitcoin/bitcoin/issues/33901#issuecomment-3643393113)
This is now fixed.
(https://github.com/bitcoin/bitcoin/issues/33901#issuecomment-3643393113)
This is now fixed.
✅ fanquake closed an issue: "29.x depends: fallback server missing capnp downloads"
(https://github.com/bitcoin/bitcoin/issues/33901)
(https://github.com/bitcoin/bitcoin/issues/33901)
💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3643415546)
> I don't think it tests anything useful:
>
> * If it wanted to test the exact behavior of the skip-height pointer building, it could test that directly. No need for averages or medians here; it could test that block 171 points to block 161 and no other, for example.
>
> * If it wanted to test something related to the performance of the constructed index, without hardcoding the exact behavior, it would need to do something like pick random pairs of blocks in the chain, and see how
...
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3643415546)
> I don't think it tests anything useful:
>
> * If it wanted to test the exact behavior of the skip-height pointer building, it could test that directly. No need for averages or medians here; it could test that block 171 points to block 161 and no other, for example.
>
> * If it wanted to test something related to the performance of the constructed index, without hardcoding the exact behavior, it would need to do something like pick random pairs of blocks in the chain, and see how
...
💬 sipa commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3643423211)
My point is that these two things, regardless of how they are accomplished, are the interesting ones.
* Your test achieves the first, but only approximately, and is much more complicated than needed for that purposes.
* Your test does not achieve the second at all.
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3643423211)
My point is that these two things, regardless of how they are accomplished, are the interesting ones.
* Your test achieves the first, but only approximately, and is much more complicated than needed for that purposes.
* Your test does not achieve the second at all.
💬 achow101 commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#issuecomment-3643539057)
ACK c1e554d3e5834a140f2a53854018499a3bfe6822
(https://github.com/bitcoin/bitcoin/pull/32414#issuecomment-3643539057)
ACK c1e554d3e5834a140f2a53854018499a3bfe6822
💬 mzumsande commented on pull request "test: add functional test for outbound connection management":
(https://github.com/bitcoin/bitcoin/pull/33954#discussion_r2611870620)
I think so. But I think that `tor_port(MAX_NODES) + 1` isn't a good choice because it will lead to collisions among parallel running tests as soon as there is more than one test using the feature. I changed it to `p2p_port(n) + PORT_RANGE *3`.
(https://github.com/bitcoin/bitcoin/pull/33954#discussion_r2611870620)
I think so. But I think that `tor_port(MAX_NODES) + 1` isn't a good choice because it will lead to collisions among parallel running tests as soon as there is more than one test using the feature. I changed it to `p2p_port(n) + PORT_RANGE *3`.
🚀 achow101 merged a pull request: "validation: periodically flush dbcache during reindex-chainstate"
(https://github.com/bitcoin/bitcoin/pull/32414)
(https://github.com/bitcoin/bitcoin/pull/32414)
💬 maflcko commented on pull request "test: add functional test for outbound connection management":
(https://github.com/bitcoin/bitcoin/pull/33954#discussion_r2611907262)
Haven't looked here in detail, but my preference would be to not call `+` or `+=` on an integral port returned by a helper function. This will lead to issues down the line, and it would be better to always call the helper that internally asserts and checks the port range. See https://github.com/bitcoin/bitcoin/pull/33260/files
(https://github.com/bitcoin/bitcoin/pull/33954#discussion_r2611907262)
Haven't looked here in detail, but my preference would be to not call `+` or `+=` on an integral port returned by a helper function. This will lead to issues down the line, and it would be better to always call the helper that internally asserts and checks the port range. See https://github.com/bitcoin/bitcoin/pull/33260/files
💬 mzumsande commented on pull request "test: add functional test for outbound connection management":
(https://github.com/bitcoin/bitcoin/pull/33954#discussion_r2611923052)
In trying to apply this I think I found a more general problem with port collisions:
As soon as more than one test uses this feature, there is the possibility of port collisions between two tests run in parallel. So we would need to assign separate ranges.
We know that a node will only make up to 11 outbound connections at a given moment, but currently disconnections/new connections will just be added to `self.auto_outbound_destinations`, so something like
```
def auto_listener_port(n):
...
(https://github.com/bitcoin/bitcoin/pull/33954#discussion_r2611923052)
In trying to apply this I think I found a more general problem with port collisions:
As soon as more than one test uses this feature, there is the possibility of port collisions between two tests run in parallel. So we would need to assign separate ranges.
We know that a node will only make up to 11 outbound connections at a given moment, but currently disconnections/new connections will just be added to `self.auto_outbound_destinations`, so something like
```
def auto_listener_port(n):
...
💬 mzumsande commented on pull request "test: add functional test for outbound connection management":
(https://github.com/bitcoin/bitcoin/pull/33954#discussion_r2611923854)
done
(https://github.com/bitcoin/bitcoin/pull/33954#discussion_r2611923854)
done
🤔 pablomartin4btc reviewed a pull request: "refactor: unify container presence checks"
(https://github.com/bitcoin/bitcoin/pull/33192#pullrequestreview-3569365873)
re-ACK d9319b06cf82664d55f255387a348135fd7f91c7
Checked the diffs since my last [review](https://github.com/bitcoin/bitcoin/pull/33192#pullrequestreview-3302612544).
(https://github.com/bitcoin/bitcoin/pull/33192#pullrequestreview-3569365873)
re-ACK d9319b06cf82664d55f255387a348135fd7f91c7
Checked the diffs since my last [review](https://github.com/bitcoin/bitcoin/pull/33192#pullrequestreview-3302612544).
💬 maflcko commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3643843248)
Nice. Only non-test change is adding the missing `std::move` to avoid the copy.
review ACK 07135290c1720a14c9d2f18a5700bb6565ae7a10 🏪
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N9
...
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3643843248)
Nice. Only non-test change is adding the missing `std::move` to avoid the copy.
review ACK 07135290c1720a14c9d2f18a5700bb6565ae7a10 🏪
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N9
...
🤔 brunoerg reviewed a pull request: "Implementation of SwiftSync"
(https://github.com/bitcoin/bitcoin/pull/34004#pullrequestreview-3569068972)
I've ran a mutation analysis on this PR. Unkilled mutants should be killed (covered) by an unit or functional test (if make sense). However, I noticed that there are part of the code that doesn't even have test coverage (corecheck.dev would have show it but did not generated the report for this PR) and should have it. As soon as more tests are added to this PR, I can re-run the analysis again.
(https://github.com/bitcoin/bitcoin/pull/34004#pullrequestreview-3569068972)
I've ran a mutation analysis on this PR. Unkilled mutants should be killed (covered) by an unit or functional test (if make sense). However, I noticed that there are part of the code that doesn't even have test coverage (corecheck.dev would have show it but did not generated the report for this PR) and should have it. As soon as more tests are added to this PR, I can re-run the analysis again.
💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611914615)
We could exercise these errors (file doesn't exist and open failure) in the functional tests. An (obvious) unkilled mutant:
```diff
diff --git a/src/init.cpp b/muts-pr-34004-init-cpp/init.mutant.1.cpp
index fb7336f9ae..59de0f6b7a 100644
--- a/src/init.cpp
+++ b/muts-pr-34004-init-cpp/init.mutant.1.cpp
@@ -1812,7 +1812,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
AutoFile afile{file};
if (afile.IsNull()) {
LogError
...
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611914615)
We could exercise these errors (file doesn't exist and open failure) in the functional tests. An (obvious) unkilled mutant:
```diff
diff --git a/src/init.cpp b/muts-pr-34004-init-cpp/init.mutant.1.cpp
index fb7336f9ae..59de0f6b7a 100644
--- a/src/init.cpp
+++ b/muts-pr-34004-init-cpp/init.mutant.1.cpp
@@ -1812,7 +1812,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
AutoFile afile{file};
if (afile.IsNull()) {
LogError
...
💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611893305)
Unkilled mutant:
```diff
diff --git a/src/validation.cpp b/muts-pr-34004-validation-cpp/validation.mutant.16.cpp
index 3d84576e7d..efc5037570 100644
--- a/src/validation.cpp
+++ b/muts-pr-34004-validation-cpp/validation.mutant.16.cpp
@@ -2613,7 +2613,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
// * legacy (always)
// * p2sh (when P2SH enabled in flags and excludes coinbase)
// * witness (when witness enabled in flags an
...
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611893305)
Unkilled mutant:
```diff
diff --git a/src/validation.cpp b/muts-pr-34004-validation-cpp/validation.mutant.16.cpp
index 3d84576e7d..efc5037570 100644
--- a/src/validation.cpp
+++ b/muts-pr-34004-validation-cpp/validation.mutant.16.cpp
@@ -2613,7 +2613,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
// * legacy (always)
// * p2sh (when P2SH enabled in flags and excludes coinbase)
// * witness (when witness enabled in flags an
...
💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611981935)
This error isn't covered by any test and could be addressed in `feature_swiftsync`. Perhaps by deleting a block file before calling the `generatetxohints` RPC.
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611981935)
This error isn't covered by any test and could be addressed in `feature_swiftsync`. Perhaps by deleting a block file before calling the `generatetxohints` RPC.
💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611879532)
> these failures could be exercised in the provided unit test
+1. Some unkilled mutants for obvious lack of testing:
```diff
--- a/src/swiftsync.cpp
+++ b/muts-pr-34004-swiftsync-cpp/swiftsync.mutant.25.cpp
@@ -82,7 +82,7 @@ HintsfileReader::HintsfileReader(AutoFile& file) : m_file(file.release())
}
uint8_t version{};
m_file >> version;
- if (version != FILE_VERSION) {
+ if (1==0) {
throw std::ios_base::failure("HintsfileReader: Unsupported file versi
...
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611879532)
> these failures could be exercised in the provided unit test
+1. Some unkilled mutants for obvious lack of testing:
```diff
--- a/src/swiftsync.cpp
+++ b/muts-pr-34004-swiftsync-cpp/swiftsync.mutant.25.cpp
@@ -82,7 +82,7 @@ HintsfileReader::HintsfileReader(AutoFile& file) : m_file(file.release())
}
uint8_t version{};
m_file >> version;
- if (version != FILE_VERSION) {
+ if (1==0) {
throw std::ios_base::failure("HintsfileReader: Unsupported file versi
...
💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2612145025)
nit:
```suggestion
for _ in range(NUM_SWIFTSYNC_BLOCKS):
```
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2612145025)
nit:
```suggestion
for _ in range(NUM_SWIFTSYNC_BLOCKS):
```