⚠️ maflcko reopened an issue: "bitcoind immediately segfaults on ppc64 openbsd 7.4"
(https://github.com/bitcoin/bitcoin/issues/29517)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

v26.0 and v26.1rc1 both build successfully on big endian ppc64 openbsd 7.4 but when starting, bitcoind immediately segfaults. CPU is POWER9.

Unfortunately, gdb on this platform also crashes when attempting to debug so all I have is this:


```
$ egdb bitcoind bitcoind.core
GNU gdb (GDB) 9.2
Copyright (C) 2020 Free S
...

💬 maflcko commented on issue "bitcoind immediately segfaults on ppc64 openbsd 7.4":
(https://github.com/bitcoin/bitcoin/issues/29517#issuecomment-2743158064)
Can you try if this happens with 29.0rc2 as well? https://github.com/bitcoin/bitcoin/releases/tag/v29.0rc2

I am asking, because it is using a new build system (cmake), so things may have changed.

Can you try with both g++ and clang++, again? Also, do all binaries in the `build_dir/bin` crash, or just bitcoind? Also, can you compile and run a simple stand-alone `std::cout << "Hi"` program?

💬 kevkevinpal commented on pull request "test: replace hardcoded fee with node relay fee based calculation":
(https://github.com/bitcoin/bitcoin/pull/32058#issuecomment-2743163279)
Concept ACK [6aff347](https://github.com/bitcoin/bitcoin/pull/32058/commits/6aff3477349e51180cf4f62784d07a4d4ce00603)

This looks to address the TODO correctly

💬 laanwj commented on pull request "fuzz: avoid returning non-conforming results from FuzzedSock::GetSockName()":
(https://github.com/bitcoin/bitcoin/pull/32109#issuecomment-2743167723)
Concept ACK

> if it indicates that the result is less than sizeof(sockaddr_in6) and sets sa_family equal to AF_INET6 in the output.

FWIW, i've tried to make our code robust to this case by adding the length checks in ab1d3ece026844e682676673b8a461964a5b3ce4.

But it's a very unlikely OS bug and this change will likely result in more useful test coverage.

🤔 janb84 reviewed a pull request: "contrib: Make deterministic-coverage error messages more readable"
(https://github.com/bitcoin/bitcoin/pull/32074#pullrequestreview-2705873894)
ACK [fa7a40d](https://github.com/bitcoin/bitcoin/commit/fa7a40d952ad7588f45f6229aeae754b02fdfb55)

- Code review ✅
- Tested several error messages on fuzz tool (no fuzz build, no coverage support, on-existent build-dir) ✅
- Tested several error message on unit tool (on-existent build-dir, no coverage support) ✅
- Tested for correct working for deterministic Unit test finding: util_string_tests ✅
- Tested for correct working for deterministic Fuzz test finding: uint160_deserialize ✅

💬 janb84 commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2007538466)
Perhaps it’s worth adding a note to clarify that the fuzz tool should be allowed to run until it completes. Given the nature of fuzz tests, someone might assume that, unlike fuzzing runs with "runs=1," this process will continue indefinitely, and thus a few "succeeded against 1 file in xs" messages might seem sufficient.

💬 BrandonOdiwuor commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2743338545)
> Also, the CI fails currently.

rebased and fixed the ci error

💬 brunoerg commented on pull request "fuzz: coinselection: cover `SetBumpFeeDiscount`":
(https://github.com/bitcoin/bitcoin/pull/31806#issuecomment-2743340366)
Rebased

💬 maflcko commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2007568411)
thx, maybe in a follow-up. With two acks, this seems close to merge

💬 janb84 commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2007572387)
> thx, maybe in a follow-up. With two acks, this seems close to merge

Agreed ! Lets not invalidate the ACKS over this.

💬 darosior commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2743368308)
> The complexity of doing this is likely the reason that no one has added any Tapscript tests to JSON script tests until this PR.

I think the reason no one has added more tests is because the Tapscript logic is already extensively covered through the JSON script unit tests (in additional to the extensive functional test). See this unit test (which works with [this JSON file](https://raw.githubusercontent.com/bitcoin-core/qa-assets/refs/heads/main/unit_test_data/script_assets_test.json) from t
...

💬 flack commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r2007610573)
nit: challange => challenge

🤔 furszy reviewed a pull request: "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan"
(https://github.com/bitcoin/bitcoin/pull/32023#pullrequestreview-2705977157)
Code reviewed, thanks for changing the approach. Looking good with a few nuances.

There are two more places where we call `AddWalletDescriptor` that haven't been considered:
1) `AddKey()` in `wallet_tests.cpp`.
2) `CreateSyncedWallet()` in `wallet/test/util.cpp`

💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007600350)
nano nit: maybe call it `spkm_res` to clarify that this is not the spkm anymore.

💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007597399)
not for this PR but there is no need to call `GetDescriptorScriptPubKeyMan()` here, can just return `AddWalletDescriptor()` output.

💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007604057)
We are no longer asserting that `spk_manager!=nullptr`.
Could unify these lines + the missing check:
```c++
auto spk_manager = *Assert(wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", internal));
assert(spk_manager);
```

💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007619742)
Same as above; would be good to not mix coding styles. No jump line after the condition statement.

And the `res` variable is shadowing another variable in this function. Would be better to rename it to `res_spkm` or `spkm_res`.

💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007630950)
The `res` variable is already used in this function, by declaring it here again you are shadowing the other one. Would be better to rename it to `res_spkm` or `spkm_res`.

Also, missing whitespace after `if`.

💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007611055)
Would be good to follow the same code style we have in this function. No jump line after the condition statement.

💬 sipa commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2743492906)
I do understand the desire to have accessible simple unit test cases for tapscript, and more testing certainly doesn't hurt. However, making things like this possible and easy to write was the goal of the taproot test framework, though), which has advantages like supporting combinations with non-trivial script trees, adding auto-generated signatures, integrating it all into transactions and blocks and running through the entire stack, plus having the ability to export the resulting cases to JSON
...