💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1709816237)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708942643
> Nit: it would be nice to give this the global `g_` prefix (though it is not introduced in this PR).
It's a good point but I think modifying serialize.h is outside the scope of this PR, so I will leave it alone here.
I'm not sure a `g_` prefix is ideal since this is a constant, so all caps `DESERIALIZE` would probably communicate its purpose more accurately and be more in spirit of the the developer notes.
I th
...
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1709816237)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708942643
> Nit: it would be nice to give this the global `g_` prefix (though it is not introduced in this PR).
It's a good point but I think modifying serialize.h is outside the scope of this PR, so I will leave it alone here.
I'm not sure a `g_` prefix is ideal since this is a constant, so all caps `DESERIALIZE` would probably communicate its purpose more accurately and be more in spirit of the the developer notes.
I th
...
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2276410668)
An [announcement](https://groups.google.com/g/bitcoindev/c/hgKkfQWzrTo) about the migration to the CMake build system was made on the mailing list. Therefore, more comments from fellow reviewers and testers are expected.
To let us maintain focus on making progress, the following statement has been add to the PR description:
We, the build system developers, kindly ask reviewers to refrain from making suggestions that are not directly related to the migration process or can be implemented se
...
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2276410668)
An [announcement](https://groups.google.com/g/bitcoindev/c/hgKkfQWzrTo) about the migration to the CMake build system was made on the mailing list. Therefore, more comments from fellow reviewers and testers are expected.
To let us maintain focus on making progress, the following statement has been add to the PR description:
We, the build system developers, kindly ask reviewers to refrain from making suggestions that are not directly related to the migration process or can be implemented se
...
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1710058792)
It is a TODO comment. I'll update it if/when this file will be touched for other changes.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1710058792)
It is a TODO comment. I'll update it if/when this file will be touched for other changes.
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1710065883)
Source files were separated in two groups: fixtures and tests themselves. Both groups are sorted alphabetically.
That seemed convenient to me while I was working on this script.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1710065883)
Source files were separated in two groups: fixtures and tests themselves. Both groups are sorted alphabetically.
That seemed convenient to me while I was working on this script.
💬 paplorinc commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1710012397)
I was also wondering, checked the actual execution for both paths (with & without `[[likely]]`), and if the benchmarks are accurate, AppleClang 15 doesn't seem to make any change in this case (yet?).
<details><summary>FeeFracEvaluate bench</summary>
> cmake --build build && ./build/src/bench/bench_bitcoin -filter='FeeFracEvaluate.*' --min-time=10000
// tried a few different ways of testing it, none of them showed any difference
```C++
#include <bench/bench.h>
#include <util/feefrac
...
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1710012397)
I was also wondering, checked the actual execution for both paths (with & without `[[likely]]`), and if the benchmarks are accurate, AppleClang 15 doesn't seem to make any change in this case (yet?).
<details><summary>FeeFracEvaluate bench</summary>
> cmake --build build && ./build/src/bench/bench_bitcoin -filter='FeeFracEvaluate.*' --min-time=10000
// tried a few different ways of testing it, none of them showed any difference
```C++
#include <bench/bench.h>
#include <util/feefrac
...
💬 paplorinc commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1710070948)
Checked the same with a simple logging to make sure the reproducer is representative:
```diff
diff --git a/src/arith_uint256.h b/src/arith_uint256.h
index d91689c3b3..0198edbbbe 100644
--- a/src/arith_uint256.h
+++ b/src/arith_uint256.h
@@ -12,6 +12,7 @@
#include <limits>
#include <stdexcept>
#include <string>
+#include <iostream>
class uint256;
@@ -212,7 +213,10 @@ public:
friend inline base_uint operator*(const base_uint& a, uint32_t b) { return base_uint(a) *= b; }
...
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1710070948)
Checked the same with a simple logging to make sure the reproducer is representative:
```diff
diff --git a/src/arith_uint256.h b/src/arith_uint256.h
index d91689c3b3..0198edbbbe 100644
--- a/src/arith_uint256.h
+++ b/src/arith_uint256.h
@@ -12,6 +12,7 @@
#include <limits>
#include <stdexcept>
#include <string>
+#include <iostream>
class uint256;
@@ -212,7 +213,10 @@ public:
friend inline base_uint operator*(const base_uint& a, uint32_t b) { return base_uint(a) *= b; }
...
💬 andrewtoth commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/30610#issuecomment-2276431786)
Concept ACK
Indeed, the `FlushStateMode::ALWAYS` variant is now confusing. It could mean we want to always write the chainstate to disk, or we want to always erase the dbcache. Splitting it up into the two cases makes sense.
(https://github.com/bitcoin/bitcoin/pull/30610#issuecomment-2276431786)
Concept ACK
Indeed, the `FlushStateMode::ALWAYS` variant is now confusing. It could mean we want to always write the chainstate to disk, or we want to always erase the dbcache. Splitting it up into the two cases makes sense.
💬 achow101 commented on pull request "C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-2276461122)
ACK 2925bd537cbd8c70594e23f6c4298b7101f7f73d
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-2276461122)
ACK 2925bd537cbd8c70594e23f6c4298b7101f7f73d
👍 hodlinator approved a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2228661985)
ACK 855784d3a0026414159acc42fceeb271f8a28133
Passed `make check`.
Passed ``RANDOM_CTX_SEED=1231231231231231231231231231231231231231231231231231231231312312 src/test/test_bitcoin``.
Failed with expected error when removing char or inserting non-hex char into `RANDOM_CTX_SEED` from above.
Thanks @stickies-v for humoring me regarding the error handling in **test/util/random.cpp**!
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2228661985)
ACK 855784d3a0026414159acc42fceeb271f8a28133
Passed `make check`.
Passed ``RANDOM_CTX_SEED=1231231231231231231231231231231231231231231231231231231231312312 src/test/test_bitcoin``.
Failed with expected error when removing char or inserting non-hex char into `RANDOM_CTX_SEED` from above.
Thanks @stickies-v for humoring me regarding the error handling in **test/util/random.cpp**!
💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1710130409)
nit: An `Assume()` or `Assert()` around the `FromHex()` result before the `*`-deref might be better here, even though it should work :tm:.
Maybe linking `TrySanitizeHexNumber` and `FromHex` together in the fuzz-test would be good for enforcing that the former only allows through something acceptable to the latter (although `result_size` must also be correct).
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1710130409)
nit: An `Assume()` or `Assert()` around the `FromHex()` result before the `*`-deref might be better here, even though it should work :tm:.
Maybe linking `TrySanitizeHexNumber` and `FromHex` together in the fuzz-test would be good for enforcing that the former only allows through something acceptable to the latter (although `result_size` must also be correct).
🤔 furszy reviewed a pull request: "descriptors: Be able to specify change and receiving in a single descriptor string"
(https://github.com/bitcoin/bitcoin/pull/22838#pullrequestreview-2228717838)
Code review ACK 6f8a7e0565393c9f0b85586fa65940df40900586
(https://github.com/bitcoin/bitcoin/pull/22838#pullrequestreview-2228717838)
Code review ACK 6f8a7e0565393c9f0b85586fa65940df40900586
💬 davidgumberg commented on pull request "lint: Find function calls in default arguments":
(https://github.com/bitcoin/bitcoin/pull/30553#discussion_r1710194796)
```suggestion
let files = check_output(
git()
.args(["ls-files", "--", "*.py"])
.args(get_pathspecs_exclude_subtrees())
)?;
```
nit: exclude subtrees from this check
(https://github.com/bitcoin/bitcoin/pull/30553#discussion_r1710194796)
```suggestion
let files = check_output(
git()
.args(["ls-files", "--", "*.py"])
.args(get_pathspecs_exclude_subtrees())
)?;
```
nit: exclude subtrees from this check
💬 zawy12 commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2276562644)
@murchandamus I was only interested in letting those calling it a fix to be aware that it's not a fix.
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2276562644)
@murchandamus I was only interested in letting those calling it a fix to be aware that it's not a fix.
💬 TheCharlatan commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2276568749)
Thanks for all the explanations @ryanofsky , I'll take some time to digest all of this, and read some more libmultiprocess code.
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2276568749)
Thanks for all the explanations @ryanofsky , I'll take some time to digest all of this, and read some more libmultiprocess code.
💬 fjahr commented on pull request "doc, chainparams: 29775 release notes and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/30604#discussion_r1710231533)
Sigh, I guess several people feel this way, so I have removed it now.
(https://github.com/bitcoin/bitcoin/pull/30604#discussion_r1710231533)
Sigh, I guess several people feel this way, so I have removed it now.
💬 fjahr commented on pull request "doc, chainparams: 29775 release notes and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/30604#discussion_r1710231933)
Automatically fixed since I have removed the version reference.
(https://github.com/bitcoin/bitcoin/pull/30604#discussion_r1710231933)
Automatically fixed since I have removed the version reference.
💬 fjahr commented on pull request "doc, chainparams: 29775 release notes and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/30604#issuecomment-2276588957)
Removed reference to a specific version in the deprecation warning and addressed [@Sjors comment](https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2275425772) by using the `MAX_FUTURE_BLOCK_TIME` explicitly as the timewarp defense delta.
(https://github.com/bitcoin/bitcoin/pull/30604#issuecomment-2276588957)
Removed reference to a specific version in the deprecation warning and addressed [@Sjors comment](https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2275425772) by using the `MAX_FUTURE_BLOCK_TIME` explicitly as the timewarp defense delta.
📝 whitslack opened a pull request: "Fixes for GCC 15 compatibility"
(https://github.com/bitcoin/bitcoin/pull/30612)
GCC 15 introduces three build failures:
* Two are related to missing includes. You can't use `uint16_t` et al. without including `<cstdint>`.
* The third is harder to understand but easy to fix. GCC changed something about the way templates are instantiated when checking type constraints, and now there is a dependency loop while checking `std::optional<CFeeRate>`. This manifests as the following compile-time mess:
```
In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/15/incl
...
(https://github.com/bitcoin/bitcoin/pull/30612)
GCC 15 introduces three build failures:
* Two are related to missing includes. You can't use `uint16_t` et al. without including `<cstdint>`.
* The third is harder to understand but easy to fix. GCC changed something about the way templates are instantiated when checking type constraints, and now there is a dependency loop while checking `std::optional<CFeeRate>`. This manifests as the following compile-time mess:
```
In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/15/incl
...
💬 fjahr commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2276589658)
> Followup suggestion: let's make `60 * 60 * 2` a constant and add a `static_assert` that it's equal to `MAX_FUTURE_BLOCK_TIME`. They are not the same thing, since `time-timewarp-attack` is (testnet4) consensus whereas `time-too-new` is not. But if we ever change one without changing the other, it can cause a chain split.
Well, sounds like we could also just use `MAX_FUTURE_BLOCK_TIME` there then. I have addressed this now in #30604.
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2276589658)
> Followup suggestion: let's make `60 * 60 * 2` a constant and add a `static_assert` that it's equal to `MAX_FUTURE_BLOCK_TIME`. They are not the same thing, since `time-timewarp-attack` is (testnet4) consensus whereas `time-too-new` is not. But if we ever change one without changing the other, it can cause a chain split.
Well, sounds like we could also just use `MAX_FUTURE_BLOCK_TIME` there then. I have addressed this now in #30604.
💬 davidgumberg commented on pull request "lint: Find function calls in default arguments":
(https://github.com/bitcoin/bitcoin/pull/30553#discussion_r1710263538)
nit:
```suggestion
"`{}` was not found in $PATH, skipping this check.",
```
(https://github.com/bitcoin/bitcoin/pull/30553#discussion_r1710263538)
nit:
```suggestion
"`{}` was not found in $PATH, skipping this check.",
```