💬 Sjors commented on pull request "rpc: add optional blockhash to waitfornewblock":
(https://github.com/bitcoin/bitcoin/pull/30635#issuecomment-2491024996)
Rebased, added comment and expanded commit message to note the GUI improvement.

💬 maflcko commented on issue "Avoid internet traffic from tests":
(https://github.com/bitcoin/bitcoin/issues/31339#issuecomment-2491034618)
I think it makes sense to enforce this in CI (and have a tracking issue or pull for that). Otherwise, it is easy to regress, like in https://github.com/bitcoin/bitcoin/pull/30529#issuecomment-2252854223

However, if there is a violation of this, or a bug, it would be good to add exact steps to reproduce, or an explanation. Claiming that this is broken on "Ubuntu 28.04 LTS" doesn't seem helpful to uncover the bug, if there is one.

Related: https://github.com/bitcoin/bitcoin/issues/30030

💬 maflcko commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1851990186)
Actually, fixed. You can drop this now.

👍 theStack approved a pull request: "test: Deduplicate assert_mempool_contents()"
(https://github.com/bitcoin/bitcoin/pull/31338#pullrequestreview-2451281918)
ACK a0eafc10f94362408f54195ffd5a9237dc1ef638

Thanks for following up!

💬 Sjors commented on pull request "rpc: add optional blockhash to waitfornewblock":
(https://github.com/bitcoin/bitcoin/pull/30635#issuecomment-2491063362)
I added a commit to make `waitfornewblock` visible in `help`. It was introduced in 2016 by #8680 as a quick fix to deal with CI failures.

At this point is seem mature enough, no longer has weird quirks like requiring GUI users to enable the RPC server, it's probably actually useful and at least [one library uses it anyway](https://github.com/rust-bitcoin/rust-bitcoincore-rpc/blob/06a6c156b4d469c60c1abe8afdc002e7e11e5d3b/client/src/client.rs#L1100). So it seems fine to list it.

💬 m3dwards commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2491116799)
> Made some changes to this part of `TestNode.stop_node()` to handle an already dead process

Looks cleaner.

ACK ba28593147ccc880c3a4d40db0d4ef57f4766254

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2491178957)
Rebased after #31288 and #31122.

This PR is now based on #31325 since that has enough support. Keeping this draft until that's merged (or dropped).

The test changes will probably cause a merge conflict with #31318, but it should be trivial.

I dropped the early return in `waitNext()` because `wait_until` should already take care of that. I also made the while loop use `<=` instead of `<` so this actually works: https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1843589729 Added a
...

✅ fanquake closed an issue: "ci: brew link workaround step no-longer working"
(https://github.com/bitcoin/bitcoin/issues/31334)

🚀 fanquake merged a pull request: "macOS: swap docs & CI from pkg-config to pkgconf"
(https://github.com/bitcoin/bitcoin/pull/31335)

👍 brunoerg approved a pull request: "fuzz: Implement G_TEST_GET_FULL_NAME"
(https://github.com/bitcoin/bitcoin/pull/31333#pullrequestreview-2451460077)
code review ACK 92d3d691f097ead8e5ae571eb9bf691133a6aa49

🚀 fanquake merged a pull request: "fuzz: Implement G_TEST_GET_FULL_NAME"
(https://github.com/bitcoin/bitcoin/pull/31333)

💬 hebasto commented on issue "CMake `CheckPIESupported` doesn't always work":
(https://github.com/bitcoin/bitcoin/issues/30771#issuecomment-2491293124)
An upstream issue: https://gitlab.kitware.com/cmake/cmake/-/issues/26463.

💬 TheCharlatan commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1852145969)
I would squash the last commit, then you can also drop the `using node::BlockAssembler` declaration entirely by just calling `MineBlock(g_setup->m_node, {})`.

💬 Sjors commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#issuecomment-2491372423)
I dropped a few spurious `(...)coinbase(_output)_script_pub_key` renames from the test commit.

And dropped another unneeded `OP_TRUE` from tx_pool.

💬 Sjors commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2491393950)
Rebased after fe3457c.

💬 brunoerg commented on pull request "fuzz: set the output argument of FuzzedSock::Accept()":
(https://github.com/bitcoin/bitcoin/pull/31316#issuecomment-2491407513)
> Perhaps this makes more sense in combination with a test that uses it? Currently FuzzedSock::Accept seems entirely unused: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/test/fuzz/util/net.cpp.gcov.html

I agree. I was going to ask steps to reproduce then I noticed this is unused.

💬 Sjors commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#discussion_r1852276089)
@mzumsande I don't recall such discussions. The only thing that matters for `getblockfrompeer` is that it _can_ store previously pruned blocks.

That said, I do think it's useful to allow `submitblock` to store a previously pruned block. It lets a user dump the block on one node and submit it on this node, without having go through the effort of establishing a p2p connection, figuring out the peer id and then calling `getblockfrompeer`.

💬 Sjors commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#discussion_r1852312507)
@maflcko it's probably useful to briefly reopen #10146 and link to this thread for the likely real reason for that PR.

I'm speculating it was done in a stealthy way because there might be someone running a public service to submit blocks, and it would use that RPC under the hood. Or they were worried that since mining pools use this RPC internally (see e.g. #3658) they might have it exposed to the internet and therefore could be attacked.

cc @dergoegge @darosior in case you want to add it
...

📝 theStack opened a pull request: "test: add missing segwitv1 test cases to `script_standard_tests`"
(https://github.com/bitcoin/bitcoin/pull/31340)
Currently we have two segwitv1 output script types that are considered standard:
- `TxoutType::WITNESS_V1_TAPROOT` (P2TR): witness program has size 32 (introduced with taproot soft-fork)
- `TxoutType::ANCHOR` (P2A): witness program is {0x4e, 0x7e} (introduced with #30352)

This PR adds them to the script standardness unit tests where missing, i.e. for using them with the `ExtractDestination` and `GetScriptForDestination` functions.

💬 Sjors commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#issuecomment-2491534171)
Note that #31196 drops `processNewBlock()` from the Mining IPC interface, which straight-forwardly uses `ProcessNewBlock`. The interface does still use it via `submitSolution()`.