👋 hebasto's pull request is ready for review: "ci, iwyu: Treat warnings as errors for specific targets"
(https://github.com/bitcoin/bitcoin/pull/31308)
(https://github.com/bitcoin/bitcoin/pull/31308)
💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific targets":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-2901917485)
Rebased.
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-2901917485)
Rebased.
🚀 fanquake merged a pull request: "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage"
(https://github.com/bitcoin/bitcoin/pull/32467)
(https://github.com/bitcoin/bitcoin/pull/32467)
👍 fanquake approved a pull request: "ci: Downgrade DEBUG=1 to -D_GLIBCXX_ASSERTIONS in centos task"
(https://github.com/bitcoin/bitcoin/pull/32586#pullrequestreview-2861961840)
ACK fa079538e32d20aec6786c93e1117da1e8ea0cab - we can followup
(https://github.com/bitcoin/bitcoin/pull/32586#pullrequestreview-2861961840)
ACK fa079538e32d20aec6786c93e1117da1e8ea0cab - we can followup
✅ fanquake closed an issue: "intermittent issue in rpc_signer.py (enumeratesigners timeout) under GLIBCXX debug mode"
(https://github.com/bitcoin/bitcoin/issues/32524)
(https://github.com/bitcoin/bitcoin/issues/32524)
🚀 fanquake merged a pull request: "ci: Downgrade DEBUG=1 to -D_GLIBCXX_ASSERTIONS in centos task"
(https://github.com/bitcoin/bitcoin/pull/32586)
(https://github.com/bitcoin/bitcoin/pull/32586)
👋 romanz's pull request is ready for review: "rest: fetch spent transaction outputs by blockhash"
(https://github.com/bitcoin/bitcoin/pull/32540)
(https://github.com/bitcoin/bitcoin/pull/32540)
💬 fanquake commented on pull request "log: Additional compact block logging":
(https://github.com/bitcoin/bitcoin/pull/32582#issuecomment-2901996108)
cc @instagibbs
(https://github.com/bitcoin/bitcoin/pull/32582#issuecomment-2901996108)
cc @instagibbs
💬 jonasschnelli commented on issue "seeds: seed.bitcoin.jonasschnelli.ch not returning results":
(https://github.com/bitcoin/bitcoin/issues/32590#issuecomment-2902032188)
Fixed.
(https://github.com/bitcoin/bitcoin/issues/32590#issuecomment-2902032188)
Fixed.
💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2103080376)
The server timer *should* start after receiving the last packet from the client, because its an idle timer not a session timer. So its more correct to have:
> send, server timer starts immediately after (B)
I tried to to nail this in https://github.com/bitcoin/bitcoin/commit/ce9847e284a361fb050f366848cdf1a38b2b933b but that also failed.
And then, if we start the timer too early, we will always get a duration > 1 no matter what the server actually does... maybe its the initial TCP connec
...
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2103080376)
The server timer *should* start after receiving the last packet from the client, because its an idle timer not a session timer. So its more correct to have:
> send, server timer starts immediately after (B)
I tried to to nail this in https://github.com/bitcoin/bitcoin/commit/ce9847e284a361fb050f366848cdf1a38b2b933b but that also failed.
And then, if we start the timer too early, we will always get a duration > 1 no matter what the server actually does... maybe its the initial TCP connec
...
🤔 instagibbs reviewed a pull request: "log: Additional compact block logging"
(https://github.com/bitcoin/bitcoin/pull/32582#pullrequestreview-2862076315)
concept ACK
seem like sensible things to be reporting
(https://github.com/bitcoin/bitcoin/pull/32582#pullrequestreview-2862076315)
concept ACK
seem like sensible things to be reporting
💬 instagibbs commented on pull request "log: Additional compact block logging":
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2103091393)
seems odd to add it then modify it immediately next commit, can be one commit?
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2103091393)
seems odd to add it then modify it immediately next commit, can be one commit?
💬 instagibbs commented on pull request "log: Additional compact block logging":
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2103089215)
to make it more explicit it's not a tx count thing
```Suggestion
LogDebug(BCLog::CMPCTBLOCK, "Initializing PartiallyDownloadedBlock for block %s using a cmpctblock of %lu bytes\n", cmpctblock.header.GetHash().ToString(), GetSerializeSize(cmpctblock));
```
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2103089215)
to make it more explicit it's not a tx count thing
```Suggestion
LogDebug(BCLog::CMPCTBLOCK, "Initializing PartiallyDownloadedBlock for block %s using a cmpctblock of %lu bytes\n", cmpctblock.header.GetHash().ToString(), GetSerializeSize(cmpctblock));
```
💬 instagibbs commented on pull request "log: Additional compact block logging":
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2103092771)
we don't know they were missing them really, just mention they asked for them for the specific block?
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2103092771)
we don't know they were missing them really, just mention they asked for them for the specific block?
🤔 ryanofsky reviewed a pull request: "bitcoin-cli: Add -ipcconnect option"
(https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-2859167293)
Great review! I improved a number of things in response to your comments.
Rebased 9ff5bc75659e0314f42c1477fc0c37ac0ad132fa -> 6bf727fb88009f99dab7c06bff7f8fb391147ac1 ([`pr/ipc-cli.5`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-cli.5) -> [`pr/ipc-cli.6`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-cli.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-cli.5-rebase..pr/ipc-cli.6)) due to conflict with #28710, also made a number of improvements to -ipcbind docume
...
(https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-2859167293)
Great review! I improved a number of things in response to your comments.
Rebased 9ff5bc75659e0314f42c1477fc0c37ac0ad132fa -> 6bf727fb88009f99dab7c06bff7f8fb391147ac1 ([`pr/ipc-cli.5`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-cli.5) -> [`pr/ipc-cli.6`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-cli.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-cli.5-rebase..pr/ipc-cli.6)) due to conflict with #28710, also made a number of improvements to -ipcbind docume
...
💬 ryanofsky commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101199458)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070591693
> I notice this `ExecuteHTTPRPC()` bypasses our only authorization barrier for RPC which is an HTTP header, maybe you deal with this in another commit but that should probably be documented in the `ipcbind` help text.
Makes sense, updated ipcbind help text to mention RPC authentication.
> You also left behind the `jreq.authUser` check for RPC whitelist -- which I believe only gets filled in by `HTTPReq_JSONRPC()`.
...
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101199458)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070591693
> I notice this `ExecuteHTTPRPC()` bypasses our only authorization barrier for RPC which is an HTTP header, maybe you deal with this in another commit but that should probably be documented in the `ipcbind` help text.
Makes sense, updated ipcbind help text to mention RPC authentication.
> You also left behind the `jreq.authUser` check for RPC whitelist -- which I believe only gets filled in by `HTTPReq_JSONRPC()`.
...
💬 ryanofsky commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101228151)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070587369
> Might want to indicate `status` is an "out" param?
Thanks, expanded comment.
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101228151)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070587369
> Might want to indicate `status` is an "out" param?
Thanks, expanded comment.
💬 ryanofsky commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101223433)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070617845
> Do we ever use this return value here on on master? It looks like it just gets swallowed inside `HTTPWorkItem`
Nice catch, dropped the unused return value.
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101223433)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070617845
> Do we ever use this return value here on on master? It looks like it just gets swallowed inside `HTTPWorkItem`
Nice catch, dropped the unused return value.
💬 ryanofsky commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101237982)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070707092
> I presume you can ignore `rpcwait` because the socket won't exist until the server is running?
Not really, this is a good catch. I was just ignoring rpcwait option before but updated the code now so it will work for both RPC and IPC. Now the connection failed case is handled the same for both.
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101237982)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070707092
> I presume you can ignore `rpcwait` because the socket won't exist until the server is running?
Not really, this is a good catch. I was just ignoring rpcwait option before but updated the code now so it will work for both RPC and IPC. Now the connection failed case is handled the same for both.
💬 ryanofsky commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101257896)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070730905
> Hm for some reason I thought it could be as low as 92:
You are right. I added some code to fall back to 92 if this is not a known platform with a higher limit. This could use 92 everywhere, but since test coverage here is lost when the datadir path length exceeds this value, I think it is good to run the extra checks more places if the platform does support a higher limit.
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2101257896)
re: https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2070730905
> Hm for some reason I thought it could be as low as 92:
You are right. I added some code to fall back to 92 if this is not a known platform with a higher limit. This could use 92 everywhere, but since test coverage here is lost when the datadir path length exceeds this value, I think it is good to run the extra checks more places if the platform does support a higher limit.