💬 ryanofsky commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1900964589)
In commit "test: use Mining interface in miner_tests" (e4e76f27d73646d887ed8b660c67da4b4b785d8e)

Can this comment be updated to say the reason for alternating between these? (e.g. to provide test coverage for net_processing code)

I think it would also be clearer to drop the comments about this in the PR and commit descriptions:

- "We mainly call the createNewBlock() method, but also getTip()->height and submitSolution(). Calls to the latter are alternated with direct calls to Chainman's
...

💬 ryanofsky commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1900936095)
In commit "test: use Mining interface in miner_tests" (e4e76f27d73646d887ed8b660c67da4b4b785d8e)

In other tests below this is followed by `BOOST_REQUIRE(miner);`. Seems fine to keep or drop, but would be good to be consistent.

Also I think it might be a little clearer if variable was called `mining` instead of `miner` since it's not a miner and can't mine blocks, it's just a pointer to the interface used for mining. But feel free to ignore this if you prefer the current name.

💬 ryanofsky commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1900922458)
In commit "test: use Mining interface in miner_tests" (e4e76f27d73646d887ed8b660c67da4b4b785d8e)

Other tests seem to add BOOST_REQUIRE(block_template); before this.

💬 ryanofsky commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1900923460)
In commit "test: use Mining interface in miner_tests" (e4e76f27d73646d887ed8b660c67da4b4b785d8e)

Other tests seem to add BOOST_REQUIRE(block_template); before this.

💬 ryanofsky commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1900973493)
In commit "test: use Mining interface in miner_tests" (e4e76f27d73646d887ed8b660c67da4b4b785d8e)

Unless I'm misunderstanding something it doesn't seem accurate to say that these don't check if the new block is valid. Would seem more accurate to say they return true even if the block is invalid. Also it would seem be more accurate to say that they update the tip, rather than that they wait for the tip to update.

💬 ryanofsky commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1900981503)
In commit "test: use Mining interface in miner_tests" (e4e76f27d73646d887ed8b660c67da4b4b785d8e)

Seems fine to call waitTipChange if idea is to add more test coverage for it, but wouldn't it be more direct to just check that the new tip matches the block hash if the idea is to check that the block was connected? This way if the block was not connected the test would fail instead of hanging indefinitely.

💬 i-am-yuvi commented on pull request "fuzz: Abort if system time is called without mock time being set":
(https://github.com/bitcoin/bitcoin/pull/31549#issuecomment-2567974643)
I am unable to crash the fuzz test when `setmocktime()` is removed from one of the targets.

clang version:

```
Homebrew clang version 18.1.7
Target: arm64-apple-darwin24.2.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm/bin
```

clang++ version:

```
Homebrew clang version 18.1.7
Target: arm64-apple-darwin24.2.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm/bin
```

and I've used this command mentioned in docs for [macos](https://github.com/bitcoin/bit
...

🤔 pinheadmz reviewed a pull request: "Extend signetchallenge to set target block spacing"
(https://github.com/bitcoin/bitcoin/pull/29365#pullrequestreview-2527798109)
concept ACK

💬 pinheadmz commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#discussion_r1901008846)
Why change this? Why not also include nodes 0 & 1?

💬 maflcko commented on pull request "fuzz: Abort if system time is called without mock time being set":
(https://github.com/bitcoin/bitcoin/pull/31549#discussion_r1901022152)
Shouldn't this be `g_used_system_time |= bool(mocktime.count())`? This would allow to remove the `GetMockTime().count() == 0` check as well.

💬 maflcko commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1901027110)
> Personally, I find the `ByteType auto` construct slightly more confusing than the more verbose template.

Ok, resolving for now.

If there is a preference about the style, my preference would be to have a clang-tidy (plugin) rule for this, instead of using human review resources.

💬 pinheadmz commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2568022971)
> This change is backward-compatible, as per the old rules, such a signet challenge would always evaluate to false, rendering it unused.

I don't completely agree with this, since starting an old version of bitcoind with (for example)
`-signetchallenge=6a4c0901ff000000000000004c0151` would create an incompatible node. It would have a different network magic bytes and I think the `OP_RETURN` challenge would cause it to reject all blocks.

💬 hebasto commented on pull request "qa: Use `sys.executable` when invoking other Python scripts":
(https://github.com/bitcoin/bitcoin/pull/31541#issuecomment-2568043575)
> Encountered a failure with `tool_wallet` on NetBSD that I want to check out before full ACK.

This issue is fixed in https://github.com/bitcoin/bitcoin/pull/31543. It could be worked around by setting the `LD_LIBRARY_PATH` environment variable.

💬 i-am-yuvi commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2568058991)
Concept ACK

It's a pretty required feature!! I will test it soon!!

💬 maflcko commented on pull request "test: Call generate RPCs through test framework only":
(https://github.com/bitcoin/bitcoin/pull/31403#issuecomment-2568104009)
The follow-up idea in https://github.com/bitcoin/bitcoin/pull/31403#pullrequestreview-2498806354 is up-for-grabs, if someone is looking for something to work on

💬 l0rinc commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `getblock` RPC":
(https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1901092142)
> I will leave that as an idea for a follow-up PR.

The problem is that the goal of this PR was to speed up the RPC.
But if we benchmark that instead of just a subset of the task, i.e.:
```patch
diff --git a/src/bench/rpc_blockchain.cpp b/src/bench/rpc_blockchain.cpp
--- a/src/bench/rpc_blockchain.cpp (revision bf5c569898d0297de010102a623bf52009607ed8)
+++ b/src/bench/rpc_blockchain.cpp (date 1735836623994)
@@ -24,7 +24,6 @@
namespace {

struct TestBlockAndIndex {
- const std::
...

💬 i-am-yuvi commented on pull request "test: Call generate RPCs through test framework only":
(https://github.com/bitcoin/bitcoin/pull/31403#issuecomment-2568122327)
> The follow-up idea in https://github.com/bitcoin/bitcoin/pull/31403#pullrequestreview-2498806354 is up-for-grabs, if someone is looking for something to work on

I'll take that!! Thanks for sharing!!

✅ maflcko closed a pull request: "fuzz: Faster leak check when generating corpus"
(https://github.com/bitcoin/bitcoin/pull/31481)

💬 maflcko commented on issue " (bitcoin core warning: unknown new rules activated versionbit 2) ":
(https://github.com/bitcoin/bitcoin/issues/31536#issuecomment-2568133106)
> i downloaded the newest version of bitcoin core wallet and run it and it finished sync but no balance showing up there either

You may have to rescan, when you load the wallet again, or restore from a backup. Otherwise, the transaction history (and balance) may be missing.

💬 maflcko commented on issue " (bitcoin core warning: unknown new rules activated versionbit 2) ":
(https://github.com/bitcoin/bitcoin/issues/31536#issuecomment-2568133444)
Usually the issue tracker is used to track technical issues related to the Bitcoin Core code base.

General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com) or the `#bitcoin` IRC channel on Libera Chat, or one of the Bitcoin subreddits, or any other place that you feel is well suited.