π¬ mabu44 commented on pull request "[WIP] refactor: migrate unit tests to Google Test":
(https://github.com/bitcoin/bitcoin/pull/31988#discussion_r1982998479)
This instruction is the reason I needed Google Mock. Otherwise, I could have avoided such a dependency in the POC and checked with ASSERT_EQ on size and on items.
(https://github.com/bitcoin/bitcoin/pull/31988#discussion_r1982998479)
This instruction is the reason I needed Google Mock. Otherwise, I could have avoided such a dependency in the POC and checked with ASSERT_EQ on size and on items.
π¬ vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2703309315)
Or could pass an additional argument to `waitNext()`: `CThreadInterrupt` and have `waitNext()` honor that one in addition to `chainman().m_interrupt`.
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2703309315)
Or could pass an additional argument to `waitNext()`: `CThreadInterrupt` and have `waitNext()` honor that one in addition to `chainman().m_interrupt`.
π¬ Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2703359479)
The problem is a bit less urgent now, since I refactored the Template Provider to have one thread per connection. Worst case this thread lingers for half an hour until a new block arrives. It only becomes a potential DoS problem for public facing template providers, but that use case needs additional safety and performance changes anyway.
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2703359479)
The problem is a bit less urgent now, since I refactored the Template Provider to have one thread per connection. Worst case this thread lingers for half an hour until a new block arrives. It only becomes a potential DoS problem for public facing template providers, but that use case needs additional safety and performance changes anyway.
π fanquake's pull request is ready for review: "doc: remove note about macOS self-signing"
(https://github.com/bitcoin/bitcoin/pull/32003)
(https://github.com/bitcoin/bitcoin/pull/32003)
π¬ hodlinator commented on pull request "[WIP] refactor: migrate unit tests to Google Test":
(https://github.com/bitcoin/bitcoin/pull/31988#issuecomment-2703406538)
> Yes, this was a draft PR before being closed.
Seems a bit harsh to me in that case. I understand that hebasto, fanquake & others have put time into carrying over Boost.Test from autotools into CMake recently, so not eager to have that work replaced. Getting Boost out of the production code is a higher priority than getting it out of test code, but I still think it is valid to work on.
> Right, but still valid if test_bitcoin is run directly, for instance because you need to provide custo
...
(https://github.com/bitcoin/bitcoin/pull/31988#issuecomment-2703406538)
> Yes, this was a draft PR before being closed.
Seems a bit harsh to me in that case. I understand that hebasto, fanquake & others have put time into carrying over Boost.Test from autotools into CMake recently, so not eager to have that work replaced. Getting Boost out of the production code is a higher priority than getting it out of test code, but I still think it is valid to work on.
> Right, but still valid if test_bitcoin is run directly, for instance because you need to provide custo
...
π fanquake merged a pull request: "kernel: pre-29.x chainparams and headerssync update"
(https://github.com/bitcoin/bitcoin/pull/31978)
(https://github.com/bitcoin/bitcoin/pull/31978)
π¬ Sjors commented on pull request "ci: use LLVM 20.1.0 for MSAN":
(https://github.com/bitcoin/bitcoin/pull/31993#issuecomment-2703425241)
ACK d76647eb8f11708ae90600ce3b92570a86f62bac
Seems obviously useful and CI passes. Two hundred commits between rc1 and the final version is impressive: https://github.com/llvm/llvm-project/compare/llvmorg-20.1.0-rc1...llvmorg-20.1.0
(https://github.com/bitcoin/bitcoin/pull/31993#issuecomment-2703425241)
ACK d76647eb8f11708ae90600ce3b92570a86f62bac
Seems obviously useful and CI passes. Two hundred commits between rc1 and the final version is impressive: https://github.com/llvm/llvm-project/compare/llvmorg-20.1.0-rc1...llvmorg-20.1.0
π¬ Sjors commented on pull request "docs: fix typos":
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1983101490)
Makes sense: https://www.utwente.nl/en/language-centre/translation-editing-services/english-styleguide/abbreviations/#abbreviating-dutch-diplomas-and-degrees-in-english
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1983101490)
Makes sense: https://www.utwente.nl/en/language-centre/translation-editing-services/english-styleguide/abbreviations/#abbreviating-dutch-diplomas-and-degrees-in-english
π¬ davidrobinsonau commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#issuecomment-2703469691)
Cloned fresh bitcoin src, then "gh pr checkout 31668" and built on Ubuntu WSL2.
Confirmed importdescriptors with "timestamp":"now" causes "RescanFromTime: Rescanning last 16 blocks".
Confirmed importdescriptors with "timestamp":"never" returns in less then a second and no messages about rescanning blocks on bitcoind console.
Thank you!
(https://github.com/bitcoin/bitcoin/pull/31668#issuecomment-2703469691)
Cloned fresh bitcoin src, then "gh pr checkout 31668" and built on Ubuntu WSL2.
Confirmed importdescriptors with "timestamp":"now" causes "RescanFromTime: Rescanning last 16 blocks".
Confirmed importdescriptors with "timestamp":"never" returns in less then a second and no messages about rescanning blocks on bitcoind console.
Thank you!
β οΈ fanquake opened an issue: "build: cmake --install fails after --target deploy"
(https://github.com/bitcoin/bitcoin/issues/32007)
Somewhat similar to #31745. If you build `--target deploy` and then try and `cmake --install`, it will fail:
```bash
cmake -B build --toolchain /root/ci_scratch/depends/x86_64-w64-mingw32/toolchain.cmake -DCMAKE_INSTALL_PREFIX=/root/testing
cmake --build build --target deploy
cmake --install build
-- Install configuration: "RelWithDebInfo"
-- Installing: /root/testing/bin/bitcoin-wallet.exe
-- Installing: /root/testing/share/man/man1/bitcoin-wallet.1
-- Installing: /root/testing/bin/bitcoind.ex
...
(https://github.com/bitcoin/bitcoin/issues/32007)
Somewhat similar to #31745. If you build `--target deploy` and then try and `cmake --install`, it will fail:
```bash
cmake -B build --toolchain /root/ci_scratch/depends/x86_64-w64-mingw32/toolchain.cmake -DCMAKE_INSTALL_PREFIX=/root/testing
cmake --build build --target deploy
cmake --install build
-- Install configuration: "RelWithDebInfo"
-- Installing: /root/testing/bin/bitcoin-wallet.exe
-- Installing: /root/testing/share/man/man1/bitcoin-wallet.1
-- Installing: /root/testing/bin/bitcoind.ex
...
π¬ Crypt-iQ commented on issue "p2p: lingering entries in `mapBlockSource`":
(https://github.com/bitcoin/bitcoin/issues/29410#issuecomment-2703525661)
The disconnect issue appears to have been fixed in https://github.com/bitcoin/bitcoin/commit/49257c0304828a185c273fcb99742c54bbef0c8e by no longer processing mutated blocks. The disconnect issue was working in a functional test when I originally opened the PR.
(https://github.com/bitcoin/bitcoin/issues/29410#issuecomment-2703525661)
The disconnect issue appears to have been fixed in https://github.com/bitcoin/bitcoin/commit/49257c0304828a185c273fcb99742c54bbef0c8e by no longer processing mutated blocks. The disconnect issue was working in a functional test when I originally opened the PR.
π¬ suiyuan1314 commented on pull request "docs: fix typos":
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1983169197)
thanks for sharing thisοΌ
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1983169197)
thanks for sharing thisοΌ
π¬ pinheadmz commented on pull request "docs: fix typos":
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1983172634)
I looked this up also but have no strong opinion. It could also be read "a remote procedure call" which would make the original correct
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1983172634)
I looked this up also but have no strong opinion. It could also be read "a remote procedure call" which would make the original correct
π¬ Sjors commented on pull request "docs: fix typos":
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1983205795)
@pinheadmz no, a/an always refers to the "RPC" acronym, not to its meaning. It only depends on how you pronounce the letter RPC, and specifically on how you pronounce "R" as a standalone letter, rather than as part of a word. This varies by accent. A pirate would say "an Ahrrr PC".
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r1983205795)
@pinheadmz no, a/an always refers to the "RPC" acronym, not to its meaning. It only depends on how you pronounce the letter RPC, and specifically on how you pronounce "R" as a standalone letter, rather than as part of a word. This varies by accent. A pirate would say "an Ahrrr PC".
π¬ mabu44 commented on pull request "[WIP] refactor: migrate unit tests to Google Test":
(https://github.com/bitcoin/bitcoin/pull/31988#discussion_r1983210720)
It seems that Bitcoin defines an Assert macro in src/util/check.h, while GMock has an Assert inline function definition. If you try to move the inclusion of GMock at the end of the other include instructions, the preprocessor will try to use the Assert macro to change the code of gmock-internal-utils.h and fail in doing so because of a different number of parameters.
(https://github.com/bitcoin/bitcoin/pull/31988#discussion_r1983210720)
It seems that Bitcoin defines an Assert macro in src/util/check.h, while GMock has an Assert inline function definition. If you try to move the inclusion of GMock at the end of the other include instructions, the preprocessor will try to use the Assert macro to change the code of gmock-internal-utils.h and fail in doing so because of a different number of parameters.
π¬ vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2703769420)
@theuni, thanks for looking into this! I find your feedback constructive.
If we would have different parts of the code listen for incoming connections, accept them, read and write data to them, then there is bound to be some code duplication in that around the low level sockets handling. `SockMan` aims to handle that code duplication. It is an in-house minimalistic version of a generic IO library, not a feature rich competitor to them. Something like [PCP](https://github.com/bitcoin/bitcoin/p
...
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2703769420)
@theuni, thanks for looking into this! I find your feedback constructive.
If we would have different parts of the code listen for incoming connections, accept them, read and write data to them, then there is bound to be some code duplication in that around the low level sockets handling. `SockMan` aims to handle that code duplication. It is an in-house minimalistic version of a generic IO library, not a feature rich competitor to them. Something like [PCP](https://github.com/bitcoin/bitcoin/p
...
π¬ fanquake commented on issue "[rfc] build: Reject unclean configure?":
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2703801270)
> reliable steps to reproduce those issues.
For example, configuring with GCC + thread sanitizer (where libtsan is missing). After installing the missing dependency, and re-running CMake, even though the check is being re-run, CMake doesn't "fix" itself, and requires removing the build directory entirely. i.e:
```bash
cmake -B build -DSANITIZERS=thread
-- The CXX compiler identification is GNU 14.2.1
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for work
...
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2703801270)
> reliable steps to reproduce those issues.
For example, configuring with GCC + thread sanitizer (where libtsan is missing). After installing the missing dependency, and re-running CMake, even though the check is being re-run, CMake doesn't "fix" itself, and requires removing the build directory entirely. i.e:
```bash
cmake -B build -DSANITIZERS=thread
-- The CXX compiler identification is GNU 14.2.1
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for work
...
π¬ brunoerg commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1983332561)
56e9bef655673becb3e265948143a9e860a5c68b: nit: could also update the example in `test_runner.py`:
```py
for exclude_test in exclude_tests:
# A space in the name indicates it has arguments such as "wallet_basic.py --descriptors"
if ' ' in exclude_test:
```
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1983332561)
56e9bef655673becb3e265948143a9e860a5c68b: nit: could also update the example in `test_runner.py`:
```py
for exclude_test in exclude_tests:
# A space in the name indicates it has arguments such as "wallet_basic.py --descriptors"
if ' ' in exclude_test:
```
π€ rkrux reviewed a pull request: "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`"
(https://github.com/bitcoin/bitcoin/pull/31243#pullrequestreview-2664201741)
Few initial comments, I would like to spend more time on this PR to complete the review.
(https://github.com/bitcoin/bitcoin/pull/31243#pullrequestreview-2664201741)
Few initial comments, I would like to spend more time on this PR to complete the review.
π¬ rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1983212714)
Now that `out` is not being used in this function.
```diff
-std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, Span<const CScript>, FlatSigningProvider& out) const override
+std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, Span<const CScript>, FlatSigningProvider&) const override
```
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1983212714)
Now that `out` is not being used in this function.
```diff
-std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, Span<const CScript>, FlatSigningProvider& out) const override
+std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, Span<const CScript>, FlatSigningProvider&) const override
```