💬 garlonicon commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2330694091)
Also, we can now clearly see, why weak blocks are needed: mempool.space shows more than 500 transactions, waiting for being confirmed, but only a few of them are really mined, and the rest is waiting patiently, because nobody rebroadcasted them.

And there is more: it is possible to easily double-spend them, because it will take some time, before they will be shared in P2P way.

It is also something to consider, in case when mempools are congested: if you can see a miner, sending blocks with
...

💬 Sjors commented on pull request "signet: fixing mining for OP_TRUE challenge":
(https://github.com/bitcoin/bitcoin/pull/29032#issuecomment-2330721828)
Rebased after #28417.

💬 Sjors commented on pull request "contrib/signet/miner updates":
(https://github.com/bitcoin/bitcoin/pull/28417#issuecomment-2330721959)
If you liked this PR, you may also like the freshly rebased #29032!

💬 Sjors commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2330736873)
@garlonicon I think you're going a bit off topic here, a weak blocks proposals seems more suitable for the mailinglist.

💬 Sjors commented on pull request "build: Minor build system fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30803#discussion_r1744925332)
This explanation makes more sense to me than the current code comment.

> try `find_package(ZeroMQ)` first, and fallback to `pkg_check_modules(libzmq)`

That would be nice. Then a short clarifying comment could be:

```
# Use CMake config file if the distro provides it (pseudo code)
try find_package(ZeroMQ)
# Otherwise
catch pkg_check_modules(libzmq)
```

💬 Sjors commented on pull request "build: Minor build system fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30803#discussion_r1744927863)
cc9d65dc05f802aaaa108a2edcf30c3758c3f459: maybe add a comment for why there's not test command (yet?)

👍 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