💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1731519643)
Nice! I split the changes to `rpc/mining.cpp` into its own commit, that looks correct.

Running tests before pushing...

🤔 ismaelsadeeq reviewed a pull request: "bench: [refactor] iwyu"
(https://github.com/bitcoin/bitcoin/pull/30716#pullrequestreview-2261138334)
Concept ACK

💬 ismaelsadeeq commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2310653344)
post merge ACK 647fa37cdbadbeebba147ca6b24e138559cffaaf

🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: optimized candidate search"
(https://github.com/bitcoin/bitcoin/pull/30286#pullrequestreview-2260608958)
Concept ACK

💬 ismaelsadeeq commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1731199919)
While reading through the bench I noticed that docstring explanation of `BenchLinearizeNoItersWorstCaseAnc` still refer to `BenchLinearizePerIterWorstCase`

🤔 ismaelsadeeq reviewed a pull request: "Use MiniWallet in functional test rpc_signrawtransactionwithkey."
(https://github.com/bitcoin/bitcoin/pull/30701#pullrequestreview-2261157933)
Concept ACK

I'll prefer if you split this commit intro three as you highlight in commit message

1- Replace custom funding tx creation with MiniWallet
2- Remove unnecessary clean chain setup and second node
2- Simplify transaction handling

💬 luke-jr commented on issue "[build] cannot build tests using v26.0":
(https://github.com/bitcoin/bitcoin/issues/29051#issuecomment-2310689306)
This should be reopened. It's a bug if `make clean` fixes things. The build system is supposed to automatically detect when a file needs to be rebuilt. (Maybe CMake will fix it?)

👍 ismaelsadeeq approved a pull request: "test: fix `TestShell` initialization (late follow-up for #30463)"
(https://github.com/bitcoin/bitcoin/pull/30714#pullrequestreview-2261197556)
I verified that this behavior occurs locally:

```python
Python 3.12.4 (main, Jun 6 2024, 18:26:44) [Clang 15.0.0 (clang-1500.3.9.4)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.path.insert(0, "/Users/abubakarismail/Desktop/bitcoin-dev/bitcoin/test/functional")
>>> from test_framework.test_shell import TestShell
>>> test = TestShell().setup()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/U
...

💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2310705314)
Alright then, dropping `g_best_block` as well!

Some tests failed with `EnsureAnyNodeContext(context).notifications`, so I fixed that (relative to the above patches). In `init.cpp` the RPC server starts at step 4a, but the notifications are added at step 7. This case is hit by several functional tests that shut the node down early.

📝 Prabhat1308 opened a pull request: "rpc: Add test-only RPCs under `-test=<option>` flag"
(https://github.com/bitcoin/bitcoin/pull/30717)
followup of #29007 , implements adding of test-only RPC options in `-test` as discussed [here](https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1417082950)

Currently a general user has access to `test-only` RPCs , where user might accidentally use it in production. This PR splits off test-only RPCs to be included in `-test` flag with additional restrictions of use in `regtest` segregating the user facing and testing RPCs . These test-only RPCs are split off from args and have their o
...

🤔 hebasto reviewed a pull request: "bench: [refactor] iwyu"
(https://github.com/bitcoin/bitcoin/pull/30716#pullrequestreview-2261245718)
I still see IWYU warnings in https://cirrus-ci.com/task/5307830952001536. Here are some of them:

```
bench/addrman.cpp should add these lines:
#include "netaddress.h" // for CService, CNetAddr, Network
#include "protocol.h" // for CAddress, ServiceFlags
```
```
bench/bech32.cpp should remove these lines:
- #include <cstdint> // lines 9-9
```
```
bench/bench.h should add these lines:
#include "nanobench.h" // for Bench (ptr only)
```
```
bench/data.cpp should add th
...

💬 murchandamus commented on pull request "rpc: Add test-only RPCs under `-test=<option>` flag":
(https://github.com/bitcoin/bitcoin/pull/30717#discussion_r1731615240)
From what I understand, the startup options `deprecatedrpc`, `stopatheight`, `limitancestorcount`, `limitancestorsize`, `limitdescendantcount`, and `limitdescendantsize` are definitely used in production by some users.

🤔 murchandamus reviewed a pull request: "rpc: Add test-only RPCs under `-test=<option>` flag"
(https://github.com/bitcoin/bitcoin/pull/30717#pullrequestreview-2261289148)
Looking at the discussion in #29007, I had the impression that it was suggested to move some RPC options to a "test-only" grouping, so I’m a bit surprised that this PR also moves a number of startup options to "test-only".

I’m wondering whether there was a misunderstanding regarding "RPC options" and "startup options" here.

💬 martinsaposnic commented on pull request "Use MiniWallet in functional test rpc_signrawtransactionwithkey.":
(https://github.com/bitcoin/bitcoin/pull/30701#issuecomment-2310776625)
@ismaelsadeeq rebased and pushed. thanks for the review!

📝 theStack opened a pull request: "test: switch MiniWallet padding unit from weight to vsize "
(https://github.com/bitcoin/bitcoin/pull/30718)
This PR is a late follow-up for #30162, where I retrospectively consider the padding unit of choice as a mistake. The weight unit is merely a consensus rule detail and is largely irrelevant from a user's perspective w.r.t. fee-rate calculations and mempool policy rules (e.g. for package relay and TRUC limits), so there doesn't seem to be any value of using a granularity that we can't even guarantee to reach exactly anyway.

Switch to the more natural unit of vsize instead, which simplifies bot
...

💬 Prabhat1308 commented on pull request "rpc: Add test-only RPCs under `-test=<option>` flag":
(https://github.com/bitcoin/bitcoin/pull/30717#discussion_r1731625165)
In the arguments description in `init.cpp` they are categorised as `DEBUG_ONLY` AND `DEBUG_TEST` which is where I picked them up to be "test-only". There was not much documentation elsewhere to decide otherwise.

💬 murchandamus commented on pull request "rpc: Add test-only RPCs under `-test=<option>` flag":
(https://github.com/bitcoin/bitcoin/pull/30717#discussion_r1731628845)
I see. What’s the overarching intent here: is the purpose just to label them more explicitly as being for testing, or is the intent to restrict their use to test networks?

👍 jaonoctus approved a pull request: "seeds: Pull additional nodes from my seeder and update fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/30008#pullrequestreview-2261317336)
utACK

💬 Prabhat1308 commented on pull request "rpc: Add test-only RPCs under `-test=<option>` flag":
(https://github.com/bitcoin/bitcoin/pull/30717#discussion_r1731633356)
We are restricting its use in test networks only specifically to `regtest` only . the `-test` flag has a check to assert use of `regtest`.

💬 achow101 commented on pull request "test: XORed blocks test follow up":
(https://github.com/bitcoin/bitcoin/pull/30669#issuecomment-2310802609)
ACK e1d5dd732d5dc641faf1dde316275c84b6bb224b