💬 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

💬 fjahr commented on pull request "test: Use std::span and std::string_view for raw data":
(https://github.com/bitcoin/bitcoin/pull/30796#issuecomment-2331002451)
re-ACK https://github.com/bitcoin/bitcoin/commit/faecca9a85c1c1917d024f55cca34d19cc94f3b9

Only changes since last review were usage of `std::to_integer` and some shuffling of newline characters.

👍 TheCharlatan approved a pull request: "depends: build libevent with `-D_GNU_SOURCE`"
(https://github.com/bitcoin/bitcoin/pull/30743#pullrequestreview-2282333673)
ACK 556775408797d8e27154c3edaf139820b0979cce

The reproducibility issues seems to be unrelated to this change, so I don't think this should be blocked by it.

💬 somethingbeta commented on issue "Test Framework - test_framework.test_node.FailedToStartError: No RPC credentials":
(https://github.com/bitcoin/bitcoin/issues/30818#issuecomment-2331029828)
> > 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 mess
...

💬 TheCharlatan commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#discussion_r1745115109)
I think we are trying to consistently use `cmake --build` over `make `.

👍 TheCharlatan approved a pull request: "contrib: fix check-deps.sh to check for weak symbols"
(https://github.com/bitcoin/bitcoin/pull/30415#pullrequestreview-2282355222)
ACK 3e4312eef78f233eb7ae1d7d85e497de34144f2e