💬 theStack commented on pull request "test: avoid treating hash results as integers (part 1)":
(https://github.com/bitcoin/bitcoin/pull/32050#discussion_r1993663031)
Agree. My gut-feeling preference would be to remove the caching both for txid and wtxids, as I guess it doesn't make a notable performance difference (likely most of the time in functional tests is spent waiting for RPC results), but have to verify that assumption first.
(https://github.com/bitcoin/bitcoin/pull/32050#discussion_r1993663031)
Agree. My gut-feeling preference would be to remove the caching both for txid and wtxids, as I guess it doesn't make a notable performance difference (likely most of the time in functional tests is spent waiting for RPC results), but have to verify that assumption first.
💬 jonatack commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993676044)
Not sure but I think this would cause a conversion from size_t to int8_t on each iteration, rather than once for `return i`. Doesn't seem worth it.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993676044)
Not sure but I think this would cause a conversion from size_t to int8_t on each iteration, rather than once for `return i`. Doesn't seem worth it.
🤔 rkrux reviewed a pull request: "test: add missing segwitv1 test cases to `script_standard_tests`"
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2682207939)
Concept ACK a3a4d199e298a76725d0c0424195ae54c7e95fe0
I'm in favour of adding these tests. I've not reviewed the PR in detail but only glanced as of now. I'd be quick to complete the review once few of the outstanding feedback is addressed. Specially this one: https://github.com/bitcoin/bitcoin/pull/31340#discussion_r1856607483, this https://github.com/bitcoin/bitcoin/pull/31340#discussion_r1858636182 has a few interesting ideas as well.
Re: https://github.com/bitcoin/bitcoin/pull/31340#pul
...
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2682207939)
Concept ACK a3a4d199e298a76725d0c0424195ae54c7e95fe0
I'm in favour of adding these tests. I've not reviewed the PR in detail but only glanced as of now. I'd be quick to complete the review once few of the outstanding feedback is addressed. Specially this one: https://github.com/bitcoin/bitcoin/pull/31340#discussion_r1856607483, this https://github.com/bitcoin/bitcoin/pull/31340#discussion_r1858636182 has a few interesting ideas as well.
Re: https://github.com/bitcoin/bitcoin/pull/31340#pul
...
💬 mabu44 commented on pull request "contrib: Fix `gen-bitcoin-conf.sh`":
(https://github.com/bitcoin/bitcoin/pull/32049#issuecomment-2721511568)
Tested ACK a24419f8bed5e1145ce171dbbdad957750585471
(https://github.com/bitcoin/bitcoin/pull/32049#issuecomment-2721511568)
Tested ACK a24419f8bed5e1145ce171dbbdad957750585471
💬 jonatack commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993689355)
Yes, IIRC `std::optional` wasn't available when this was added, as the codebase was using C++11 at that time. I think this would be more of a modernization rewrite than a cleanup, but happy to review if you want to propose it.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993689355)
Yes, IIRC `std::optional` wasn't available when this was added, as the codebase was using C++11 at that time. I think this would be more of a modernization rewrite than a cleanup, but happy to review if you want to propose it.
💬 jonatack commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993710323)
If I'm understanding the comment ("simple structs are fine"), it comes from the idea of structs being for POD, say, in C. In C++, classes and structs are identical except for their default inheritance and member access levels, and for the use cases here, a struct appears to be the more appropriate choice that allows simpler, cleaner code (see also [stackoverflow.com/a/46339933](https://stackoverflow.com/a/46339933). Struct inheritance is also fine (see discussions like [stackoverflow.com/questio
...
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993710323)
If I'm understanding the comment ("simple structs are fine"), it comes from the idea of structs being for POD, say, in C. In C++, classes and structs are identical except for their default inheritance and member access levels, and for the use cases here, a struct appears to be the more appropriate choice that allows simpler, cleaner code (see also [stackoverflow.com/a/46339933](https://stackoverflow.com/a/46339933). Struct inheritance is also fine (see discussions like [stackoverflow.com/questio
...
💬 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
...