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

💬 achow101 commented on pull request "util: remove unused cpp-subprocess options":
(https://github.com/bitcoin/bitcoin/pull/29865#discussion_r1571141464)
In 0f847336428153042dbf226caf52ecd43188f22e "remove unneeded environment option from cpp-subprocess"

This variable seems to also be unused now as it is never set to anything. It's only passed in as an argument to `CreateProcessW` which could take the nullptr directly.

💬 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_r1571153002)
i agree that introducing a new name is unfortunate here, as "warnings" is consistent with naming for the wallet warnings
a deprecatedrpc flag that switches the format would work, i suppose

💬 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_r1571164754)
`-deprecatedrpc=warnings` is acceptable, but not ideal

💬 ariard commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2064843397)
> An update to @petertodd's stats from July, at least 97.7 % of hashrate (monthly average) is now running FullRBF based on FullRBF'd transactions from within the past 24 hours.

97.7 % of hashrate is just overwhelming in matters of hashrate support for `mempoolfullrbf`. I’m still curious if anyone has data on the additional accumulated absolute fee traffic yielded by non-opting replacement that would have not propagated under lack of `mempoolfullrbf` support, since we introduced it in Bitcoin
...

💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#issuecomment-2064887943)
Addressed @laanwj 's comment and removed the hooking.

FWIW, I wondered for a moment why this is in common and not util but that was a conscious decision, see #26294.

> yes please... the hooking we do for `URL_DECODE` and `UrlDecodeFn` made me kind of scared, to be honest (in that context), i think can we get rid of that at the same time?

Absolutely, I hadn't even seen that yet. For reviewers that wonder as well, the point of this was to not have the wallet depend on `libevent`, see #185
...