š¬ 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.
(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).
(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?
(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?
(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.
(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.
(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?
(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.
(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
(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
(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
...
(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
...
(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
...
š¬ mzumsande commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#discussion_r1571211066)
> Any reason not to use ActiveTip() instead of m_best_header here
My main reason is that `CheckBlockIndex()` being independent from the active chainstate makes the performance improvement applicable to situations where we have all the headers but haven't validated much of the chain: See [this old answer](https://github.com/bitcoin/bitcoin/pull/28339#discussion_r1307561848). IBD and reindex seem much more relevant than extreme `invalidateblock` scenarios.
(https://github.com/bitcoin/bitcoin/pull/28339#discussion_r1571211066)
> Any reason not to use ActiveTip() instead of m_best_header here
My main reason is that `CheckBlockIndex()` being independent from the active chainstate makes the performance improvement applicable to situations where we have all the headers but haven't validated much of the chain: See [this old answer](https://github.com/bitcoin/bitcoin/pull/28339#discussion_r1307561848). IBD and reindex seem much more relevant than extreme `invalidateblock` scenarios.
š¬ fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1571211295)
I also found this a bit strange but it's the behavior of libevent and they have an explicit test case for this. I didn't investigate this further yet and I think the safest option is to keep this a pure refactor for now.
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1571211295)
I also found this a bit strange but it's the behavior of libevent and they have an explicit test case for this. I didn't investigate this further yet and I think the safest option is to keep this a pure refactor for now.
š¬ stratospher commented on pull request "test: Fix intermittent issue in p2p_handshake.py":
(https://github.com/bitcoin/bitcoin/pull/29898#discussion_r1571231602)
> Another thing we could do is to not only do peer.wait_for_disconnect() but also wait until the bitcoind side has disconnected - something like self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == X). Haven't tried that though.
yeah this sounds like a better option! updated the PR to use this approach.
> (that is for the duration of all tests, not the test itself) to avoid port collisions.
oh makes sense, a TIL!
(https://github.com/bitcoin/bitcoin/pull/29898#discussion_r1571231602)
> Another thing we could do is to not only do peer.wait_for_disconnect() but also wait until the bitcoind side has disconnected - something like self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == X). Haven't tried that though.
yeah this sounds like a better option! updated the PR to use this approach.
> (that is for the duration of all tests, not the test itself) to avoid port collisions.
oh makes sense, a TIL!
š ryanofsky opened a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906)
This PR just contains the first two commits of #25665.
It disables copying of `util::Result` objects because unnecessary copies are inefficient and not possible after #25665, which makes `util::Result` object move-only.
It disables the assignment operator and replaces it with an `Update()` method, because #25665 adds more information to `util::Result` objects (warning and error messages and failure values) and having an assignment operator that overwrites data instead of merging it would m
...
(https://github.com/bitcoin/bitcoin/pull/29906)
This PR just contains the first two commits of #25665.
It disables copying of `util::Result` objects because unnecessary copies are inefficient and not possible after #25665, which makes `util::Result` object move-only.
It disables the assignment operator and replaces it with an `Update()` method, because #25665 adds more information to `util::Result` objects (warning and error messages and failure values) and having an assignment operator that overwrites data instead of merging it would m
...
š ryanofsky converted_to_draft a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665)
**This PR is based on #29906.**
---
Add `util::Result` support for returning more error information and make use of it in [LoadChainstate method](https://github.com/bitcoin/bitcoin/pull/25665/commits/6ec8f408b81d06354277a5ee8a53c8c724e1a927) as an initial application. Followup PR [#25722](https://github.com/bitcoin/bitcoin/pull/25722) uses it more broadly to return errors and warnings from wallet loading functions as well.
This change adds two major features to the result class:
- Fo
...
(https://github.com/bitcoin/bitcoin/pull/25665)
**This PR is based on #29906.**
---
Add `util::Result` support for returning more error information and make use of it in [LoadChainstate method](https://github.com/bitcoin/bitcoin/pull/25665/commits/6ec8f408b81d06354277a5ee8a53c8c724e1a927) as an initial application. Followup PR [#25722](https://github.com/bitcoin/bitcoin/pull/25722) uses it more broadly to return errors and warnings from wallet loading functions as well.
This change adds two major features to the result class:
- Fo
...
š¬ ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-2065005417)
Converted to draft since first 2 commits were moved to a separate PR, #29906
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-2065005417)
Converted to draft since first 2 commits were moved to a separate PR, #29906
š¬ fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1571252576)
done
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1571252576)
done
š¬ fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1571252651)
Added a short docstring.
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1571252651)
Added a short docstring.