📝 hebasto opened a pull request: "Update `secp256k1` subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/32028)
This PR updates the `secp256k1` subtree to https://github.com/bitcoin-core/secp256k1/commit/4ba1ba2af953b7d124db9b80b34568e5c4a2d48a, which includes the following changes:
- https://github.com/bitcoin-core/secp256k1/pull/1633
- https://github.com/bitcoin-core/secp256k1/pull/1634
- https://github.com/bitcoin-core/secp256k1/pull/1641
- https://github.com/bitcoin-core/secp256k1/pull/1650
- https://github.com/bitcoin-core/secp256k1/pull/1646
- https://github.com/bitcoin-core/secp256k1/pull/165
...
(https://github.com/bitcoin/bitcoin/pull/32028)
This PR updates the `secp256k1` subtree to https://github.com/bitcoin-core/secp256k1/commit/4ba1ba2af953b7d124db9b80b34568e5c4a2d48a, which includes the following changes:
- https://github.com/bitcoin-core/secp256k1/pull/1633
- https://github.com/bitcoin-core/secp256k1/pull/1634
- https://github.com/bitcoin-core/secp256k1/pull/1641
- https://github.com/bitcoin-core/secp256k1/pull/1650
- https://github.com/bitcoin-core/secp256k1/pull/1646
- https://github.com/bitcoin-core/secp256k1/pull/165
...
💬 hebasto commented on pull request "[POC] build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#issuecomment-2711376386)
Rebased on https://github.com/bitcoin/bitcoin/pull/32028.
(https://github.com/bitcoin/bitcoin/pull/31507#issuecomment-2711376386)
Rebased on https://github.com/bitcoin/bitcoin/pull/32028.
🤔 mzumsande reviewed a pull request: "qa: Fix TxIndex race conditions"
(https://github.com/bitcoin/bitcoin/pull/32010#pullrequestreview-2671833519)
Code Review ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b
(https://github.com/bitcoin/bitcoin/pull/32010#pullrequestreview-2671833519)
Code Review ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b
💬 purpleKarrot commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2711463453)
Instead of passing around shared pointers to mutable data (aka global variables in disguise), would it be possible to increase the level of abstraction and pass around values with regular semantics? Every C++ programmer should rewatch https://youtu.be/W2tWOdzgXHA from time to time.
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2711463453)
Instead of passing around shared pointers to mutable data (aka global variables in disguise), would it be possible to increase the level of abstraction and pass around values with regular semantics? Every C++ programmer should rewatch https://youtu.be/W2tWOdzgXHA from time to time.
💬 yancyribbens commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1987820088)
Is this documented anywhere? This is a good question and I would imagine others will have the same in the future.
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1987820088)
Is this documented anywhere? This is a good question and I would imagine others will have the same in the future.
💬 yancyribbens commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1987822504)
This method name looks funky. SingleTxTXO is short for single transaction transaction output?
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1987822504)
This method name looks funky. SingleTxTXO is short for single transaction transaction output?
💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1987847360)
nit - this should be indented. Right now it's mis-aligned.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1987847360)
nit - this should be indented. Right now it's mis-aligned.
💬 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