💬 furszy commented on pull request "test: add test for specifying custom pidfile via `-pid`":
(https://github.com/bitcoin/bitcoin/pull/30724#discussion_r1732866434)
nit:
why not declaring this custom param within the test function?

✅ laanwj closed a pull request: "depends: Remove Qt build-time dependencies"
(https://github.com/bitcoin/bitcoin/pull/29923)

💬 laanwj commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2312593547)
closing due to lack of interest

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2312603309)
ACK 6f6fc1464ad2f2981e7c3ff5d6492f60f89a6504

Attends to my concerns with the fuzz tests. Now in `txdownloadman_one_honest_peer` no "non-good" peer will ever deliver `TX_REAL`, ensuring that the test is actually testing what we want. Determinstic randomness was also given to the member txrequest modules.

Lastly, minimum time skips were increased to 100ms, meaning 100ms*100000=~166 minutes of simulated time. Since there are up to 8+120=128 peers, this means each peer can stall out the node
...

💬 hodlinator commented on pull request "refactor: Testnet4 - Replace uint256S("str") -> uint256{"str"}":
(https://github.com/bitcoin/bitcoin/pull/30721#issuecomment-2312604054)
> A scripted-diff won't help here, because it is applied on top of the commit it is based on, which won't change when master is changed, unless you rebase on every push to master.

Couldn't CI at least run scripted diffs when the PR they belong to gets merged into master - so it could be caught post-merge?

💬 cryptoquick commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs":
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2312605120)
I'm trying to check my RAM using memtest86, but I'm having trouble booting into that too. Give me a bit to troubleshoot.

💬 hebasto commented on pull request "fuzz: Add missing fuzz targets to cmake build":
(https://github.com/bitcoin/bitcoin/pull/30712#issuecomment-2312612637)
> Can be tested via:
>
> ```
> PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 ./bld-autot/src/test/fuzz/fuzz > /tmp/f_autot
> PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 ./bld-cmake/src/test/fuzz/fuzz > /tmp/f_cmake
> diff --unified /tmp/{f_autot,f_cmake}
> ```

Such a check can be automated: https://github.com/hebasto/bitcoin/commits/pr30712_amended.

💬 maflcko commented on pull request "refactor: Testnet4 - Replace uint256S("str") -> uint256{"str"}":
(https://github.com/bitcoin/bitcoin/pull/30721#issuecomment-2312650978)
When a commit from a pull request (which may contain a scripted-diff) is merged into another branch, the commit itself does not change, so it won't affect the execution or the result of the scripted-diff in that commit.

In theory, the commits could be rebased/cherry-picked instead of merged. However, that'd make auditing harder, because it makes it easier to inject malicious code accidentally or intentionally. Also, it wouldn't help here, because then the scripted diff check may permanently f
...

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1732943642)
I'm a bit confused where you want this code to go...

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2312700483)
@laanwj I suggest adding a "Suggested Followups" section to the PR description, where you can list known issues that can be punted.

