💬 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.
⚠️ l0rinc opened an issue: "Fully validated AssumeUTXO starts revalidating after restart"
(https://github.com/bitcoin/bitcoin/issues/32029)
After fully validating the [recent update to AssumeUTXO until 880k](https://github.com/bitcoin/bitcoin/pull/31969#pullrequestreview-2658919045) in the background:
```bash
2025-03-08T09:58:19Z UpdateTip: new best=00000000000000000001b5d3d95fe8153004c0a8e4f5fbd08e4fead4298897ff height=886840 version=0x246ea000 log2_work=95.483816 tx=1163409635 date='2025-03-08T09:58:50Z' progress=1.000000 cache=136.3MiB(966526txo)
2025-03-08T10:00:03Z [background validation] UpdateTip: new best=0000000000000000000
...
(https://github.com/bitcoin/bitcoin/issues/32029)
After fully validating the [recent update to AssumeUTXO until 880k](https://github.com/bitcoin/bitcoin/pull/31969#pullrequestreview-2658919045) in the background:
```bash
2025-03-08T09:58:19Z UpdateTip: new best=00000000000000000001b5d3d95fe8153004c0a8e4f5fbd08e4fead4298897ff height=886840 version=0x246ea000 log2_work=95.483816 tx=1163409635 date='2025-03-08T09:58:50Z' progress=1.000000 cache=136.3MiB(966526txo)
2025-03-08T10:00:03Z [background validation] UpdateTip: new best=0000000000000000000
...
💬 l0rinc commented on pull request "Add mainnet assumeutxo param at height 880,000":
(https://github.com/bitcoin/bitcoin/pull/31969#issuecomment-2712065213)
I can confirm that I got to [full background validation](https://github.com/bitcoin/bitcoin/pull/31969#pullrequestreview-2658919045), but restarting the node seems to restart it - opened an issue to clarify https://github.com/bitcoin/bitcoin/issues/32029
(https://github.com/bitcoin/bitcoin/pull/31969#issuecomment-2712065213)
I can confirm that I got to [full background validation](https://github.com/bitcoin/bitcoin/pull/31969#pullrequestreview-2658919045), but restarting the node seems to restart it - opened an issue to clarify https://github.com/bitcoin/bitcoin/issues/32029
💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1988151641)
nit - you can simplify this line slightly by using the default arguments instead of passing them explicitly.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1988151641)
nit - you can simplify this line slightly by using the default arguments instead of passing them explicitly.
💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1988151782)
```suggestion
OutputGroup group = MakeCoin(input_amount);
```
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1988151782)
```suggestion
OutputGroup group = MakeCoin(input_amount);
```
💬 mzumsande commented on issue "Fully validated AssumeUTXO starts revalidating after restart":
(https://github.com/bitcoin/bitcoin/issues/32029#issuecomment-2712095682)
What do you mean with "[cancelled it]", did you abort bitcoind with Ctrl+C or similar?
As described in the [design document](https://github.com/bitcoin/bitcoin/blob/master/doc/design/assumeutxo.md), all traces of AssumeUtxo are only gone after the next restart. Not sure why the UTXO stats check is repeated, maybe to prevent circumventing the check by terminating before, or something similar.
(https://github.com/bitcoin/bitcoin/issues/32029#issuecomment-2712095682)
What do you mean with "[cancelled it]", did you abort bitcoind with Ctrl+C or similar?
As described in the [design document](https://github.com/bitcoin/bitcoin/blob/master/doc/design/assumeutxo.md), all traces of AssumeUtxo are only gone after the next restart. Not sure why the UTXO stats check is repeated, maybe to prevent circumventing the check by terminating before, or something similar.
💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2712097498)
Concept Ack to refactor the tests. `TestBnBSuccess` and `TestBnBFail` are much better and easier to read. Thanks!
> non-representative and effectuate counterintuitive behavior such as feerate = 0 or cost_of_change = 0
I'm still trying to sus out what the tests are doing now that's better than before. Most of the current tests use a cost_of_change = 0.5 * COIN, for [example](https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/src/wallet/test/coinselector_test
...
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2712097498)
Concept Ack to refactor the tests. `TestBnBSuccess` and `TestBnBFail` are much better and easier to read. Thanks!
> non-representative and effectuate counterintuitive behavior such as feerate = 0 or cost_of_change = 0
I'm still trying to sus out what the tests are doing now that's better than before. Most of the current tests use a cost_of_change = 0.5 * COIN, for [example](https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/src/wallet/test/coinselector_test
...
💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1988165735)
I spent a lot of time figuring out how `make_hard` works. Thanks for replacing it with a version that _actually_ explains itself instead of making it a mensa test.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1988165735)
I spent a lot of time figuring out how `make_hard` works. Thanks for replacing it with a version that _actually_ explains itself instead of making it a mensa test.
💬 davidgumberg commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2712132395)
> One slight problem is that Apple no longer makes XCode 15.0 available for download. They do have Xcode 15.0.1: https://download.developer.apple.com/Developer_Tools/Xcode_15.0.1/Xcode_15.0.1.xip
I'm still seeing it here: https://developer.apple.com/download/all/?q=Xcode%2015 , and with this download link: https://download.developer.apple.com/Developer_Tools/Xcode_15/Xcode_15.xip, it's labeled `Xcode 15` rather than `Xcode 15.0`.
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2712132395)
> One slight problem is that Apple no longer makes XCode 15.0 available for download. They do have Xcode 15.0.1: https://download.developer.apple.com/Developer_Tools/Xcode_15.0.1/Xcode_15.0.1.xip
I'm still seeing it here: https://developer.apple.com/download/all/?q=Xcode%2015 , and with this download link: https://download.developer.apple.com/Developer_Tools/Xcode_15/Xcode_15.xip, it's labeled `Xcode 15` rather than `Xcode 15.0`.
💬 Aditbo commented on pull request "Add mainnet assumeutxo param at height 880,000":
(https://github.com/bitcoin/bitcoin/pull/31969#issuecomment-2712153241)
ok
Pada tanggal Sen, 3 Mar 2025 20.39, rkrux ***@***.***>
menulis:
> Well, this pull request is not completely unrelated to this topic but in
> order to have a more concrete discussion, I will create a Github issue for
> it & we can move the discussion there.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/pull/31969#issuecomment-2694412711>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BNU3IQBLQTFSNQIQMWO3VCT2
...
(https://github.com/bitcoin/bitcoin/pull/31969#issuecomment-2712153241)
ok
Pada tanggal Sen, 3 Mar 2025 20.39, rkrux ***@***.***>
menulis:
> Well, this pull request is not completely unrelated to this topic but in
> order to have a more concrete discussion, I will create a Github issue for
> it & we can move the discussion there.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/pull/31969#issuecomment-2694412711>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BNU3IQBLQTFSNQIQMWO3VCT2
...
💬 yancyribbens commented on pull request "wallet: Replace "non-0" with "non-zero" in translatable error message":
(https://github.com/bitcoin/bitcoin/pull/31987#discussion_r1988190791)
```suggestion
// and no pre-selected inputs. This will result in zero-input transaction, which is consensus-invalid anyways
```
(https://github.com/bitcoin/bitcoin/pull/31987#discussion_r1988190791)
```suggestion
// and no pre-selected inputs. This will result in zero-input transaction, which is consensus-invalid anyways
```
💬 davidgumberg commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2712722396)
Tested ACK https://github.com/bitcoin/bitcoin/commit/515c64c743d581f84b8be45e5d16968d02264389
Broke a unit test on windows and this branch [caught it](https://github.com/davidgumberg/bitcoin/actions/runs/13780098004/job/38538199806), broke a functional test and this branch [caught it](https://github.com/davidgumberg/bitcoin/actions/runs/13780115705/job/38538229660).
Unfortunate that there isn't a way to validate locally that changes won't break Windows builds, but the wine tests seem to h
...
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2712722396)
Tested ACK https://github.com/bitcoin/bitcoin/commit/515c64c743d581f84b8be45e5d16968d02264389
Broke a unit test on windows and this branch [caught it](https://github.com/davidgumberg/bitcoin/actions/runs/13780098004/job/38538199806), broke a functional test and this branch [caught it](https://github.com/davidgumberg/bitcoin/actions/runs/13780115705/job/38538229660).
Unfortunate that there isn't a way to validate locally that changes won't break Windows builds, but the wine tests seem to h
...
📝 xiaobei0715 opened a pull request: "chore: remove incorrect punctuation marks"
(https://github.com/bitcoin/bitcoin/pull/32030)
(https://github.com/bitcoin/bitcoin/pull/32030)
💬 fanquake 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#discussion_r1988501276)
0~ This seems far too verbose/exposing implementation details, and we don't do this for any other dep.
(https://github.com/bitcoin/bitcoin/pull/32019#discussion_r1988501276)
0~ This seems far too verbose/exposing implementation details, and we don't do this for any other dep.