💬 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
...

💬 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
...

💬 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

💬 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?

💬 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
...

📝 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
...

💬 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.

🤔 mzumsande reviewed a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2226187388)
re-ACK 589db872e116779ab9cae693171ac8a8c02d9923

💬 furszy 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-2274343867)
@ryanofsky, I'm not seeing how multiple threads can reach `UnloadWallet` at the same time for the same wallet. `unloadwallet` first calls `RemoveWallet()` then `UnloadWallet()`. And `RemoveWallet()` locks the wallets context mutex and removes the wallet from the context. So the next thread executing `RemoveWallet()` would just return early there (the RPC would return an error) and never reach the follow-up `UnloadWallet` call.

💬 maflcko commented on issue "fuzz: crypto_fschacha20poly1305 timeout":
(https://github.com/bitcoin/bitcoin/issues/30505#issuecomment-2274344203)
> Found the timeout really fast by running:

The fuzz input should also be in the attachment in the pull request description, which also allows to reproduce it.

💬 maflcko commented on pull request "test: [refactor] Use g_rand_ctx directly":
(https://github.com/bitcoin/bitcoin/pull/30571#issuecomment-2274352643)
> But it could also help down the road, for example if we wanted to remove the global, or run test cases deterministically in parallel.

I looked into it. However, it seems more work. My findings:

* There are ~300 lines of code touched, which can use the member alias or the global
* 108 lines can not use the member alias as-is, because the compiler does not see it in their scope, so they'll need to be fixed up manually one-by-one
* Excluding them (and only them) automatically leaves some
...

🤔 hodlinator requested changes to a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2225982967)
Concept ACK d045fc7ac1729cf29140c43518f20375f2aaa1cc

Thanks for chipping away at making hex string handling more robust!

Some minor disagreements + ignorable nits. The most significant disagreement in this thread: https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1700718302

Tested:
```bash
$ RANDOM_CTX_SEED=1231231231231231231231231231231231231231231231231231231231312312 src/test/test_bitcoin
$ test/functional/feature_assumevalid.py
```

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707728879)
ni: If padding were on a warm code path I would advocate for reserving the final size of the `result` string first, avoiding the temporary '0'-repeated string and appending `input` after the zeroes.

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707734248)
Why document the type here when it appears in the code below?
Seems noisy if we were to do that for return types everywhere.

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707760135)
`constexpr` implies `inline` for functions, doesn't it also for variables? Never used `inline` on variables myself, but seems it is useful in **.h** files to avoid ODR-issues. This is a **.cpp** file though.

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707914379)
nit:
```suggestion
BOOST_REQUIRE(args_man.ParseParameters(argv.size(), argv.data(), error));
BOOST_REQUIRE(error.empty());
```

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707739633)
nit: Would personally merge the next commit (e6f81b9d81359a7ffeff6d1830188f00df0e8db0) into this one (a227cb511ec948b37ddbc9ee65de586109ebc1da) since you're already touching this line and it looks weird as an atomic commit to just throw away the sanitized value.

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707706392)
> And we can't use `BOOST_CHECK_EQUAL(set_opts({"-assumevalid=0"}).assumed_valid_block, std::make_optional(uint256::ZERO))` since there's no standard way to display optionals, right?

Would this fit your definition of displaying optionals?

https://github.com/bitcoin/bitcoin/pull/16545/files#diff-d4a2fb26adedc27f16bd3778424fa94c473342a695b228220a1810119028be5bR48-R64

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707787408)
To me asserts are for enforcing logical consistency of the code, NOT for handling invalid values of environment variables.

A slight convention seems to be using `abort()` in these circumstances.
```C++
if (auto num_parsed{uint256::FromHex(num)}) {
return *num_parsed;
} else {
std::cerr << RANDOM_CTX_SEED << " must be a " << uint256::size() * 2 << " char hex string without '0x'-prefix, was set to: '" << num << "'." << std::endl;

...

💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707936724)
Thanks, I think that would produce better error messages on failure.

💬 maflcko commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707941944)
> To me asserts are for enforcing logical consistency of the code, NOT for handling invalid values of environment variables.

Correct. This rule must be applied to real production code. However, in the tests, personally I like to use it for brevity. No objection to using something else.