💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592580254)
> This example shows that replacing `std::optional<ErrorType>` with `util::Expected<void, ErrorType>` is not totally risk-free as the old condition would still compile but be inverted.

Yes, the inversions is one of the reasons to do this change. I agree there is a risk, but is should be pretty obvious for reviewers to look out for. Also, hopefully the cases are rare where the value type and the error type are close enough to make `value()` and `error()` swappable at the call-site.

> constr
...

💬 maflcko commented on issue "mptest hangs, when run in a loop":
(https://github.com/bitcoin/bitcoin/issues/34014#issuecomment-3616809162)
> Curious how long approximately it takes for the hang to happen?

Maybe 5-15 minutes on my system. Just make sure to saturate all CPUs on the system. (You can run the functional tests with -j 99, or compile without cccache in a loop.)

💬 fanquake commented on pull request "depends: update freetype and document remaining `bitcoin-qt` runtime libs":
(https://github.com/bitcoin/bitcoin/pull/33952#issuecomment-3616849382)
Guix Build (aarch64):
```bash
760ca6bb241b02b496785a0123cea4e9e49e03e27e2280824237bd4125ffb1d0 guix-build-41e657aacfa6/output/aarch64-linux-gnu/SHA256SUMS.part
432c63a2879ef00793c9378f64d5130752258c0d845b565c4fa7c708399be01a guix-build-41e657aacfa6/output/aarch64-linux-gnu/bitcoin-41e657aacfa6-aarch64-linux-gnu-debug.tar.gz
15c65f8720598ff04dba8d15b0b49e5ab9064f6798159b331681806a62f28426 guix-build-41e657aacfa6/output/aarch64-linux-gnu/bitcoin-41e657aacfa6-aarch64-linux-gnu.tar.gz
d53c1e
...

💬 maflcko commented on issue "mptest hangs, when run in a loop":
(https://github.com/bitcoin/bitcoin/issues/34014#issuecomment-3616849854)
> I also saw failures in the "thread busy" test on i686 here

I tried reproducing those in `podman run -it --rm --platform linux/i386 alpine:3.23` (with the steps from above), and also in `podman run -it --rm --platform linux/i386 debian:unstable` (using `FILE_ENV="./ci/test/00_setup_env_native_nowallet.sh"` or `FILE_ENV="./ci/test/00_setup_env_native_previous_releases.sh"`), but they wouldn't segfault, only hang.

Only the modified `FILE_ENV="./ci/test/00_setup_env_i686_no_ipc.sh"` would reprod
...

💬 hodlinator commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592629697)
Requires `#include <cassert>`.

🚀 fanquake merged a pull request: "test: add unit test coverage for the empty leaves path in MerkleComputation"
(https://github.com/bitcoin/bitcoin/pull/33858)

💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2592634616)
its done thanks for updated test cases.

💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2592636114)
Hi @achow101 regrading this what do you think "The changes in test/functional/wallet_transactiontime_rescan.py can be removed IMHO."

💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2592660547)
I tried add the function test cases but the python was giving an exception `raise JSONRPCException(response['error'], status)` Then I debug the issue in `throw JSONRPCError(RPC_TYPE_ERROR, "timestamp should be greater than zero");` it setting if (jsonrpc_version == JSONRPCVersion::V1_LEGACY) reply.pushKV("error", NullUniValue); so in authproxy.py ` if 'error' in response:
raise JSONRPCException(response['error'], status)` checking if error is there or not the
...

💬 0xB10C commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2592674783)
Will change this to LogInfo once I figured out a better approach (or when I rebase on merged #29641).

💬 maflcko commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2592684221)
I see. I guess they could also be fixed. Either with a patch in iwyu, since it is already patched, or with `modernize-deprecated-headers` clang-tidy-diff, or with a stand-alone mapping script: `./ci/test/modernize-deprecated-headers.py $( git diff --name-only )`

💬 0xB10C commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#issuecomment-3616919898)
> Another approach could be to exempt all (not just debug) logs from a debug-enabled category? I think conceptually that makes sense, and I suspect there will be more places where this is helpful so we can minimize the workarounds needed?

That would work too, and is a way cleaner approach than duplicating stuff here. I probably won't get to picking that up anytime soon though, so if you want to open an alternative to this PR, feel free to do so!

---

I'll mark this as draft until I figur
...

📝 0xB10C converted_to_draft a pull request: "log: don't rate-limit "new peer" with -debug=net"
(https://github.com/bitcoin/bitcoin/pull/34008)
Previously, when `debug=net` is enabled, we log "New [..] peer connected" for new inbound peers with `LogPrintf`. However, `LogPrintf` will get rate-limited since https://github.com/bitcoin/bitcoin/pull/32604. When we specifically turn on `debug=net`, we don't want these log messages to be rate-limited.

To fix this, use `LogDebug` if `debug=net` is enabled. Otherwise use `LogPrintf`. This means we don't rate-limit the message with `debug=net` (due to `LogDebug` not being rate-limited) but sti
...

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592703927)
thx, added 3 includes. Let's see what the iwyu ci thinks

💬 fanquake commented on pull request "doc: document capnproto and libmultiprocess deps in 29.x":
(https://github.com/bitcoin/bitcoin/pull/33623#issuecomment-3616956249)
@ryanofsky can you circle back here?

⚠️ fanquake opened an issue: "Minor Release 30.1"
(https://github.com/bitcoin/bitcoin/issues/34015)
🚧 Milestone: https://github.com/bitcoin/bitcoin/milestone/79

Backports and other changes
- #33609
- #33997


RC 1:
- [ ] final changes #33997
- [ ] tag
- [ ] upload binaries
- [ ] announcements

<!--
RC
- [] final changes
- [] tag
- [] upload binaries
- [] announcements
-->

See [release process docs](https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md) for more details. This issue can be used to track status updates.

⚠️ fanquake pinned an issue: "Minor Release 30.1"
(https://github.com/bitcoin/bitcoin/issues/34015)
🚧 Milestone: https://github.com/bitcoin/bitcoin/milestone/79

Backports and other changes
- #33609
- #33997


RC 1:
- [ ] final changes #33997
- [ ] tag
- [ ] upload binaries
- [ ] announcements

<!--
RC
- [] final changes
- [] tag
- [] upload binaries
- [] announcements
-->

See [release process docs](https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md) for more details. This issue can be used to track status updates.

👍 ryanofsky approved a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3544661442)
Code review ACK fa80cca5dd193668300df891876287dacca6159e. Looks great! This is a minimal implementation of the std::expected class that may be marginally less efficient (doesn't have a void specialization, may do some unnecessary moves/copies) and lacks some features, but is usable and can be expanded, and is basically as simple as possible. I left some comments and suggestions below but none are important. I also updated the #25665 PR description to compare the `Result` and `Expected` classes.

...

💬 ryanofsky commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592606248)
In commit "Add util::Expected (std::expected)" (fa1eee4f46fd089d6e19b124addee7c7679585f4)

Looks like comment needs to be updated

💬 ryanofsky commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592667468)
In commit "Add util::Expected (std::expected)" (fa1eee4f46fd089d6e19b124addee7c7679585f4)

Normally T is a template parameter and internal types like this have more descriptive names. Would suggest renaming V to T to match std::expected [documentation](https://en.cppreference.com/w/cpp/utility/expected.html) and T to ValueType or similar