💬 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)?
💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-1477694019)
The linter started [complaining](https://cirrus-ci.com/task/5633626411368448?logs=lint#L224) about "E275 missing whitespace after keyword". Fixed this up in the individual commits.
ac438b10918ef05df2e564734d879699c8aa18f9 -> 6de4fc73c935baa2cc6f54dd57c8e57a5df3c7ae ([2022-05-connection-tracepoints.pr.1](https://github.com/0xB10C/bitcoin/tree/2022-05-connection-tracepoints.pr.1) -> [2022-05-connection-tracepoints.pr.2](https://github.com/0xB10C/bitcoin/tree/2022-05-connection-tracepoints.pr.2)
...
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-1477694019)
The linter started [complaining](https://cirrus-ci.com/task/5633626411368448?logs=lint#L224) about "E275 missing whitespace after keyword". Fixed this up in the individual commits.
ac438b10918ef05df2e564734d879699c8aa18f9 -> 6de4fc73c935baa2cc6f54dd57c8e57a5df3c7ae ([2022-05-connection-tracepoints.pr.1](https://github.com/0xB10C/bitcoin/tree/2022-05-connection-tracepoints.pr.1) -> [2022-05-connection-tracepoints.pr.2](https://github.com/0xB10C/bitcoin/tree/2022-05-connection-tracepoints.pr.2)
...
💬 fanquake commented on issue "test: `wallet_importdescriptors.py --descriptors` failure":
(https://github.com/bitcoin/bitcoin/issues/27282#issuecomment-1477699527)
See also https://gist.github.com/fanquake/a458badc73abb47f8c06f009d15e1916 (combined log).
(https://github.com/bitcoin/bitcoin/issues/27282#issuecomment-1477699527)
See also https://gist.github.com/fanquake/a458badc73abb47f8c06f009d15e1916 (combined log).
💬 sipa commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477705117)
Bitcoin Core has had fee estimation logic for a long time (`estimatefee` RPC was added in 2014, replaced with `estimatesmartfee` in 2015).
The Bitcoin Core wallet automatically uses this internal feerate estimation logic to decide on the fees of transactions. You need to override things, or use more low-level RPCs, to not use it.
That feerate estimation logic works by looking at how quickly mempool transactions get confirmed, over longer time windows, not by looking at the current mempool comp
...
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477705117)
Bitcoin Core has had fee estimation logic for a long time (`estimatefee` RPC was added in 2014, replaced with `estimatesmartfee` in 2015).
The Bitcoin Core wallet automatically uses this internal feerate estimation logic to decide on the fees of transactions. You need to override things, or use more low-level RPCs, to not use it.
That feerate estimation logic works by looking at how quickly mempool transactions get confirmed, over longer time windows, not by looking at the current mempool comp
...