💬 hebasto commented on pull request "guix: Pointer Authentication and Branch Target Identification for aarch64 Linux (Guix)":
(https://github.com/bitcoin/bitcoin/pull/24123#issuecomment-2312711208)
It would be helpful if someone with BTI-enabled hardware could test the binaries and verified BTI during [runtime](https://wiki.debian.org/ToolChain/PACBTI).

Unfortunately, I'm unable to do it by myself, as my hardware supports only PAC, not BTI.

💬 github12101 commented on issue "b-msghand[4988] general protection fault":
(https://github.com/bitcoin/bitcoin/issues/30706#issuecomment-2312715012)
> Thank you for reporting this.
>
> While issues in `b-msghand` can be problematic and it's good to report them, I don't think this report contains any actionable details. The gbd backtrace only contains memory addresses and no further debug information.
>
> Unless you can share more information or you don't start to see more frequent segmentation faults, I don't think it's worth keeping this issue open.

Thanks for your reply. I am aware backtrace only contains ??? instead of anything u
...

💬 hodlinator commented on pull request "refactor: Testnet4 - Replace uint256S("str") -> uint256{"str"}":
(https://github.com/bitcoin/bitcoin/pull/30721#issuecomment-2312729226)
#29775 was merged into master as da083d4bbdb37737f5080fada97bd15f5a8bfb2d, and #30560 was merged into master as 21c2879f37ce336af6df878d43ab090eb9d02157.

I'm thinking CI running on 21c2879f37ce336af6df878d43ab090eb9d02157 upon merge, could detect a new scripted-diff, re-run it on 21c2879f37ce336af6df878d43ab090eb9d02157 and in theory detect issues with #29775 if it had been merged into master earlier (wouldn't have caught this case though, as da083d4bbdb37737f5080fada97bd15f5a8bfb2d was merge
...

💬 cdecker commented on pull request "chainparams: Remove seed.bitcoinstats.com":
(https://github.com/bitcoin/bitcoin/pull/30720#issuecomment-2312740874)
Great, I'm just testing it as we speak, and I'll open a new PR adding it, once its shown to work, if that's ok.

💬 maflcko commented on issue "b-msghand[4988] general protection fault":
(https://github.com/bitcoin/bitcoin/issues/30706#issuecomment-2312745148)
The guix debug symbols are split from the `bitcoind` you are using. So someone would have download them before demangling. I tried to do that, but for some reason I had no success. I am not a build system expert, so I am not sure where the problem lies.

👍 stickies-v approved a pull request: "bench: [refactor] iwyu"
(https://github.com/bitcoin/bitcoin/pull/30716#pullrequestreview-2263571847)
ACK fab0e834b8cf906c286ebbeed86eca8d65854f75

Reviewed by:
- checking that all changes are limited to updating includes and fwd declarations in `bench/` files
- checking the clang-tidy [CI job output](https://api.cirrus-ci.com/v1/task/4676750536343552/logs/ci.log) for suggestions on `bench/` files, yielding only one result for `bench/bench.h` (as already pointed out by other reviewers):
```
grep "bench\/.*should" -A 3
bench/bench.h should add these lines:
#include "nanobench.h" // for
...

💬 stickies-v commented on pull request "bench: [refactor] iwyu":
(https://github.com/bitcoin/bitcoin/pull/30716#discussion_r1732974884)
Review note: this include (and thus the new pragma) is necessary to compile `setup_common.cpp`. Without it, fails with

```
Undefined symbols for architecture arm64:
"_G_TEST_COMMAND_LINE_ARGUMENTS", referenced from:
BasicTestingSetup::BasicTestingSetup(ChainType, TestOpts) in libtest_util.a[11](libtest_util_a-setup_common.o)
"_G_TEST_GET_FULL_NAME", referenced from:
BasicTestingSetup::BasicTestingSetup(ChainType, TestOpts) in libtest_util.a[11](libtest_util_a-setup_common
...

📝 brunoerg opened a pull request: "fuzz: fix timeout in `crypto_fschacha20poly1305`"
(https://github.com/bitcoin/bitcoin/pull/30725)
This PR fixes a timeout in `crypto_fschacha20poly1305` by reducing the number of iterations. I left it running for a while and noticed it speeds up the target and do not impact coverage.

💬 brunoerg commented on pull request "fuzz: fix timeout in `crypto_fschacha20poly1305`":
(https://github.com/bitcoin/bitcoin/pull/30725#issuecomment-2312773626)
I think it can be tested by replacing the `ConsumeBool()` to `true` which causes a timeout quickly.
```diff
--- a/src/test/fuzz/crypto_chacha20poly1305.cpp
+++ b/src/test/fuzz/crypto_chacha20poly1305.cpp
@@ -130,7 +130,7 @@ FUZZ_TARGET(crypto_fschacha20poly1305)
// data).
InsecureRandomContext rng(provider.ConsumeIntegral<uint64_t>());

- LIMITED_WHILE(provider.ConsumeBool(), 10000)
+ LIMITED_WHILE(true, 10000)
{
// Mode:
```

Maybe this is a way to k
...

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2312787737)
Just posting in case someone else learns something. Not asking for any changes:

> Lastly, minimum time skips were increased to 100ms, meaning 100ms*100000=~166 minutes of simulated time. Since there are up to 8+120=128 peers, this means each peer can stall out the node for a full minute and we still have 40 minutes left for the honest peer to succeed in giving the tx.

After doing more study, I think this simulation "only" needs a little more than 7 minutes minimum time, since the internal
...