🚀 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

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

Maybe add a test for a default constructed instance (if this will be supported)

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

Is there a reason to use std::monostate{} here instead of T{}? Actual `std::expected` class seems to allow default construction

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

Might be better to use `V` instead f T here. My concern is just that with `Expected<void>` this will leak std::monostate and monostate should be a private implementation detail. Similarly for other accessors below returning `T`

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

IMO std::optional and std::variant are still appropriate to use in many cases and it would nice to keep mentioning them.

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

Should these have LIFETIMEBOUND annotations?