💬 jonatack commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#issuecomment-2721564695)
> I like these cleanups, ans we should go a step further and modernize the code more when touching them. I'm also not a fan of the 3rd commit for the inheritance cases and would prefer seeing the last commit in a separate PR, doesn't seem like a "refactor:"
The last commit was added at the request of multiple reviewers, but I've removed "refactor" from the pull title. Addressed the other feedback individually and happy to look at any modernization follow-ups you'd like to propose.
(https://github.com/bitcoin/bitcoin/pull/31887#issuecomment-2721564695)
> I like these cleanups, ans we should go a step further and modernize the code more when touching them. I'm also not a fan of the 3rd commit for the inheritance cases and would prefer seeing the last commit in a separate PR, doesn't seem like a "refactor:"
The last commit was added at the request of multiple reviewers, but I've removed "refactor" from the pull title. Addressed the other feedback individually and happy to look at any modernization follow-ups you'd like to propose.
💬 Sjors commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2721565218)
Concept ACK (again)
It would be good to mention #25717 in the PR description.
Digging through older issues, I found a few references to the function of checkpoints.
https://github.com/bitcoin/bitcoin/issues/4512#issuecomment-48818097:
> With headers-first, checkpoint-based sigcheck skipping can be replaced by a "do not verify signatures when buried beneath enough PoW". We'll need a weaker form of checkpoints still to prevent the "fill your memory with a ton of low-difficulty headers
...
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2721565218)
Concept ACK (again)
It would be good to mention #25717 in the PR description.
Digging through older issues, I found a few references to the function of checkpoints.
https://github.com/bitcoin/bitcoin/issues/4512#issuecomment-48818097:
> With headers-first, checkpoint-based sigcheck skipping can be replaced by a "do not verify signatures when buried beneath enough PoW". We'll need a weaker form of checkpoints still to prevent the "fill your memory with a ton of low-difficulty headers
...
💬 jonatack commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993724292)
It was added at the request of multiple reviewers and doesn't seem worth splitting out to another PR for that one line and requiring an additional merge script run. Instead, I've dropped the word "refactor" from the PR title.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993724292)
It was added at the request of multiple reviewers and doesn't seem worth splitting out to another PR for that one line and requiring an additional merge script run. Instead, I've dropped the word "refactor" from the PR title.
💬 jonatack commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1993747780)
```suggestion
- It's preferable to avoid changing an RPC in a backward-incompatible manner, but in that case, add an associated `-deprecatedrpc=` option to retain previous RPC behavior during the deprecation period. Backward-incompatible changes include: data type changes (e.g. from `{"warnings":""}` to `{"warnings":[]}`, changing a value from a string to a number, etc.), logical meaning changes of a value, or key name changes (e.g. `{"warning":""}` to `{"warnings":""}`). Adding a key to an obj
...
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1993747780)
```suggestion
- It's preferable to avoid changing an RPC in a backward-incompatible manner, but in that case, add an associated `-deprecatedrpc=` option to retain previous RPC behavior during the deprecation period. Backward-incompatible changes include: data type changes (e.g. from `{"warnings":""}` to `{"warnings":[]}`, changing a value from a string to a number, etc.), logical meaning changes of a value, or key name changes (e.g. `{"warning":""}` to `{"warnings":""}`). Adding a key to an obj
...
👋 jonatack's pull request is ready for review: "p2p: protect addnode peers during IBD"
(https://github.com/bitcoin/bitcoin/pull/32051)
(https://github.com/bitcoin/bitcoin/pull/32051)
💬 mzumsande commented on issue "intermittent issue in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/issues/31700#issuecomment-2721637434)
> so not sure if a fix would conflict with that?
Haven't looked into this one yet, but it shouldn't really matter (#31829) if it conflicts or not - a fix should have priority, if possible be included in 29.0, so that we don't have a release with frequent intermittent timeouts.
(https://github.com/bitcoin/bitcoin/issues/31700#issuecomment-2721637434)
> so not sure if a fix would conflict with that?
Haven't looked into this one yet, but it shouldn't really matter (#31829) if it conflicts or not - a fix should have priority, if possible be included in 29.0, so that we don't have a release with frequent intermittent timeouts.
💬 marcofleon commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2721641157)
Rebased after 44041ae0eca9d2034b7c2bdef24724809cc35e90 and added https://github.com/bitcoin/bitcoin/pull/25717 in the description.
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2721641157)
Rebased after 44041ae0eca9d2034b7c2bdef24724809cc35e90 and added https://github.com/bitcoin/bitcoin/pull/25717 in the description.
💬 Sjors commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2721646233)
Code review ACK 3c5d1a468199722da620f1f3d8ae3319980a46d5
If #31974 lands then this PR will be more difficult to revert. So it might be marginally better to drop the checkpoints in a separate commit. Here's a [commit sequence](https://github.com/Sjors/bitcoin/commits/2025/03/checkpoint-removal/) that would work.
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2721646233)
Code review ACK 3c5d1a468199722da620f1f3d8ae3319980a46d5
If #31974 lands then this PR will be more difficult to revert. So it might be marginally better to drop the checkpoints in a separate commit. Here's a [commit sequence](https://github.com/Sjors/bitcoin/commits/2025/03/checkpoint-removal/) that would work.
💬 Sjors commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1993774077)
The two different `MiniWallet` instances can't spend each others coins, so I don't think these additions are needed - but useful to keep in mind.
(https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1993774077)
The two different `MiniWallet` instances can't spend each others coins, so I don't think these additions are needed - but useful to keep in mind.
💬 jonatack commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2721678195)
Ready for review.
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2721678195)
Ready for review.
💬 janb84 commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2721686404)
> > @brunoerg @janb84 @Prabhat1308 I expect all of you were running into the issue on macOS? I probably can't fix it myself, but I'd investigate whether the last commit is working:
> >
> > * Is your result different with or without the last commit?
> > * Does `ResetCoverageCounters` work?
> > * What is the coverage result for `src/test/util/setup_common.cpp`, especially `g_rng_temp_path_init`? (You can find it in `fuzz_det_cov.show.{run_id}.txt`)
>
> In checking this issue I discovered t
...
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2721686404)
> > @brunoerg @janb84 @Prabhat1308 I expect all of you were running into the issue on macOS? I probably can't fix it myself, but I'd investigate whether the last commit is working:
> >
> > * Is your result different with or without the last commit?
> > * Does `ResetCoverageCounters` work?
> > * What is the coverage result for `src/test/util/setup_common.cpp`, especially `g_rng_temp_path_init`? (You can find it in `fuzz_det_cov.show.{run_id}.txt`)
>
> In checking this issue I discovered t
...
💬 hodlinator commented on pull request "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets":
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2721696648)
re-ACK 1f9b2e150ce5aa192226d2daae73f7d678c47d65
We're back to an exact copy of 0ecd2e0748bdbfc12af1fc100862a1a6f046c2c9, only rebased, which I [previously ACKed](https://github.com/bitcoin/bitcoin/pull/32019#pullrequestreview-2670874014).
If you retouch and have remaining patience I would still prefer:
```
Error: NSIS not found
Satisfy the dependency and re-run CMake to regenerate the build system.
```
Tried to figure out a way to get `cmake` to return an error code after printing t
...
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2721696648)
re-ACK 1f9b2e150ce5aa192226d2daae73f7d678c47d65
We're back to an exact copy of 0ecd2e0748bdbfc12af1fc100862a1a6f046c2c9, only rebased, which I [previously ACKed](https://github.com/bitcoin/bitcoin/pull/32019#pullrequestreview-2670874014).
If you retouch and have remaining patience I would still prefer:
```
Error: NSIS not found
Satisfy the dependency and re-run CMake to regenerate the build system.
```
Tried to figure out a way to get `cmake` to return an error code after printing t
...
💬 jonatack commented on pull request "[IBD] Tracking PR for speeding up Initial Block Download":
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-2721748181)
Concept ACK, thank you for opening this.
I currently am in an environment of slow internet speed, where IBD is slower than 2 orders of magnitude worse than the times in the OP.
Opened #32051 today to address an issue I'm also seeing of very frequent disconnections+reconnections of trusted addnode peers during IBD.
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-2721748181)
Concept ACK, thank you for opening this.
I currently am in an environment of slow internet speed, where IBD is slower than 2 orders of magnitude worse than the times in the OP.
Opened #32051 today to address an issue I'm also seeing of very frequent disconnections+reconnections of trusted addnode peers during IBD.
💬 brunoerg commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2721777570)
> Word of caution, i'm running in a nix-shell on a mac. So would like to see someone on pure mac env. confirm that this is the issue
Confirmed. I still have the issue on master but I just tested with the following diff and seems to work:
```diff
diff --git a/src/test/util/coverage.cpp b/src/test/util/coverage.cpp
index bbf068a6fa..ac0fb00e36 100644
--- a/src/test/util/coverage.cpp
+++ b/src/test/util/coverage.cpp
@@ -4,7 +4,7 @@
#include <test/util/coverage.h>
-#if defined(__cl
...
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2721777570)
> Word of caution, i'm running in a nix-shell on a mac. So would like to see someone on pure mac env. confirm that this is the issue
Confirmed. I still have the issue on master but I just tested with the following diff and seems to work:
```diff
diff --git a/src/test/util/coverage.cpp b/src/test/util/coverage.cpp
index bbf068a6fa..ac0fb00e36 100644
--- a/src/test/util/coverage.cpp
+++ b/src/test/util/coverage.cpp
@@ -4,7 +4,7 @@
#include <test/util/coverage.h>
-#if defined(__cl
...
👍 TheCharlatan approved a pull request: "versionbits refactoring"
(https://github.com/bitcoin/bitcoin/pull/29039#pullrequestreview-2682297269)
ACK 7565563bc7a5bb98ebf03a7d6881912a74d3f302
(https://github.com/bitcoin/bitcoin/pull/29039#pullrequestreview-2682297269)
ACK 7565563bc7a5bb98ebf03a7d6881912a74d3f302
💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1993721926)
nit: Since corecheck is reporting `IsActiveAfter` as not being covered, how about (though not very sure the checks make a lot of sense there):
```diff
diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp
index bcacac025d..3218ed0811 100644
--- a/src/test/versionbits_tests.cpp
+++ b/src/test/versionbits_tests.cpp
@@ -407,11 +407,13 @@ void check_computeblockversion(VersionBitsCache& versionbitscache, const Consens
// check signalling continues while min_act
...
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1993721926)
nit: Since corecheck is reporting `IsActiveAfter` as not being covered, how about (though not very sure the checks make a lot of sense there):
```diff
diff --git a/src/test/versionbits_tests.cpp b/src/test/versionbits_tests.cpp
index bcacac025d..3218ed0811 100644
--- a/src/test/versionbits_tests.cpp
+++ b/src/test/versionbits_tests.cpp
@@ -407,11 +407,13 @@ void check_computeblockversion(VersionBitsCache& versionbitscache, const Consens
// check signalling continues while min_act
...
💬 ryanofsky commented on pull request "test: avoid treating hash results as integers (part 1)":
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2721796758)
Changes in this PR seem reasonable to me although I don't agree with premise that it is an improvement to treat hashes as blobs, but then then arbitrarily decide to display those blobs as reverse-byte hex strings, when there is probably not other software that displays blobs as reverse-byte hex strings.
Status quo where hashes are simply interpreted as little endian numbers, and those numbers are represented as hex strings in the standard way python and other software represents numbers repre
...
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2721796758)
Changes in this PR seem reasonable to me although I don't agree with premise that it is an improvement to treat hashes as blobs, but then then arbitrarily decide to display those blobs as reverse-byte hex strings, when there is probably not other software that displays blobs as reverse-byte hex strings.
Status quo where hashes are simply interpreted as little endian numbers, and those numbers are represented as hex strings in the standard way python and other software represents numbers repre
...
💬 Prabhat1308 commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2721800574)
> Word of caution, i'm running in a nix-shell on a mac. So would like to see someone on pure mac env. confirm that this is the issue
Applied this change and I get deterministic results on all runs
```
#if defined(__clang__) && (defined(__linux__) || defined(__APPLE__))
```
<img width="1512" alt="Screenshot 2025-03-13 at 9 31 40 PM" src="https://github.com/user-attachments/assets/fb622440-8634-47ba-b21a-54fc567803df" />
> Confirmed. I still have the issue on master but I just tested
...
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2721800574)
> Word of caution, i'm running in a nix-shell on a mac. So would like to see someone on pure mac env. confirm that this is the issue
Applied this change and I get deterministic results on all runs
```
#if defined(__clang__) && (defined(__linux__) || defined(__APPLE__))
```
<img width="1512" alt="Screenshot 2025-03-13 at 9 31 40 PM" src="https://github.com/user-attachments/assets/fb622440-8634-47ba-b21a-54fc567803df" />
> Confirmed. I still have the issue on master but I just tested
...
🤔 jonatack reviewed a pull request: "[IBD] flush UXTOs in bigger batches"
(https://github.com/bitcoin/bitcoin/pull/31645#pullrequestreview-2682568821)
Concept ACK, testing
(https://github.com/bitcoin/bitcoin/pull/31645#pullrequestreview-2682568821)
Concept ACK, testing
👍 l0rinc approved a pull request: "CLI cleanups"
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2682594542)
I would prefer doing the modernizations here to get rid of more technical debt, but this is already better than before so:
ACK d423fd9ec83ae323ac29bdc1b677ed8260bd59c4
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2682594542)
I would prefer doing the modernizations here to get rid of more technical debt, but this is already better than before so:
ACK d423fd9ec83ae323ac29bdc1b677ed8260bd59c4
💬 l0rinc commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993873254)
the problem is likely that the method return `int8_t` for an index - unnecessary, should likely be `size_t` or `int` - not important
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993873254)
the problem is likely that the method return `int8_t` for an index - unnecessary, should likely be `size_t` or `int` - not important