💬 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>
...

💬 stickies-v commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1571063727)
What's the rationale for continuing here? Should we accept invalid URLs? At first sight, I think I'd prefer having this return a `util::Result` and have the function fail in case of missing hex digits or invalid hex?

💬 stickies-v commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1571038503)
nit
```suggestion
BOOST_CHECK_EQUAL(urlDecode(encoded), result);
```

💬 stickies-v commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1571059431)
We didn't use [plus decoding](https://github.com/libevent/libevent/blob/e0a4574ba2cbcdb64bb2b593e72be7f7f4010746/http.c#L3505) with `evhttp_uridecode`, so this implementation keeps functionality the same. Since `urlDecode` is a generic utility function, I think there should be a docstring that highlights that this implementation doesn't decode `+` to ` ` in case it's used for other purposes in the future.

💬 stickies-v commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1571087728)
What do you suggest? Leave things as is? Concatenate everything into a single string? Both seem less desirable than just fixing the interface, imo. (I use the term fix since the RPC behaves different to how it is documented).

💬 stickies-v commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1571092976)
Another option is to keep `warnings` a concatenated `STR` of all messages , add a `all_warnings` `ARR` (open to better name suggestions) field, mark `warnings` as deprecated and remove it in v29?

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1571093831)
I think #28970 is handling this properly, as it's the only place where we expose package evaluation over p2p. Can just remove this commit?