💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143154239)
in commit 92dbf055528f16fcf51ea4bb310b9f319796d57f
I took another look at this and it seems this isn't a bugfix, but a workaround to silence a clang-tidy warning.
My understanding is that `FakeCheckCheckCompletion` can't be moved/swapped more efficiently than (default)constructing it, since it is just an empty struct. However, the underlying vector that holds the objects still needs to malloc (according to the comment you deleted).
I think an alternative fix would be to simply call `cle
...
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143154239)
in commit 92dbf055528f16fcf51ea4bb310b9f319796d57f
I took another look at this and it seems this isn't a bugfix, but a workaround to silence a clang-tidy warning.
My understanding is that `FakeCheckCheckCompletion` can't be moved/swapped more efficiently than (default)constructing it, since it is just an empty struct. However, the underlying vector that holds the objects still needs to malloc (according to the comment you deleted).
I think an alternative fix would be to simply call `cle
...
💬 glozow commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477583517)
There seems to be confusion here. This PR does not touch the fee estimator or the wallet in any way. The fee estimator already keeps track of feerates internally; the wallet queries it directly for feerate estimates. I don't know of any examples where the wallet needs to see a graph of something.
If you feel that the fee estimation algorithms need improvement, by all means I'd say some review/PRs there would be welcome. Maybe take a look at #25380 or #21161.
For those that feel very passio
...
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477583517)
There seems to be confusion here. This PR does not touch the fee estimator or the wallet in any way. The fee estimator already keeps track of feerates internally; the wallet queries it directly for feerate estimates. I don't know of any examples where the wallet needs to see a graph of something.
If you feel that the fee estimation algorithms need improvement, by all means I'd say some review/PRs there would be welcome. Maybe take a look at #25380 or #21161.
For those that feel very passio
...
💬 ismaelsadeeq commented on pull request "test: Fix TypeError (expected str instance, bytes found) in wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/27280#issuecomment-1477586167)
> Can be tested with:
>
> ```diff
> diff --git a/test/functional/feature_init.py b/test/functional/feature_init.py
> index 70a802dc58..1d1c6a64b6 100755
> --- a/test/functional/feature_init.py
> +++ b/test/functional/feature_init.py
> @@ -52,7 +52,7 @@ class InitStressTest(BitcoinTestFramework):
> assert_equal(200, node.getblockcount())
>
> lines_to_terminate_after = [
> - b'Validating signatures for all blocks',
> + b'.Validating signat
...
(https://github.com/bitcoin/bitcoin/pull/27280#issuecomment-1477586167)
> Can be tested with:
>
> ```diff
> diff --git a/test/functional/feature_init.py b/test/functional/feature_init.py
> index 70a802dc58..1d1c6a64b6 100755
> --- a/test/functional/feature_init.py
> +++ b/test/functional/feature_init.py
> @@ -52,7 +52,7 @@ class InitStressTest(BitcoinTestFramework):
> assert_equal(200, node.getblockcount())
>
> lines_to_terminate_after = [
> - b'Validating signatures for all blocks',
> + b'.Validating signat
...
💬 0xB10C commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477586224)
It's confusing to me why this PR, tagged as `[RPC/REST/ZMQ]` and only touching `src/rest.cpp` and files in `src/rpc/*`, should have anything to do with the wallet or the GUI. AFAIK the wallet doesn't use the RPC interface to communicate with bitcoind. If the wallet or the GUI would want a histogram, it couldn't use the code added in this PR.
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477586224)
It's confusing to me why this PR, tagged as `[RPC/REST/ZMQ]` and only touching `src/rest.cpp` and files in `src/rpc/*`, should have anything to do with the wallet or the GUI. AFAIK the wallet doesn't use the RPC interface to communicate with bitcoind. If the wallet or the GUI would want a histogram, it couldn't use the code added in this PR.
💬 kristapsk commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477594252)
> If the wallet or the GUI would want a histogram, it couldn't use the code added in this PR.
That would require just moving fee histogram code out of `MempoolInfoToJSON()` to a separate function somewhere out of `rpc/`, code itself seems to me reusable.
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477594252)
> If the wallet or the GUI would want a histogram, it couldn't use the code added in this PR.
That would require just moving fee histogram code out of `MempoolInfoToJSON()` to a separate function somewhere out of `rpc/`, code itself seems to me reusable.
💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143174094)
(No idea if this makes a difference, the compiler may optimize everything into the same binary anyway)
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143174094)
(No idea if this makes a difference, the compiler may optimize everything into the same binary anyway)
💬 darosior commented on pull request "p2p: remove adjusted time":
(https://github.com/bitcoin/bitcoin/pull/25908#issuecomment-1477612785)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/25908#issuecomment-1477612785)
Concept ACK.
💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143197655)
in commit b43ef231d241e7f7ef7c7685a9ca7a6274d1b943
Any reason to keep the unused and dangerous `CScriptCheck` default constructor? (For example, it leaves `txdata` completely uninitialized?)
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143197655)
in commit b43ef231d241e7f7ef7c7685a9ca7a6274d1b943
Any reason to keep the unused and dangerous `CScriptCheck` default constructor? (For example, it leaves `txdata` completely uninitialized?)
👍 willcl-ark approved a pull request: "Support JSON-RPC 2.0 when requested by client"
(https://github.com/bitcoin/bitcoin/pull/27101)
ACK 2da823199
Left a few comments which could be addressed here, in a followup or not at all. None of them materially affect the current implementation of this feature which works well.
Although, I would be curious to know why we are still responding with `http 500` errors for invalid json after the move to json 2.0.
(https://github.com/bitcoin/bitcoin/pull/27101)
ACK 2da823199
Left a few comments which could be addressed here, in a followup or not at all. None of them materially affect the current implementation of this feature which works well.
Although, I would be curious to know why we are still responding with `http 500` errors for invalid json after the move to json 2.0.
💬 willcl-ark commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1142695923)
Were we not using the out-of-spec `"jsonrpc": "1.0"` here previously?
edit: It seems it doesn't make any difference as was not being validated? 🤷🏼♂️
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1142695923)
Were we not using the out-of-spec `"jsonrpc": "1.0"` here previously?
edit: It seems it doesn't make any difference as was not being validated? 🤷🏼♂️
💬 willcl-ark commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1143113513)
Note that we do actually test json-rpc error code `-32602` in a different test, which might be worth documenting here?
https://github.com/bitcoin/bitcoin/blob/1cc1c7210696e3ea758ea5e390eafe824cd06a9e/test/functional/p2p_disconnect_ban.py#L111
Similarly `-32603` is tested here:
https://github.com/bitcoin/bitcoin/blob/1cc1c7210696e3ea758ea5e390eafe824cd06a9e/test/functional/feature_fee_estimation.py#L335
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1143113513)
Note that we do actually test json-rpc error code `-32602` in a different test, which might be worth documenting here?
https://github.com/bitcoin/bitcoin/blob/1cc1c7210696e3ea758ea5e390eafe824cd06a9e/test/functional/p2p_disconnect_ban.py#L111
Similarly `-32603` is tested here:
https://github.com/bitcoin/bitcoin/blob/1cc1c7210696e3ea758ea5e390eafe824cd06a9e/test/functional/feature_fee_estimation.py#L335
💬 willcl-ark commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1142690986)
Should this not be a `status: 200, error: -32602` under json 2.0?
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1142690986)
Should this not be a `status: 200, error: -32602` under json 2.0?
💬 willcl-ark commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1143114171)
Why do we choose `status: 500, error: -32700` here vs `status: 200, error: -32700`? I feel inclined to argue that we didn't experience an http server error, this is just a parsing error.
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1143114171)
Why do we choose `status: 500, error: -32700` here vs `status: 200, error: -32700`? I feel inclined to argue that we didn't experience an http server error, this is just a parsing error.
💬 willcl-ark commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1143119006)
Perhaps in demonstrating best-practice it could be nice to not use a `null` value for `id` in the request, so we can avoid the `null` response. Per json-rpc 2.0 an `id: null` request is _supposed_ to be a "notification", which does not get responded to (although I think we should ignore actually parsing them like this interally, and respond to all requests).
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1143119006)
Perhaps in demonstrating best-practice it could be nice to not use a `null` value for `id` in the request, so we can avoid the `null` response. Per json-rpc 2.0 an `id: null` request is _supposed_ to be a "notification", which does not get responded to (although I think we should ignore actually parsing them like this interally, and respond to all requests).
💬 willcl-ark commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1143200285)
Just a behaviour quirk I noticed in mutating these a little for testing: we can actually service batch requests which consist of a mixture of 1.0 and 2.0 requests. This test itself will fail, as it's asserting that `error` is not present (which if one is changed to 1.0 it is), but the server can handle it. Not that I think it's a particularly realistic or common situation...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1143200285)
Just a behaviour quirk I noticed in mutating these a little for testing: we can actually service batch requests which consist of a mixture of 1.0 and 2.0 requests. This test itself will fail, as it's asserting that `error` is not present (which if one is changed to 1.0 it is), but the server can handle it. Not that I think it's a particularly realistic or common situation...
💬 MarcoFalke commented on pull request "doc: FreeBSD DataDirectoryGroupReadable Setting":
(https://github.com/bitcoin/bitcoin/pull/26741#issuecomment-1477670624)
The changes in the first commit seem to be removed in the second commit? If so, please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
(https://github.com/bitcoin/bitcoin/pull/26741#issuecomment-1477670624)
The changes in the first commit seem to be removed in the second commit? If so, please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477680746)
Updated ebfc2a2dddedcbf384f435716d30d8418a77505b -> c7af97113459f50cf33882fa18909a6109e914f4 ([pr26749.13](https://github.com/hebasto/bitcoin/commits/pr26749.13) -> [pr26749.14](https://github.com/hebasto/bitcoin/commits/pr26749.14), [diff](https://github.com/hebasto/bitcoin/compare/pr26749.13..pr26749.14)).
Addressed @MarcoFalke's:
- https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143154239
- https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143197655
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477680746)
Updated ebfc2a2dddedcbf384f435716d30d8418a77505b -> c7af97113459f50cf33882fa18909a6109e914f4 ([pr26749.13](https://github.com/hebasto/bitcoin/commits/pr26749.13) -> [pr26749.14](https://github.com/hebasto/bitcoin/commits/pr26749.14), [diff](https://github.com/hebasto/bitcoin/compare/pr26749.13..pr26749.14)).
Addressed @MarcoFalke's:
- https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143154239
- https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143197655
💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143242170)
I can see no reasons to keep i. [Removed](https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477680746).
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143242170)
I can see no reasons to keep i. [Removed](https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477680746).
💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143243113)
> I think an alternative fix would be to simply call `clear()` before `resize()`, and maybe even `reserve()`/construct with the max capacity on construction. This should reduce `malloc` to a single call at `vChecks` construction. Also, this should make `clang-tidy` happy, because of `clear()`. Finally, `clear()` should not change the capacity.
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477680746).
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143243113)
> I think an alternative fix would be to simply call `clear()` before `resize()`, and maybe even `reserve()`/construct with the max capacity on construction. This should reduce `malloc` to a single call at `vChecks` construction. Also, this should make `clang-tidy` happy, because of `clear()`. Finally, `clear()` should not change the capacity.
Thanks! [Updated](https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477680746).
💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477691222)
Might be good to keep the clang-tidy related change in a separate commit, because it is independent (it is independent, because it can be applied on current master unreleated to the changes here in this pull)?
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477691222)
Might be good to keep the clang-tidy related change in a separate commit, because it is independent (it is independent, because it can be applied on current master unreleated to the changes here in this pull)?