💬 darosior commented on pull request "rpc: add target to getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1987859999)
This documentation is incomplete, as following the instructions one would run into the check for connections performed by `getblocktemplate`:
https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/src/rpc/mining.cpp#L764-L773
To actually follow these instructions on mainnet it is necessary to comment out these lines and recompile.
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1987859999)
This documentation is incomplete, as following the instructions one would run into the check for connections performed by `getblocktemplate`:
https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/src/rpc/mining.cpp#L764-L773
To actually follow these instructions on mainnet it is necessary to comment out these lines and recompile.
💬 Sjors commented on pull request "rpc: add target to getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1987869351)
Indeed I left out a few details because it would get tedious.
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1987869351)
Indeed I left out a few details because it would get tedious.
💬 Sjors commented on pull request "rpc: add target to getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1987870592)
@darosior depending on what operating system you're using, you may need one or more of these patches: https://github.com/pooler/cpuminer/pulls
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1987870592)
@darosior depending on what operating system you're using, you may need one or more of these patches: https://github.com/pooler/cpuminer/pulls
💬 Sjors commented on pull request "rpc: add target to getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1987871946)
If you do want to mine segwit or taproot outputs (which I did in the first attempt) you'll need https://github.com/pooler/cpuminer/pull/293
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1987871946)
If you do want to mine segwit or taproot outputs (which I did in the first attempt) you'll need https://github.com/pooler/cpuminer/pull/293
💬 hodlinator commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2711668773)
> Every C++ programmer should rewatch https://youtu.be/W2tWOdzgXHA from time to time.
I remembered that as "the seminal `std::rotate()`-talk", but it also has "No Raw Pointers" from 48:45. Probably stoked my hatred of smart pointers which led to #31713. To be fair, in the talk he also says that we used to do intrusive ref-counting... so this is at least one step away from that.
> Instead of passing around shared pointers to mutable data (aka global variables in disguise), would it be possi
...
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2711668773)
> Every C++ programmer should rewatch https://youtu.be/W2tWOdzgXHA from time to time.
I remembered that as "the seminal `std::rotate()`-talk", but it also has "No Raw Pointers" from 48:45. Probably stoked my hatred of smart pointers which led to #31713. To be fair, in the talk he also says that we used to do intrusive ref-counting... so this is at least one step away from that.
> Instead of passing around shared pointers to mutable data (aka global variables in disguise), would it be possi
...
💬 1440000bytes commented on issue "RFC: when to drop testnet3":
(https://github.com/bitcoin/bitcoin/issues/31975#issuecomment-2711798614)
I think testnet3 and testnet4 should be dropped together when testnet5 is available.
https://x.com/0x_orkun/status/1899192195175076144
(https://github.com/bitcoin/bitcoin/issues/31975#issuecomment-2711798614)
I think testnet3 and testnet4 should be dropped together when testnet5 is available.
https://x.com/0x_orkun/status/1899192195175076144
💬 purpleKarrot commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2711842250)
After reviewing the code line by line, I realized that the only places where `AddRef` was ever called, was directly after construction (plus one place in the fuzz tests, not sure why that is needed). It looks like the `CNode` instances are not actually shared (good news). Could it be passed around by `std::unique_ptr` instead? This would greatly simplify future refactoring.
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2711842250)
After reviewing the code line by line, I realized that the only places where `AddRef` was ever called, was directly after construction (plus one place in the fuzz tests, not sure why that is needed). It looks like the `CNode` instances are not actually shared (good news). Could it be passed around by `std::unique_ptr` instead? This would greatly simplify future refactoring.
👍 ryanofsky approved a pull request: "depends: patch around PlacementNew issue in capnp"
(https://github.com/bitcoin/bitcoin/pull/31998#pullrequestreview-2672276267)
Code review ACK fe283860b10709ef8e7a69269c70708cb85f47f3. This seems like a good change to get rid of a confusing failure that can affect debug builds and remove a complication from #29796.
(https://github.com/bitcoin/bitcoin/pull/31998#pullrequestreview-2672276267)
Code review ACK fe283860b10709ef8e7a69269c70708cb85f47f3. This seems like a good change to get rid of a confusing failure that can affect debug builds and remove a complication from #29796.
💬 ryanofsky commented on pull request "depends: patch around PlacementNew issue in capnp":
(https://github.com/bitcoin/bitcoin/pull/31998#discussion_r1988071512)
In commit "depends: patch around PlacementNew issue in capnp" (fe283860b10709ef8e7a69269c70708cb85f47f3)
This patch seems right but I can't figure out how it was generated. Commit `3007ac1e4a4bf80420471be45e25b3694025becd` does not seem to be a commit in the capnproto repository. The upstream patch is https://github.com/capnproto/capnproto/commit/74560f26f75dda4257dce541ca362a1e763b2971, which edits `c++/src/kj/common.h` and is applied with `-p2` while the patch here edits `src/kj/common.h` a
...
(https://github.com/bitcoin/bitcoin/pull/31998#discussion_r1988071512)
In commit "depends: patch around PlacementNew issue in capnp" (fe283860b10709ef8e7a69269c70708cb85f47f3)
This patch seems right but I can't figure out how it was generated. Commit `3007ac1e4a4bf80420471be45e25b3694025becd` does not seem to be a commit in the capnproto repository. The upstream patch is https://github.com/capnproto/capnproto/commit/74560f26f75dda4257dce541ca362a1e763b2971, which edits `c++/src/kj/common.h` and is applied with `-p2` while the patch here edits `src/kj/common.h` a
...
💬 purpleKarrot commented on pull request "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets":
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2711929176)
> Perhaps @purpleKarrot could suggest a better approach?
CPack?
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2711929176)
> Perhaps @purpleKarrot could suggest a better approach?
CPack?
💬 dergoegge commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2711968502)
> realized that the only places where AddRef was ever called, was directly after construction
It is also called in the RAII helper `NodesSnapshot` which is used in different threads (i.e. "net" and "msghand"), so `unique_ptr` won't work unfortunately.
> plus one place in the fuzz tests, not sure why that is needed
It's probably just there to have it "covered".
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2711968502)
> realized that the only places where AddRef was ever called, was directly after construction
It is also called in the RAII helper `NodesSnapshot` which is used in different threads (i.e. "net" and "msghand"), so `unique_ptr` won't work unfortunately.
> plus one place in the fuzz tests, not sure why that is needed
It's probably just there to have it "covered".
💬 ryanofsky commented on pull request "test: Fix authproxy named args debug logging":
(https://github.com/bitcoin/bitcoin/pull/31955#discussion_r1988094335)
re: https://github.com/bitcoin/bitcoin/pull/31955#discussion_r1977508527
> The RPC request seems to be logged with the `AuthServiceProxy.__id_count` but the corresponding non erroneous response doesn't seem to be. @maflcko Do you know the reason for it and should we improve it later?
It looks like `response["id"]` is explicitly logged in the v1 case and `responsedata` which includes the `"id"` field is logged in the v2/error case, so the id should be present in both cases. Probably output
...
(https://github.com/bitcoin/bitcoin/pull/31955#discussion_r1988094335)
re: https://github.com/bitcoin/bitcoin/pull/31955#discussion_r1977508527
> The RPC request seems to be logged with the `AuthServiceProxy.__id_count` but the corresponding non erroneous response doesn't seem to be. @maflcko Do you know the reason for it and should we improve it later?
It looks like `response["id"]` is explicitly logged in the v1 case and `responsedata` which includes the `"id"` field is logged in the v2/error case, so the id should be present in both cases. Probably output
...
👍 ryanofsky approved a pull request: "test: Fix authproxy named args debug logging"
(https://github.com/bitcoin/bitcoin/pull/31955#pullrequestreview-2672305067)
Code review ACK fac1dd9dffba1033245c283bc0468e801c14e910. Thanks for logging fix. This change should have been included in #19762
(https://github.com/bitcoin/bitcoin/pull/31955#pullrequestreview-2672305067)
Code review ACK fac1dd9dffba1033245c283bc0468e801c14e910. Thanks for logging fix. This change should have been included in #19762
💬 ryanofsky commented on pull request "test: Fix authproxy named args debug logging":
(https://github.com/bitcoin/bitcoin/pull/31955#discussion_r1988089962)
re: https://github.com/bitcoin/bitcoin/pull/31955#discussion_r1977504465
> Older code and doesn't need to be fixed in this PR but this line seems buggy to me, Idk how these 2 conditions can be true at the same time.
The code and behavior here could probably be improved, but this is just trying to reduce noise in log output and avoid showing `"error": null` in [JSONRPC v1 responses](https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/src/rpc/request.cpp#L57-L65)
...
(https://github.com/bitcoin/bitcoin/pull/31955#discussion_r1988089962)
re: https://github.com/bitcoin/bitcoin/pull/31955#discussion_r1977504465
> Older code and doesn't need to be fixed in this PR but this line seems buggy to me, Idk how these 2 conditions can be true at the same time.
The code and behavior here could probably be improved, but this is just trying to reduce noise in log output and avoid showing `"error": null` in [JSONRPC v1 responses](https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/src/rpc/request.cpp#L57-L65)
...
💬 purpleKarrot commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988117321)
This looks suspicious. No custom deleter is passed to the `shared_ptr` constructor, so it defaults to [`default_delete`](https://en.cppreference.com/w/cpp/memory/default_delete) which will `delete ptr;` eventually. Is the argument `CNode& node` guaranteed to be heap allocated? Otherwise:
```cpp
m_nodes.push_back(std::shared_ptr<CNode>(&node, [](CNode*){}));
```
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988117321)
This looks suspicious. No custom deleter is passed to the `shared_ptr` constructor, so it defaults to [`default_delete`](https://en.cppreference.com/w/cpp/memory/default_delete) which will `delete ptr;` eventually. Is the argument `CNode& node` guaranteed to be heap allocated? Otherwise:
```cpp
m_nodes.push_back(std::shared_ptr<CNode>(&node, [](CNode*){}));
```
💬 hodlinator commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988085255)
Not sure about adding non-transitive `const` here since we set `pnode->fDisconnect` inside the loop.
Similar case in `DisconnectNodes()`-loop.
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988085255)
Not sure about adding non-transitive `const` here since we set `pnode->fDisconnect` inside the loop.
Similar case in `DisconnectNodes()`-loop.
💬 hodlinator commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988125455)
Since we aren't doing anything with `pnode` anymore, we should be able to remove this `LOCK()` too, avoiding re-entrant locking inside `FindNode()`.
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988125455)
Since we aren't doing anything with `pnode` anymore, we should be able to remove this `LOCK()` too, avoiding re-entrant locking inside `FindNode()`.
💬 hodlinator commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988127685)
It is notable how unsafe the design was for the `FindNode()`-methods before switching to `shared_ptr`. Returning a raw pointer as we let go of the mutex protecting it.
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988127685)
It is notable how unsafe the design was for the `FindNode()`-methods before switching to `shared_ptr`. Returning a raw pointer as we let go of the mutex protecting it.
💬 purpleKarrot commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988141217)
This is not a raw pointer, but an observer ("Raw pointer" is coined by Sean Parent as "anything that implies ownership", which explicitly includes `shared_ptr`. Stricly speaking, changing `CNode*` to `std::shared_ptr<CNode>` turns an observer **into** a raw pointer).
I actually think that returning an observer is fine here, even after the change.
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988141217)
This is not a raw pointer, but an observer ("Raw pointer" is coined by Sean Parent as "anything that implies ownership", which explicitly includes `shared_ptr`. Stricly speaking, changing `CNode*` to `std::shared_ptr<CNode>` turns an observer **into** a raw pointer).
I actually think that returning an observer is fine here, even after the change.
💬 purpleKarrot commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2712060971)
> unique_ptr won't work unfortunately.
Got it. `m_nodes` needs to be able to be snapshotted. But that seems to be the only place where copies are made. `FindNode` should return non-owning observers rather than extend the ownership.
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2712060971)
> unique_ptr won't work unfortunately.
Got it. `m_nodes` needs to be able to be snapshotted. But that seems to be the only place where copies are made. `FindNode` should return non-owning observers rather than extend the ownership.