💬 gmaxwell commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1710491169)
Yeah, I assumed that the specific limit used in the tests right how happened to evade failing. I don't have a strong opinion. The adjustments seemed fine to me but maybe just making some function for the adjustments and having the test use that to derive its test limits might make sense-- so that further updates to those adhoc adjustments don't accidentally break test limits that might be hit infrequently (or after changing the amount of work the test does).
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1710491169)
Yeah, I assumed that the specific limit used in the tests right how happened to evade failing. I don't have a strong opinion. The adjustments seemed fine to me but maybe just making some function for the adjustments and having the test use that to derive its test limits might make sense-- so that further updates to those adhoc adjustments don't accidentally break test limits that might be hit infrequently (or after changing the amount of work the test does).
👍 tdb3 approved a pull request: "contrib: support reading XORed blocks in linearize-data.py script"
(https://github.com/bitcoin/bitcoin/pull/30607#pullrequestreview-2229129698)
ACK 77ff0ec1f185b818b30877de2bedc1750319e6c4
Light code review. In addition to running unit/functionals locally, performed some sanity checks:
1) Created a regtest chain (~11k transactions in 411 blocks)
2) Confirmed that xor.dat had a non-zero key, created hashlist, then stopped bitcoind
3) Created a bootstrap.dat with `linearize-data.py` (successful)
4) Deleted the regtest datadir
5) Started bitcoind with `-loadblock=/path/to/bootstrap.dat -blocksxor=0` (successful restore), then stop
...
(https://github.com/bitcoin/bitcoin/pull/30607#pullrequestreview-2229129698)
ACK 77ff0ec1f185b818b30877de2bedc1750319e6c4
Light code review. In addition to running unit/functionals locally, performed some sanity checks:
1) Created a regtest chain (~11k transactions in 411 blocks)
2) Confirmed that xor.dat had a non-zero key, created hashlist, then stopped bitcoind
3) Created a bootstrap.dat with `linearize-data.py` (successful)
4) Deleted the regtest datadir
5) Started bitcoind with `-loadblock=/path/to/bootstrap.dat -blocksxor=0` (successful restore), then stop
...
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1710554112)
I've simplified the change a bit (now much closer to older deserialization code) and moved it to a separate commit (together with the introduction of the `DepGraph` reordering constructor).
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1710554112)
I've simplified the change a bit (now much closer to older deserialization code) and moved it to a separate commit (together with the introduction of the `DepGraph` reordering constructor).
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2277019007)
Unsure where this should live, but I have a Python reimplementation of the depgraph serialization code + mermaid formatter: https://gist.github.com/sipa/822f60db6608a26bb4aae75fd31bcb12
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2277019007)
Unsure where this should live, but I have a Python reimplementation of the depgraph serialization code + mermaid formatter: https://gist.github.com/sipa/822f60db6608a26bb4aae75fd31bcb12
💬 ajtowns commented on pull request "contrib/signet/miner updates":
(https://github.com/bitcoin/bitcoin/pull/28417#discussion_r1710716168)
`next_block_delta` can be somewhat small if you have a high difficulty target (ie 2.5m instead of 10m) and if `--poisson` is specified will be smaller again (the deterministic random calculation used against the genesis block reduces the target by ~73%). The `--poisson` result alone is already annoying -- it will generally only take 25 blocks before it's caught up to real time; but even without that if you switch from mining with a high nbits target to a minimal nbits target, then even without `
...
(https://github.com/bitcoin/bitcoin/pull/28417#discussion_r1710716168)
`next_block_delta` can be somewhat small if you have a high difficulty target (ie 2.5m instead of 10m) and if `--poisson` is specified will be smaller again (the deterministic random calculation used against the genesis block reduces the target by ~73%). The `--poisson` result alone is already annoying -- it will generally only take 25 blocks before it's caught up to real time; but even without that if you switch from mining with a high nbits target to a minimal nbits target, then even without `
...
💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2277190642)
(The drahtbot guix build failed due to a silent merge conflict, I presume)
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2277190642)
(The drahtbot guix build failed due to a silent merge conflict, I presume)
💬 maflcko commented on pull request "assumeutxo: Drop block height from metadata":
(https://github.com/bitcoin/bitcoin/pull/30598#issuecomment-2277205856)
The CI failure is known and can be ignored (https://github.com/bitcoin/bitcoin/actions/runs/10310090865/job/28541115356?pr=30598#step:27:229)
Only change since my last review is a small style-only cleanup to remove the unused parameter from the constructor.
re-ACK 00618e8745192d209c23e3ae873c077e58168957 🎌
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_fi
...
(https://github.com/bitcoin/bitcoin/pull/30598#issuecomment-2277205856)
The CI failure is known and can be ignored (https://github.com/bitcoin/bitcoin/actions/runs/10310090865/job/28541115356?pr=30598#step:27:229)
Only change since my last review is a small style-only cleanup to remove the unused parameter from the constructor.
re-ACK 00618e8745192d209c23e3ae873c077e58168957 🎌
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_fi
...
💬 maflcko commented on pull request "lint: Find function calls in default arguments":
(https://github.com/bitcoin/bitcoin/pull/30553#discussion_r1710808023)
This will also drop the "All checks passed!" on the success case (https://cirrus-ci.com/task/4866693351079936?logs=lint#L656), which I did not like when writing this.
(https://github.com/bitcoin/bitcoin/pull/30553#discussion_r1710808023)
This will also drop the "All checks passed!" on the success case (https://cirrus-ci.com/task/4866693351079936?logs=lint#L656), which I did not like when writing this.
💬 maflcko commented on pull request "lint: Find function calls in default arguments":
(https://github.com/bitcoin/bitcoin/pull/30553#discussion_r1710809136)
I copy-pasted this from the mlc linter, so maybe this can be done in a follow-up for both. Otherwise, it seems inconsistent.
(https://github.com/bitcoin/bitcoin/pull/30553#discussion_r1710809136)
I copy-pasted this from the mlc linter, so maybe this can be done in a follow-up for both. Otherwise, it seems inconsistent.
💬 maflcko commented on pull request "lint: Find function calls in default arguments":
(https://github.com/bitcoin/bitcoin/pull/30553#discussion_r1710811261)
I am trying to preserve the behavior of the previous check based on `git grep --extended-regexp`, which didn't exclude subtrees either.
However, I'll apply your suggestion in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/30553#discussion_r1710811261)
I am trying to preserve the behavior of the previous check based on `git grep --extended-regexp`, which didn't exclude subtrees either.
However, I'll apply your suggestion in a follow-up.
💬 maflcko commented on pull request "lint: Find function calls in default arguments":
(https://github.com/bitcoin/bitcoin/pull/30553#discussion_r1710817315)
Thanks, fixed typo (in another way)
(https://github.com/bitcoin/bitcoin/pull/30553#discussion_r1710817315)
Thanks, fixed typo (in another way)
🤔 maflcko reviewed a pull request: "Fixes for GCC 15 compatibility"
(https://github.com/bitcoin/bitcoin/pull/30612#pullrequestreview-2229402700)
> * Two are related to missing includes. You can't use `uint16_t` et al. without including `<cstdint>`.
Probably a good time to enable iwyu. :sweat_smile:
> This manifests as the following compile-time mess:
Are you sure this is not a GCC bug?
(https://github.com/bitcoin/bitcoin/pull/30612#pullrequestreview-2229402700)
> * Two are related to missing includes. You can't use `uint16_t` et al. without including `<cstdint>`.
Probably a good time to enable iwyu. :sweat_smile:
> This manifests as the following compile-time mess:
Are you sure this is not a GCC bug?
💬 maflcko commented on pull request "Fixes for GCC 15 compatibility":
(https://github.com/bitcoin/bitcoin/pull/30612#discussion_r1710836374)
```suggestion
template<std::integral I> // Disallow silent float -> int conversion
```
Please keep the comment
(https://github.com/bitcoin/bitcoin/pull/30612#discussion_r1710836374)
```suggestion
template<std::integral I> // Disallow silent float -> int conversion
```
Please keep the comment
💬 maflcko commented on pull request "Fixes for GCC 15 compatibility":
(https://github.com/bitcoin/bitcoin/pull/30612#discussion_r1710833459)
iwyu:
```
node/interface_ui.h should add these lines:
#include <vector> // for vector
node/interface_ui.h should remove these lines:
- #include <memory> // lines 11-11
(https://github.com/bitcoin/bitcoin/pull/30612#discussion_r1710833459)
iwyu:
```
node/interface_ui.h should add these lines:
#include <vector> // for vector
node/interface_ui.h should remove these lines:
- #include <memory> // lines 11-11
💬 whitslack commented on pull request "Fixes for GCC 15 compatibility":
(https://github.com/bitcoin/bitcoin/pull/30612#discussion_r1710848701)
With all due respect, that's a semantically void comment. `std::integral I` already means "disallow floats."
(https://github.com/bitcoin/bitcoin/pull/30612#discussion_r1710848701)
With all due respect, that's a semantically void comment. `std::integral I` already means "disallow floats."
💬 whitslack commented on pull request "Fixes for GCC 15 compatibility":
(https://github.com/bitcoin/bitcoin/pull/30612#issuecomment-2277260451)
> Are you sure this is not a GCC bug?
The compile-time error is of a class of errors that is not specific to GCC. Maybe the stdlibc++ in GCC 15 changed the implementation of `std::optional` so that the previously valid code in `feerate.h` is no longer valid. In general, if, in the course of checking a type constraint, the compiler instantiates a template that invokes the same type constraint, then there is an infinite recursion, and the compiler will reject the code with an error saying that
...
(https://github.com/bitcoin/bitcoin/pull/30612#issuecomment-2277260451)
> Are you sure this is not a GCC bug?
The compile-time error is of a class of errors that is not specific to GCC. Maybe the stdlibc++ in GCC 15 changed the implementation of `std::optional` so that the previously valid code in `feerate.h` is no longer valid. In general, if, in the course of checking a type constraint, the compiler instantiates a template that invokes the same type constraint, then there is an infinite recursion, and the compiler will reject the code with an error saying that
...
💬 maflcko commented on pull request "C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-2277264560)
ACK 2925bd537cbd8c70594e23f6c4298b7101f7f73d 🎷
<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+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 2925bd537cbd8c70594e23f6c4
...
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-2277264560)
ACK 2925bd537cbd8c70594e23f6c4298b7101f7f73d 🎷
<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+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 2925bd537cbd8c70594e23f6c4
...
💬 Sjors commented on pull request "doc, chainparams: 29775 release notes and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/30604#discussion_r1710890448)
35ad61a6bde14a74cc07dc8ecf3bb1cd49590e46: let's define a separate constant and make it clear these are different but related:
```cpp
/**
* Maximum number of seconds that the timestamp of the first
* block of a difficulty adjustment period is allowed to
* be earlier than the last block of the previous period.
*/
static constexpr int64_t MAX_TIMEWARP{MAX_FUTURE_BLOCK_TIME};
/**
* If the timestamp of the last block in a difficulty period is set
* MAX_FUTURE_BLOCK_TIME seconds in
...
(https://github.com/bitcoin/bitcoin/pull/30604#discussion_r1710890448)
35ad61a6bde14a74cc07dc8ecf3bb1cd49590e46: let's define a separate constant and make it clear these are different but related:
```cpp
/**
* Maximum number of seconds that the timestamp of the first
* block of a difficulty adjustment period is allowed to
* be earlier than the last block of the previous period.
*/
static constexpr int64_t MAX_TIMEWARP{MAX_FUTURE_BLOCK_TIME};
/**
* If the timestamp of the last block in a difficulty period is set
* MAX_FUTURE_BLOCK_TIME seconds in
...
💬 maflcko commented on pull request "Fixes for GCC 15 compatibility":
(https://github.com/bitcoin/bitcoin/pull/30612#issuecomment-2277287178)
> > Are you sure this is not a GCC bug?
>
> The compile-time error is of a class of errors that is not specific to GCC. Maybe the stdlibc++ in GCC 15 changed the implementation of `std::optional` so that the previously valid code in `feerate.h` is no longer valid.
Correct. Though, GCC 15 is so far out that there is a chance the change will be reverted again internally, because I assume it breaks compilation of more code than just this single instance. I am wondering if their change was int
...
(https://github.com/bitcoin/bitcoin/pull/30612#issuecomment-2277287178)
> > Are you sure this is not a GCC bug?
>
> The compile-time error is of a class of errors that is not specific to GCC. Maybe the stdlibc++ in GCC 15 changed the implementation of `std::optional` so that the previously valid code in `feerate.h` is no longer valid.
Correct. Though, GCC 15 is so far out that there is a chance the change will be reverted again internally, because I assume it breaks compilation of more code than just this single instance. I am wondering if their change was int
...
⚠️ Sjors opened an issue: "UpdateTime() needs to account for timewarp fix on testnet4 "
(https://github.com/bitcoin/bitcoin/issues/30614)
In (`miner.cpp`) we set the block (template) timestamp as follows:
```cpp
pblock->nTime = TicksSinceEpoch<std::chrono::seconds>(NodeClock::now());
...
UpdateTime(pblock, ...)
int64_t UpdateTime(CBlockHeader* pblock, ...)
{
int64_t nOldTime = pblock->nTime;
int64_t nNewTime{std::max<int64_t>(pindexPrev->GetMedianTimePast() + 1, TicksSinceEpoch<std::chrono::seconds>(NodeClock::now()))};
if (nOldTime < nNewTime) {
pblock->nTime = nNewTime;
}
```
In oth
...
(https://github.com/bitcoin/bitcoin/issues/30614)
In (`miner.cpp`) we set the block (template) timestamp as follows:
```cpp
pblock->nTime = TicksSinceEpoch<std::chrono::seconds>(NodeClock::now());
...
UpdateTime(pblock, ...)
int64_t UpdateTime(CBlockHeader* pblock, ...)
{
int64_t nOldTime = pblock->nTime;
int64_t nNewTime{std::max<int64_t>(pindexPrev->GetMedianTimePast() + 1, TicksSinceEpoch<std::chrono::seconds>(NodeClock::now()))};
if (nOldTime < nNewTime) {
pblock->nTime = nNewTime;
}
```
In oth
...