π l0rinc reopened a pull request: "bench: run `FindByte` across block-sized buffer"
(https://github.com/bitcoin/bitcoin/pull/34046)
The current benchmark doesn't represent a realistic scenario:
* it only checks 200 bytes, while in reality we have 8 MB: https://github.com/bitcoin/bitcoin/blob/2c44c41984e0a170cf38d595635a9e861854573c/src/validation.cpp#L5009
* it only checks the very last position (which is basically the missing case), while in reality it's a circular buffer, so the magic bytes can be anywhere
* plain array was needlessly serialized instead of written directly
The benchmark was updated to start from diff
...
(https://github.com/bitcoin/bitcoin/pull/34046)
The current benchmark doesn't represent a realistic scenario:
* it only checks 200 bytes, while in reality we have 8 MB: https://github.com/bitcoin/bitcoin/blob/2c44c41984e0a170cf38d595635a9e861854573c/src/validation.cpp#L5009
* it only checks the very last position (which is basically the missing case), while in reality it's a circular buffer, so the magic bytes can be anywhere
* plain array was needlessly serialized instead of written directly
The benchmark was updated to start from diff
...
π¬ l0rinc commented on pull request "bench: run `FindByte` across block-sized buffer":
(https://github.com/bitcoin/bitcoin/pull/34046#issuecomment-3643249468)
(close/open dance to fix boost download issue)
(https://github.com/bitcoin/bitcoin/pull/34046#issuecomment-3643249468)
(close/open dance to fix boost download issue)
β
l0rinc closed a pull request: "merkle: preβreserve leaves to prevent reallocs with odd vtx count"
(https://github.com/bitcoin/bitcoin/pull/32497)
(https://github.com/bitcoin/bitcoin/pull/32497)
π l0rinc reopened a pull request: "merkle: preβreserve leaves to prevent reallocs with odd vtx count"
(https://github.com/bitcoin/bitcoin/pull/32497)
#### Summary
`ComputeMerkleRoot` [duplicates the last hash](https://github.com/bitcoin/bitcoin/blob/39b6c139bd6be33699af781f1d71f6fed303d468/src/consensus/merkle.cpp#L54-L56) when the input size is odd. If the caller provides a `std::vector` whose capacity equals its size, that extra `push_back` forces a reallocation, doubling its capacity (causing peak memory usage of 3x the necessary size).
This affects roughly half of the created blocks (those with odd transaction counts), causing unnec
...
(https://github.com/bitcoin/bitcoin/pull/32497)
#### Summary
`ComputeMerkleRoot` [duplicates the last hash](https://github.com/bitcoin/bitcoin/blob/39b6c139bd6be33699af781f1d71f6fed303d468/src/consensus/merkle.cpp#L54-L56) when the input size is odd. If the caller provides a `std::vector` whose capacity equals its size, that extra `push_back` forces a reallocation, doubling its capacity (causing peak memory usage of 3x the necessary size).
This affects roughly half of the created blocks (those with odd transaction counts), causing unnec
...
π¬ theuni commented on issue "ci: The bitcoincore.org depends fallback is missing sqlite-autoconf-3500400.tar.gz":
(https://github.com/bitcoin/bitcoin/issues/34021#issuecomment-3643265612)
Yeah, there was a firewall issue after a server move. Fixed now, and sqlite-autoconf-3500400.tar.gz is available.
(https://github.com/bitcoin/bitcoin/issues/34021#issuecomment-3643265612)
Yeah, there was a firewall issue after a server move. Fixed now, and sqlite-autoconf-3500400.tar.gz is available.
β
theuni closed an issue: "ci: The bitcoincore.org depends fallback is missing sqlite-autoconf-3500400.tar.gz"
(https://github.com/bitcoin/bitcoin/issues/34021)
(https://github.com/bitcoin/bitcoin/issues/34021)
π¬ l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3643280306)
reACK 1a0423cf25bb60f98a29e06c39b7db2998860cd9
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3643280306)
reACK 1a0423cf25bb60f98a29e06c39b7db2998860cd9
β
plebhash closed an issue: "consider adding a new `interface RawTxFeed` on Mining IPC"
(https://github.com/bitcoin/bitcoin/issues/34030)
(https://github.com/bitcoin/bitcoin/issues/34030)
π¬ 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