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

💬 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_r1571106475)
> 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?

Yes, actually. We did this for various `warning` fields in the wallet in #27279.

💬 laanwj commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1571120307)
Yes, that's usually how these changes are done: add a new field with the new format and a new name, keep or emulate the old format (eg concatenate the warnings), deprecate the old field after a few releases.

💬 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_r1571138927)
I don't think adding a new field makes sense here. `warnings` is the best name, unlike #27279 where we upgraded from `warning` to `warnings`, this PR is more of a bugfix. Is it fair to assume you'd both be happy with keeping default behaviour as it is in this PR but allowing users to revert `warnings` to a (concatenated) string with a `-deprecatedrpc=warning` option, until we remove it in some future version?