💬 Prabhat1308 commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#issuecomment-2719819873)
Are the files mentioned in `files_to_delete` and `files_to_perturb` just for informational purposes that they don't trigger an error or you want to indicate that these files should trigger an error but currently they don't.
(https://github.com/bitcoin/bitcoin/pull/32010#issuecomment-2719819873)
Are the files mentioned in `files_to_delete` and `files_to_perturb` just for informational purposes that they don't trigger an error or you want to indicate that these files should trigger an error but currently they don't.
🤔 glozow reviewed a pull request: "contrib: Fix `gen-bitcoin-conf.sh`"
(https://github.com/bitcoin/bitcoin/pull/32049#pullrequestreview-2680493978)
ACK a24419f8bed5e1145ce171dbbdad957750585471
(https://github.com/bitcoin/bitcoin/pull/32049#pullrequestreview-2680493978)
ACK a24419f8bed5e1145ce171dbbdad957750585471
🚀 fanquake merged a pull request: "contrib: Add deterministic-unittest-coverage"
(https://github.com/bitcoin/bitcoin/pull/31901)
(https://github.com/bitcoin/bitcoin/pull/31901)
🤔 furszy reviewed a pull request: "removed duplicate call to GetDescriptorScriptPubKeyMan"
(https://github.com/bitcoin/bitcoin/pull/32023#pullrequestreview-2680558849)
Please try to avoid introducing and reverting changes within the same PR. The first two commits should be squashed. Aside from that, I left you a comment suggesting a possible alternative implementation.
(https://github.com/bitcoin/bitcoin/pull/32023#pullrequestreview-2680558849)
Please try to avoid introducing and reverting changes within the same PR. The first two commits should be squashed. Aside from that, I left you a comment suggesting a possible alternative implementation.
💬 furszy commented on pull request "removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1992789597)
What about returning `util::Result<void>` instead and return the error message here? (see how we do it in other places).
Also, if you're going down this route, you should check any other usages of this function, as it would no longer be throwing an exception.
And you would also have to return an `util::Result<ScriptPubKeyMan*>` in `AddWalletDescriptor` too. Which I think that is cleaner than catching the specific exception.
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1992789597)
What about returning `util::Result<void>` instead and return the error message here? (see how we do it in other places).
Also, if you're going down this route, you should check any other usages of this function, as it would no longer be throwing an exception.
And you would also have to return an `util::Result<ScriptPubKeyMan*>` in `AddWalletDescriptor` too. Which I think that is cleaner than catching the specific exception.
💬 davidrobinsonau commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1992796261)
Is there a use case for someone to pass a timestamp prior to January 1, 1970? Which would be a negative number, causing bitcoind not to rescan when it was expected to.
Alternatively, the code could be updated to only look for "-1", meaning anything below that could still be a full rescan.
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1992796261)
Is there a use case for someone to pass a timestamp prior to January 1, 1970? Which would be a negative number, causing bitcoind not to rescan when it was expected to.
Alternatively, the code could be updated to only look for "-1", meaning anything below that could still be a full rescan.
⚠️ fanquake opened an issue: "v29.0 Testing"
(https://github.com/bitcoin/bitcoin/issues/32052)
Umbrella issue for 29.0 testing. Please help testing on a wide variety of supported platforms, as well as interaction with different software.
Let us know which version you tested on which operating system.
If you find an issue, please search Github for known issues first and then open a new Github issue.
This meta issue should not be used to report bugs, as a single thread makes it impossible to track more than one topic.
See [29.0 Release Notes Draft](https://github.com/bitcoin-core/bitcoi
...
(https://github.com/bitcoin/bitcoin/issues/32052)
Umbrella issue for 29.0 testing. Please help testing on a wide variety of supported platforms, as well as interaction with different software.
Let us know which version you tested on which operating system.
If you find an issue, please search Github for known issues first and then open a new Github issue.
This meta issue should not be used to report bugs, as a single thread makes it impossible to track more than one topic.
See [29.0 Release Notes Draft](https://github.com/bitcoin-core/bitcoi
...
⚠️ fanquake pinned an issue: "v29.0 Testing"
(https://github.com/bitcoin/bitcoin/issues/32052)
Umbrella issue for 29.0 testing. Please help testing on a wide variety of supported platforms, as well as interaction with different software.
Let us know which version you tested on which operating system.
If you find an issue, please search Github for known issues first and then open a new Github issue.
This meta issue should not be used to report bugs, as a single thread makes it impossible to track more than one topic.
See [29.0 Release Notes Draft](https://github.com/bitcoin-core/bitcoi
...
(https://github.com/bitcoin/bitcoin/issues/32052)
Umbrella issue for 29.0 testing. Please help testing on a wide variety of supported platforms, as well as interaction with different software.
Let us know which version you tested on which operating system.
If you find an issue, please search Github for known issues first and then open a new Github issue.
This meta issue should not be used to report bugs, as a single thread makes it impossible to track more than one topic.
See [29.0 Release Notes Draft](https://github.com/bitcoin-core/bitcoi
...
💬 fanquake commented on issue "Release Schedule for 29.0":
(https://github.com/bitcoin/bitcoin/issues/31029#issuecomment-2720068615)
`rc1` has been tagged: https://github.com/bitcoin/bitcoin/releases/tag/v29.0rc1.
(https://github.com/bitcoin/bitcoin/issues/31029#issuecomment-2720068615)
`rc1` has been tagged: https://github.com/bitcoin/bitcoin/releases/tag/v29.0rc1.
💬 ajtowns commented on pull request "[IBD] Tracking PR for speeding up Initial Block Download":
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-2720097064)
> Plotting the performance of the blocks from the produced `debug.log` files shows (from the last run, can differ slightly from the normalized average shown below) the effect of each commit:
Wouldn't these plots be easier to read with block height on the x-axis and time on the y-axis, giving a consistent domain (each case goes from height 0 to 880k or so) with a simple "lower is better" comparison? (rather than "further left is better")
Plotting the difference between the various proposed
...
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-2720097064)
> Plotting the performance of the blocks from the produced `debug.log` files shows (from the last run, can differ slightly from the normalized average shown below) the effect of each commit:
Wouldn't these plots be easier to read with block height on the x-axis and time on the y-axis, giving a consistent domain (each case goes from height 0 to 880k or so) with a simple "lower is better" comparison? (rather than "further left is better")
Plotting the difference between the various proposed
...
💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2720322464)
@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`)
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2720322464)
@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`)
⚠️ Sjors opened an issue: "Add test coverage for getblocktemplate fees"
(https://github.com/bitcoin/bitcoin/issues/32053)
Suggested in https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1992245751
There's probably other aspects of `getblocktemplate` and the underlying template construction methods that coverage.
Related:
- #31981 adds coverage for the `proposal` mode
(https://github.com/bitcoin/bitcoin/issues/32053)
Suggested in https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1992245751
There's probably other aspects of `getblocktemplate` and the underlying template construction methods that coverage.
Related:
- #31981 adds coverage for the `proposal` mode
💬 Sjors commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1992988592)
Tracking in #32053.
(https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1992988592)
Tracking in #32053.
💬 maflcko commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1992994451)
It has been permitted previously and this commit is silently breaking it. If you want to use -1, it seems better to properly sanitize the user input first, or use a different approach.
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1992994451)
It has been permitted previously and this commit is silently breaking it. If you want to use -1, it seems better to properly sanitize the user input first, or use a different approach.
💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#issuecomment-2720368885)
Rebased after #31283.
(https://github.com/bitcoin/bitcoin/pull/31981#issuecomment-2720368885)
Rebased after #31283.
💬 maflcko commented on pull request "build: align debugging flags to `-O0`":
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2720373606)
no opinion about the build changes here, but about the CI changes I slightly tend toward NACK, as per my previous comment https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2685140630
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2720373606)
no opinion about the build changes here, but about the CI changes I slightly tend toward NACK, as per my previous comment https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2685140630
💬 fanquake commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2720395222)
> it should be the intersection of what's needed for ... Sv2
> Maybe also worth considering is to have a common library for Sv2
As far as I'm aware, nothing SV2 related is going into Core (aside from the mining interface), and we aren't planning on refactoring P2P code so it could be copy-pasted out/reused in some other SV2 utility, so, SV2 shouldn't be a motivating usecase here? It looks like #30694 could also be updated to remove any reference to SV2 as motivation (i.e "Doing Stratum V2")
...
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2720395222)
> it should be the intersection of what's needed for ... Sv2
> Maybe also worth considering is to have a common library for Sv2
As far as I'm aware, nothing SV2 related is going into Core (aside from the mining interface), and we aren't planning on refactoring P2P code so it could be copy-pasted out/reused in some other SV2 utility, so, SV2 shouldn't be a motivating usecase here? It looks like #30694 could also be updated to remove any reference to SV2 as motivation (i.e "Doing Stratum V2")
...
💬 maflcko commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1993037907)
Why change to `Debug` when there is no reason? The docs literally say in the first line:
> This document explains how to use clang’s source-based code coverage feature. It’s called “source-based” because it operates on AST and preprocessor information directly. This allows it to generate very precise coverage data.
https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#introduction
If it doesn't work, at a minimum, you should provide exact steps to reproduce, not add workarounds for b
...
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1993037907)
Why change to `Debug` when there is no reason? The docs literally say in the first line:
> This document explains how to use clang’s source-based code coverage feature. It’s called “source-based” because it operates on AST and preprocessor information directly. This allows it to generate very precise coverage data.
https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#introduction
If it doesn't work, at a minimum, you should provide exact steps to reproduce, not add workarounds for b
...
💬 laanwj commented on pull request "depends: remove `NO_HARDEN` option":
(https://github.com/bitcoin/bitcoin/pull/32038#issuecomment-2720530698)
> Would it make sense to also drop ENABLE_HARDENING? Currently set OFF in the clang-tidy ci job, but I [flipped it ON](https://github.com/davidgumberg/bitcoin/commit/ce79bea14ae1427395b7eddb0a0e1c21fa78516d) and ran the clang-tidy job locally and it seems to work. (But I have no context for why hardening was disabled in clang-tidy cc: @maflcko)
Right, if it's needed for some of our own tooling/testing that might be a valid reason to keep it, but otherwise, nah.
And even then cmake provides
...
(https://github.com/bitcoin/bitcoin/pull/32038#issuecomment-2720530698)
> Would it make sense to also drop ENABLE_HARDENING? Currently set OFF in the clang-tidy ci job, but I [flipped it ON](https://github.com/davidgumberg/bitcoin/commit/ce79bea14ae1427395b7eddb0a0e1c21fa78516d) and ran the clang-tidy job locally and it seems to work. (But I have no context for why hardening was disabled in clang-tidy cc: @maflcko)
Right, if it's needed for some of our own tooling/testing that might be a valid reason to keep it, but otherwise, nah.
And even then cmake provides
...
💬 l0rinc commented on pull request "[IBD] Tracking PR for speeding up Initial Block Download":
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-2720533816)
> with a simple "lower is better" comparison
I like the idea, updated the description and the code:
> Plotting the difference between the various proposed commits and the baseline (time_baseline[height] / time_commit_X[height], higher is better) might also be helpful?
Something like this? Conceptually seems useful, but here I don't know how to interpret it, seems too far zoomed in
<img width="10
...
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-2720533816)
> with a simple "lower is better" comparison
I like the idea, updated the description and the code:
> Plotting the difference between the various proposed commits and the baseline (time_baseline[height] / time_commit_X[height], higher is better) might also be helpful?
Something like this? Conceptually seems useful, but here I don't know how to interpret it, seems too far zoomed in
<img width="10
...