👍 MarnixCroes approved a pull request: "doc: fix assumeutxo design doc link"
(https://github.com/bitcoin/bitcoin/pull/30819#pullrequestreview-2282046097)
ACK e5f7272ad322aa4eb7a7d6f531419a4d7aee4802

💬 maflcko commented on pull request "build: Minor build system fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30803#discussion_r1744939626)
See https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1743353913

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1744945740)
> ```shell
> *** 4 failures are detected in the test module "Bitcoin Core Test Suite"
> ```

Does `export LC_ALL=C.UTF-8` make any difference? This is set in the CI, where it "works" before and after the cmake migration. Maybe a separate issue can be created, as it seems unrelated to cmake?

💬 maflcko commented on pull request "test: Add coverage for dumptxoutset failure robustness":
(https://github.com/bitcoin/bitcoin/pull/30817#issuecomment-2330806815)
You can also remove `UniValue error`, now that it is no longer used:

```diff
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 3ba89d08d9..b88e9103c1 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -2822,8 +2822,6 @@ static RPCHelpMan dumptxoutset()
Chainstate* chainstate;
std::unique_ptr<CCoinsViewCursor> cursor;
CCoinsStats stats;
- UniValue result;
- UniValue error;
{
// Lock the chainstate before calling P
...

💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1744966102)
> Is it fair to summarize that your concern is that even though the tests I remove here are theoretically not necessary since we should have equivalent unit test coverage on the 2 underlying functions (`uint256::FromHex()` and `UintToArith256()`), you'd rather keep them because ensuring that the unit test coverage is indeed equivalent is burdensome and makes this PR harder to review?

Yes. At least I had difficulty seeing that the test coverage does not decrease, and the tests run so fast that
...

💬 fjahr commented on pull request "test: Use std::span and std::string_view for raw data":
(https://github.com/bitcoin/bitcoin/pull/30796#discussion_r1744967558)
Ok, I hadn't come across this yet. I don't have a preference, you can leave it as is.

💬 maflcko commented on issue "Test Framework - test_framework.test_node.FailedToStartError: No RPC credentials":
(https://github.com/bitcoin/bitcoin/issues/30818#issuecomment-2330879461)
> Error: Specified data directory "/tmp/bitcoin_func_test_u9zpt_2g/node0" does not exist.

This seems to be the first error, so it would be good to debug this first. Can you check whether `/usr/src/bitcoin/test/cache/node0` exists and whether the following is successful:

> 2024-09-04T18:07:10.780000Z TestFramework (DEBUG): Copy cache directory /usr/src/bitcoin/test/cache/node0 to node 0

Given that you claim that `bitcoin.conf` exists (presumably in this dir), the error message that the
...

💬 maflcko commented on issue "Test Framework - test_framework.test_node.FailedToStartError: No RPC credentials":
(https://github.com/bitcoin/bitcoin/issues/30818#issuecomment-2330883102)
Even if you get it to run, Wine will print errors to stderr, which the functional test framework does not like, so they will have to be silenced one way or another.


```
# test/functional/feature_rbf.py
2024-09-05T08:02:41.987000Z TestFramework (INFO): PRNG seed is: 7350764182512023160
2024-09-05T08:02:41.988000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_bdledk26
2024-09-05T08:02:43.869000Z TestFramework (INFO): Running test simple doublespend...
2024-09-0
...

💬 maflcko commented on issue "Test Framework - test_framework.test_node.FailedToStartError: No RPC credentials":
(https://github.com/bitcoin/bitcoin/issues/30818#issuecomment-2330898737)
Running the Windows tests on Wine in Linux is obviously better than nothing, but it would be better to run them directly on Windows. However, I am not sure what the easiest way to do this would be. For now, editing the config ini to adjust the paths is required.

🤔 TheCharlatan reviewed a pull request: "test: Use std::span and std::string_view for raw data"
(https://github.com/bitcoin/bitcoin/pull/30796#pullrequestreview-2282176057)
Approach ACK

This looks good, just left a small question, and there is a typo at the start of the second commit's message.

💬 TheCharlatan commented on pull request "test: Use std::span and std::string_view for raw data":
(https://github.com/bitcoin/bitcoin/pull/30796#discussion_r1745006225)
Would it better to use `std::to_integer` here? It does the same thing, but maybe it would be good to use it for its compile-time guarantees.

🚀 fanquake merged a pull request: "doc: fix assumeutxo design doc link"
(https://github.com/bitcoin/bitcoin/pull/30819)

💬 fjahr commented on pull request "test: Add coverage for dumptxoutset failure robustness":
(https://github.com/bitcoin/bitcoin/pull/30817#discussion_r1745034884)
done

💬 fjahr commented on pull request "test: Add coverage for dumptxoutset failure robustness":
(https://github.com/bitcoin/bitcoin/pull/30817#discussion_r1745035071)
done

💬 fjahr commented on pull request "test: Add coverage for dumptxoutset failure robustness":
(https://github.com/bitcoin/bitcoin/pull/30817#issuecomment-2330925948)
> You can also remove UniValue error

Done in the refactoring commit, thanks!

👍 TheCharlatan approved a pull request: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block"
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2282242465)
ACK 6c419285720fe5a954e01f09cbe053a0f51bef47

But I think it would be good to get more review here. I am fairly convinced that the changes to when the notifications are issued now are good, but the change in logic is a bit tricky to follow.

💬 fanquake commented on pull request "test: fixing failing system_tests/run_command under some Locales":
(https://github.com/bitcoin/bitcoin/pull/30788#issuecomment-2330951266)
Backported to 28.x in #30762.

💬 maflcko commented on pull request "test: Use std::span and std::string_view for raw data":
(https://github.com/bitcoin/bitcoin/pull/30796#discussion_r1745076473)
Sure, added compile time check that ensures `std::is_integral_v<uint8_t>`.

Also, fixed typo in commit message.

Should be trivial to re-ACK

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2330975711)
The merge conflicts are addressed in #29032 and #29346. I plan to do another rebase round when more of the interface pull requests are merged, e.g. #30409.

👍 TheCharlatan approved a pull request: "test: Use std::span and std::string_view for raw data"
(https://github.com/bitcoin/bitcoin/pull/30796#pullrequestreview-2282318162)
ACK faecca9a85c1c1917d024f55cca34d19cc94f3b9