👍 ryanofsky approved a pull request: "logging: use bitset for categories"
(https://github.com/bitcoin/bitcoin/pull/26697#pullrequestreview-2225977743)
Code review ACK dadce0ba946aa9c209fc5c15d9efe627f7a486a2
Not important, but I think you could consider renaming `BCLog::LogFlagsBitset` to `BCLog::CategoryMask` to provide a less clunky type name and be consistent with existing `BCLog::::Logger::GetCategoryMask()` method.
(https://github.com/bitcoin/bitcoin/pull/26697#pullrequestreview-2225977743)
Code review ACK dadce0ba946aa9c209fc5c15d9efe627f7a486a2
Not important, but I think you could consider renaming `BCLog::LogFlagsBitset` to `BCLog::CategoryMask` to provide a less clunky type name and be consistent with existing `BCLog::::Logger::GetCategoryMask()` method.
💬 ryanofsky commented on pull request "logging: use bitset for categories":
(https://github.com/bitcoin/bitcoin/pull/26697#discussion_r1707700923)
In commit "logging: use bitset class for categories" (dadce0ba946aa9c209fc5c15d9efe627f7a486a2)
In std::bitset, the "is_any" and "is_none" methods seem to be called just any and none. IMO, the standard names make more sense than these names because I don't think it makes sense to say a set "is any" or a set "is none" but I do think it makes sense for it to have "any" and "none" properties.
(https://github.com/bitcoin/bitcoin/pull/26697#discussion_r1707700923)
In commit "logging: use bitset class for categories" (dadce0ba946aa9c209fc5c15d9efe627f7a486a2)
In std::bitset, the "is_any" and "is_none" methods seem to be called just any and none. IMO, the standard names make more sense than these names because I don't think it makes sense to say a set "is any" or a set "is none" but I do think it makes sense for it to have "any" and "none" properties.
💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707740750)
do we need the escape at the end?
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707740750)
do we need the escape at the end?
📝 justinvforvendetta opened a pull request: "Revert "depends: Fetch miniupnpc sources from an alternative website""
(https://github.com/bitcoin/bitcoin/pull/30602)
This reverts commit 21b8a14d37c19ce292d5529597e0d52338db48a9.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
s
...
(https://github.com/bitcoin/bitcoin/pull/30602)
This reverts commit 21b8a14d37c19ce292d5529597e0d52338db48a9.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
s
...
✅ justinvforvendetta closed a pull request: "Revert "depends: Fetch miniupnpc sources from an alternative website""
(https://github.com/bitcoin/bitcoin/pull/30602)
(https://github.com/bitcoin/bitcoin/pull/30602)
💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707746253)
if this comment still refers to the mac build, note that mac doesn't have `nproc`
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707746253)
if this comment still refers to the mac build, note that mac doesn't have `nproc`
📝 justinvforvendetta opened a pull request: "Revert "depends: Fetch miniupnpc sources from an alternative website""
(https://github.com/bitcoin/bitcoin/pull/30603)
This reverts commit 21b8a14d37c19ce292d5529597e0d52338db48a9.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
s
...
(https://github.com/bitcoin/bitcoin/pull/30603)
This reverts commit 21b8a14d37c19ce292d5529597e0d52338db48a9.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
s
...
📝 fjahr opened a pull request: "doc, chainparams: 29775 follow-ups"
(https://github.com/bitcoin/bitcoin/pull/30604)
This completes follow-ups left open in #29775.
- Adds release notes
- Addresses the [misalignment](https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1706982102) in deprecation warnings and hints at the intention to remove support for Testnet3 in v30.
- Adds initial minimum chainwork for Testnet4.
(https://github.com/bitcoin/bitcoin/pull/30604)
This completes follow-ups left open in #29775.
- Adds release notes
- Addresses the [misalignment](https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1706982102) in deprecation warnings and hints at the intention to remove support for Testnet3 in v30.
- Adds initial minimum chainwork for Testnet4.
💬 ryanofsky commented on issue "ci: failure in `wallet_multiwallet.py --legacy-wallet` - (`void wallet::UnloadWallet(std::shared_ptr<CWallet> &&): Assertion 'it.second' failed.`)":
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2274237242)
This seems like a bug in the UnloadWallet function, where it just crashes if it is called by more than one thread at the same time for the same wallet. (For example if there are multiple unloadwallet RPC calls at the same time, or if there is an unloadwallet RPC call at the same time as a shutdown.) Possible fix would be:
```diff
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -244,15 +244,19 @@ void UnloadWallet(std::shared_ptr<CWallet>&& wallet)
{
// Mark wallet for un
...
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2274237242)
This seems like a bug in the UnloadWallet function, where it just crashes if it is called by more than one thread at the same time for the same wallet. (For example if there are multiple unloadwallet RPC calls at the same time, or if there is an unloadwallet RPC call at the same time as a shutdown.) Possible fix would be:
```diff
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -244,15 +244,19 @@ void UnloadWallet(std::shared_ptr<CWallet>&& wallet)
{
// Mark wallet for un
...
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707807652)
> Looks like `AC_SYS_LARGEFILE` / `_LARGE_FILES` was never ported?
The comment in the [implementation code](https://git.savannah.gnu.org/gitweb/?p=autoconf.git;a=blob;f=lib/autoconf/specific.m4;h=cea6247a9d1417661e05f77e378460248540ea80;hb=HEAD#l264) mentions "32-bit AIX 4.2.1+, 32-bit z/OS". Do we support those systems?
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707807652)
> Looks like `AC_SYS_LARGEFILE` / `_LARGE_FILES` was never ported?
The comment in the [implementation code](https://git.savannah.gnu.org/gitweb/?p=autoconf.git;a=blob;f=lib/autoconf/specific.m4;h=cea6247a9d1417661e05f77e378460248540ea80;hb=HEAD#l264) mentions "32-bit AIX 4.2.1+, 32-bit z/OS". Do we support those systems?
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707812278)
cc @dergoegge
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707812278)
cc @dergoegge
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707812607)
cc @dergoegge
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707812607)
cc @dergoegge
💬 brunoerg commented on issue "fuzz: crypto_fschacha20poly1305 timeout":
(https://github.com/bitcoin/bitcoin/issues/30505#issuecomment-2274264770)
Found the timeout really fast by running:
```diff
diff --git a/src/test/fuzz/crypto_chacha20poly1305.cpp b/src/test/fuzz/crypto_chacha20poly1305.cpp
index 2b39a06094..e2e6df6c77 100644
--- a/src/test/fuzz/crypto_chacha20poly1305.cpp
+++ b/src/test/fuzz/crypto_chacha20poly1305.cpp
@@ -130,7 +130,7 @@ FUZZ_TARGET(crypto_fschacha20poly1305)
// data).
InsecureRandomContext rng(provider.ConsumeIntegral<uint64_t>());
- LIMITED_WHILE(provider.ConsumeBool(), 10000)
+ LIMIT
...
(https://github.com/bitcoin/bitcoin/issues/30505#issuecomment-2274264770)
Found the timeout really fast by running:
```diff
diff --git a/src/test/fuzz/crypto_chacha20poly1305.cpp b/src/test/fuzz/crypto_chacha20poly1305.cpp
index 2b39a06094..e2e6df6c77 100644
--- a/src/test/fuzz/crypto_chacha20poly1305.cpp
+++ b/src/test/fuzz/crypto_chacha20poly1305.cpp
@@ -130,7 +130,7 @@ FUZZ_TARGET(crypto_fschacha20poly1305)
// data).
InsecureRandomContext rng(provider.ConsumeIntegral<uint64_t>());
- LIMITED_WHILE(provider.ConsumeBool(), 10000)
+ LIMIT
...
💬 melvincarvalho commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1707820839)
Testnet3 is always available from the right branch and can be forked by any interested community.
Consensus is that Testnet4 will be better for Bitcoin testing.
There is still value in Testnet3, especially for observing Bitcoin beyond 2.8 million blocks, understanding reorgs, and exploring reorg protection strategies.
If there's a community for Testnet3, fork it, create a website, and get folks to run nodes.
Bitcoin devs have decided that Testnet3 is getting harder to use, and Testne
...
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1707820839)
Testnet3 is always available from the right branch and can be forked by any interested community.
Consensus is that Testnet4 will be better for Bitcoin testing.
There is still value in Testnet3, especially for observing Bitcoin beyond 2.8 million blocks, understanding reorgs, and exploring reorg protection strategies.
If there's a community for Testnet3, fork it, create a website, and get folks to run nodes.
Bitcoin devs have decided that Testnet3 is getting harder to use, and Testne
...
💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707821187)
I still wasn't able to run the fuzz tests with cmake (tried all mac tricks I could find here), I'm getting:
```
-- Performing Test CXX_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER
-- Performing Test CXX_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER - Success
-- Performing Test LINKER_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER_a797
-- Performing Test LINKER_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER_a797 - Failed
CMake Error at CMakeLists.txt:389 (message):
Linker did not accept reque
...
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707821187)
I still wasn't able to run the fuzz tests with cmake (tried all mac tricks I could find here), I'm getting:
```
-- Performing Test CXX_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER
-- Performing Test CXX_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER - Success
-- Performing Test LINKER_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER_a797
-- Performing Test LINKER_SUPPORTS__FSANITIZE_UNDEFINED_ADDRESS_FUZZER_a797 - Failed
CMake Error at CMakeLists.txt:389 (message):
Linker did not accept reque
...
💬 achow101 commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2274285542)
ACK 589db872e116779ab9cae693171ac8a8c02d9923
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2274285542)
ACK 589db872e116779ab9cae693171ac8a8c02d9923
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707843731)
> I wasn't able to run the fuzz tests with cmake
Are you able to build them using Autotools?
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707843731)
> I wasn't able to run the fuzz tests with cmake
Are you able to build them using Autotools?
💬 mzumsande commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2274310940)
> This PR exposes that nodes are currently making duplicate transaction requests not too infrequently. (...)
I think that this will also happen with just one peer and chains of transactions:
1.) orphan X is received, put into orphanage, parent P requested
2.) a child Y of X is received -> It's also an orphan, parent X is requested again (by txid!) because `AlreadyHaveTx` does not check the orphanage by txid ([reason](https://github.com/bitcoin/bitcoin/blob/0f68a05c084bef3e53e3f549c403bc90
...
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2274310940)
> This PR exposes that nodes are currently making duplicate transaction requests not too infrequently. (...)
I think that this will also happen with just one peer and chains of transactions:
1.) orphan X is received, put into orphanage, parent P requested
2.) a child Y of X is received -> It's also an orphan, parent X is requested again (by txid!) because `AlreadyHaveTx` does not check the orphanage by txid ([reason](https://github.com/bitcoin/bitcoin/blob/0f68a05c084bef3e53e3f549c403bc90
...
📝 sipa opened a pull request: "Cluster linearization: separate tests from tests-of-tests"
(https://github.com/bitcoin/bitcoin/pull/30605)
The current cluster linearization fuzz tests contain two tests which combine testing of production code with testing of the test code itself:
* `clusterlin_search_finder`: establishes the correctness of `SearchCandidateFinder` by comparing against both `SimpleCandidateFinder` and `ExhaustiveCandidateFinder` (which is even more simple than `SimpleCandidateFinder`). If `SimpleCandidateFinder` works correctly, then this comparison with `ExhaustiveCandidateFinder` is redundant. If it isn't, we ough
...
(https://github.com/bitcoin/bitcoin/pull/30605)
The current cluster linearization fuzz tests contain two tests which combine testing of production code with testing of the test code itself:
* `clusterlin_search_finder`: establishes the correctness of `SearchCandidateFinder` by comparing against both `SimpleCandidateFinder` and `ExhaustiveCandidateFinder` (which is even more simple than `SimpleCandidateFinder`). If `SimpleCandidateFinder` works correctly, then this comparison with `ExhaustiveCandidateFinder` is redundant. If it isn't, we ough
...
💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707893570)
Yes, but I do get some [errors](https://github.com/user-attachments/files/16534146/scratch_62.txt) at the beginning.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1707893570)
Yes, but I do get some [errors](https://github.com/user-attachments/files/16534146/scratch_62.txt) at the beginning.