💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2064247329)
> > There are no conflicts with #29865.
>
> Looks like there are now?

Well, there is a conflict in the code, which is being removed in #29865. It won't be a problem for either PR if the other one goes first.

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1571015884)
(deleted `GetCombinedHash`)

🤔 glozow reviewed a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2009283703)
Main changes:
- Try same-peer orphanage children (most recent first) before different-peer ones (randomized). Replaced `GetChildren` with `GetChildrenFromSamePeer` and `GetChildrenFromDifferentPeer`.
- Deleted `GetCombinedHash` because it's unused. Also deleted the tests for it
- Changed `GetPackageHash` a bit. Using single-sha256 instead of double-sha256 and using just wtxids instead of serialized vector

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1571015190)
deleted

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1571014755)
I figure it's easier to read the test this way; you can easily tell that the lexicographical ordering is what I claim it to be

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1571015988)
(deleted `GetCombinedHash`)

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1571006395)
(deleted `GetCombinedHash`)

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1571005910)
removed

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1571017712)
Did this now. We first gather all children by the same peer and try by recency order. If we cannot find one that matches, we look for children not by this peer, and try in random order.

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1571006616)
done, thanks

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1571006745)
done, thanks

🤔 hebasto reviewed a pull request: "util: remove unused cpp-subprocess options"
(https://github.com/bitcoin/bitcoin/pull/29865#pullrequestreview-2009319290)
Approach ACK 0f847336428153042dbf226caf52ecd43188f22e.

 :rocket:

I have reviewed all commits except the last one (in progress).

The CI run of the combined branch [ this PR + #29868 ] is available here: https://github.com/hebasto/bitcoin/actions/runs/8740663547/job/23984855961

💬 glozow commented on pull request "doc: update release-process.md":
(https://github.com/bitcoin/bitcoin/pull/29645#issuecomment-2064387960)
> I think the "Update repositories and websites for new version" section could be split into 2 distinct steps

Done, I split into website updates and everything else

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1570820413)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1552474458

> I have not quite grasped yet what the immediate benefit of these more generic decorators on the Messages are.

The benefit is letting the `Result` struct work with any possible `MessagesType` without placing requirements on it. The PR provides a simple default type to hold error and warning messages:

```c++
struct Messages {
std::vector<bilingual_str> errors{};
std::vector<bilingual_str> warnings{};
}
...

🤔 ryanofsky reviewed a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-2008992241)
Updated 7a4741eaf892646e9d02e440c39fbbfa03f29fc3 -> 28e20812109757e307bc29a4adbef8aae90d94a6 ([`pr/bresult2.52`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.52) -> [`pr/bresult2.53`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.53), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult2.52..pr/bresult2.53)) just improving some comments

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1570823966)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1552459477

> Reviewers might be wondering what the benefits of the ResultTraits are. They are useful for potential future extensions of the move behaviour: [3951afc](https://github.com/bitcoin/bitcoin/commit/3951afc3b708326cea653951ef331d8f96a28682) .

Thanks I added a comment to the traits struct to make this clearer. Traits are an old pattern in c++ and are commonly used. The standard library has std::allocator_traits, std::ite
...

💬 achow101 commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1571053657)
We should stop gratuitously breaking the RPC interface by changing existing field types like this.

💬 achow101 commented on pull request "doc: update release-process.md":
(https://github.com/bitcoin/bitcoin/pull/29645#issuecomment-2064448046)
ACK 1ea8674316f2dce0005f6094b6ee111b045dd770

💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1571063644)
For some code deduplication you could just have `GetChildrenFromPeer` which takes `std::optional<NodeId>`, returning full `std::vector<std::pair<CTransactionRef, NodeId>>` list if `std::nullopt` or filtered to the specific nodeid otherwise. You can call it filtered once, then if a suitable orphan isn't found, call it again unfiltered.

🤔 stickies-v reviewed a pull request: "refactor: Use our own implementation of urlDecode"
(https://github.com/bitcoin/bitcoin/pull/29904#pullrequestreview-2009339803)
Approach ACK

Since `urlDecode` no longer needs libevent, we can clean up the workaround introduced in https://github.com/bitcoin/bitcoin/pull/18504 too:

<details>
<summary>git diff on 2447bc7774</summary>

```diff
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 129deeec60..830171f63a 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -11,7 +11,6 @@
#include <clientversion.h>
#include <common/args.h>
#include <common/system.h>
-#include <common/url.h>
...