💬 fanquake commented on pull request "Update `secp256k1` subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/32028#issuecomment-2712912544)
> The latter is required for #31507.
If we are going to start bumping subtrees, can you at least add a PR description, given 31507 [still doesn't have one](https://github.com/bitcoin/bitcoin/pull/31507#issuecomment-2548249261).
(https://github.com/bitcoin/bitcoin/pull/32028#issuecomment-2712912544)
> The latter is required for #31507.
If we are going to start bumping subtrees, can you at least add a PR description, given 31507 [still doesn't have one](https://github.com/bitcoin/bitcoin/pull/31507#issuecomment-2548249261).
💬 saikiran57 commented on pull request "removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1988607821)
your right CanUpdateToWalletDescriptor removed completely no need now. I've used UpdateWalletDescriptor only.
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1988607821)
your right CanUpdateToWalletDescriptor removed completely no need now. I've used UpdateWalletDescriptor only.
💬 saikiran57 commented on pull request "removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1988608454)
there is no issue.
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1988608454)
there is no issue.
💬 hodlinator commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988630364)
If that is the case I think Parent goes one step too far in redefining terms.
Returning an raw C pointer is not an okay design, as another thread could potentially come along and delete whatever the returned pointer is referencing, resulting in use-after-free. This is mitigated by `FindNode` being private and node deletion being done in a controlled fashion on master. But it's a loaded foot-gun IMO.
Adding `EXCLUSIVE_LOCKS_REQUIRED()` would have been a different fix, forcing the caller to
...
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988630364)
If that is the case I think Parent goes one step too far in redefining terms.
Returning an raw C pointer is not an okay design, as another thread could potentially come along and delete whatever the returned pointer is referencing, resulting in use-after-free. This is mitigated by `FindNode` being private and node deletion being done in a controlled fashion on master. But it's a loaded foot-gun IMO.
Adding `EXCLUSIVE_LOCKS_REQUIRED()` would have been a different fix, forcing the caller to
...
💬 Sjors commented on issue "Fully validated AssumeUTXO starts revalidating after restart":
(https://github.com/bitcoin/bitcoin/issues/32029#issuecomment-2713078323)
From the logs it seems the background sync was finished and the node was cleanly shut down. But the error suggests something was missed in the shutdown sequence?
There's still some open issues related to AssumeUTXO that may or may not be relevant here: #31382, #30610, #30214.
cc @ryanofsky, @TheCharlatan
It seems very unlikely that this is related to the _new_ snapshot added in #31969.
(https://github.com/bitcoin/bitcoin/issues/32029#issuecomment-2713078323)
From the logs it seems the background sync was finished and the node was cleanly shut down. But the error suggests something was missed in the shutdown sequence?
There's still some open issues related to AssumeUTXO that may or may not be relevant here: #31382, #30610, #30214.
cc @ryanofsky, @TheCharlatan
It seems very unlikely that this is related to the _new_ snapshot added in #31969.
💬 Sjors commented on issue "RFC: when to drop testnet3":
(https://github.com/bitcoin/bitcoin/issues/31975#issuecomment-2713086735)
@1440000bytes I can't see the X post without an account (or maybe they're still dealing with a DoS0. Can you quote it or post a screenshot?
Dropping testnet4 without first deprecating it, is not the usual way we do things. It also doesn't really simplify anything, compared to e.g. releasing testnet5, deprecating testnet4 and dropping testnet3 in one release, and then dropping testnet4 in the next release.
(https://github.com/bitcoin/bitcoin/issues/31975#issuecomment-2713086735)
@1440000bytes I can't see the X post without an account (or maybe they're still dealing with a DoS0. Can you quote it or post a screenshot?
Dropping testnet4 without first deprecating it, is not the usual way we do things. It also doesn't really simplify anything, compared to e.g. releasing testnet5, deprecating testnet4 and dropping testnet3 in one release, and then dropping testnet4 in the next release.
💬 Sjors commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2713098707)
@davidgumberg ah indeed, I see it as well. You have to click on the download link in order to for the download URL to work at all, so that's probably why it didn't work for me.
So that just leaves the hash mismatch to figure out.
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2713098707)
@davidgumberg ah indeed, I see it as well. You have to click on the download link in order to for the download URL to work at all, so that's probably why it didn't work for me.
So that just leaves the hash mismatch to figure out.
💬 1440000bytes commented on issue "RFC: when to drop testnet3":
(https://github.com/bitcoin/bitcoin/issues/31975#issuecomment-2713130600)
This link should work: https://xcancel.com/0x_orkun/status/1899192195175076144
(https://github.com/bitcoin/bitcoin/issues/31975#issuecomment-2713130600)
This link should work: https://xcancel.com/0x_orkun/status/1899192195175076144
💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988726994)
I do not think that is necessary. I mean - if we would assume that the code is going to be modified in a careless way to break the logic, in other words to destroy `CNode` objects while holding `m_nodes_mutex`, then we might as well assume that the code could be modified in a way that breaks the manual reference counting. IMO the manual reference counting is more fragile and error prone.
Further, if the code is changed to destroy `CNode` objects while holding `m_nodes_mutex`, then 83 function
...
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988726994)
I do not think that is necessary. I mean - if we would assume that the code is going to be modified in a careless way to break the logic, in other words to destroy `CNode` objects while holding `m_nodes_mutex`, then we might as well assume that the code could be modified in a way that breaks the manual reference counting. IMO the manual reference counting is more fragile and error prone.
Further, if the code is changed to destroy `CNode` objects while holding `m_nodes_mutex`, then 83 function
...
💬 Sjors commented on issue "RFC: when to drop testnet3":
(https://github.com/bitcoin/bitcoin/issues/31975#issuecomment-2713211270)
> At this point, another Bitcoin testnet is becoming unusable for devs, with huge reorgs for months and another likely incoming.
> Signet isn’t an option since we need to test hashrate-related proofs for BitVM, but Testnet4 is becoming more painful to use every day.
Fork Obsever also shows frequent reorgs on testnet4: https://fork.observer/?network=4
However they don't look "huge" to me, just frequent.
...
(https://github.com/bitcoin/bitcoin/issues/31975#issuecomment-2713211270)
> At this point, another Bitcoin testnet is becoming unusable for devs, with huge reorgs for months and another likely incoming.
> Signet isn’t an option since we need to test hashrate-related proofs for BitVM, but Testnet4 is becoming more painful to use every day.
Fork Obsever also shows frequent reorgs on testnet4: https://fork.observer/?network=4
However they don't look "huge" to me, just frequent.
...
📝 volkanural opened a pull request: "Rename managing-wallets.md to managing-wallets.md."
(https://github.com/bitcoin/bitcoin/pull/32031)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/32031)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
✅ fanquake closed a pull request: "Rename managing-wallets.md to managing-wallets.md."
(https://github.com/bitcoin/bitcoin/pull/32031)
(https://github.com/bitcoin/bitcoin/pull/32031)
💬 hodlinator 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_r1988781317)
I think the code is noisy, but like that the text is precise, could skip the example though. Tried to make the code less noisy too but best I could come up with while still using `echo` was:
```
COMMAND ${CMAKE_COMMAND} -E echo &&
${CMAKE_COMMAND} -E echo "Error: NSIS not found." &&
${CMAKE_COMMAND} -E echo "Please install NSIS and/or ensure that its executable is accessible to the find_program() command." &&
${CMAKE_COMMAND} -E echo "
...
(https://github.com/bitcoin/bitcoin/pull/32019#discussion_r1988781317)
I think the code is noisy, but like that the text is precise, could skip the example though. Tried to make the code less noisy too but best I could come up with while still using `echo` was:
```
COMMAND ${CMAKE_COMMAND} -E echo &&
${CMAKE_COMMAND} -E echo "Error: NSIS not found." &&
${CMAKE_COMMAND} -E echo "Please install NSIS and/or ensure that its executable is accessible to the find_program() command." &&
${CMAKE_COMMAND} -E echo "
...
💬 l0rinc commented on issue "Fully validated AssumeUTXO starts revalidating after restart":
(https://github.com/bitcoin/bitcoin/issues/32029#issuecomment-2713478902)
> all traces of AssumeUtxo are only gone after the next restart
Yes, after fully validating I have restarted the node and it wants to validate again, which I haven't done (cancelled the process)
(https://github.com/bitcoin/bitcoin/issues/32029#issuecomment-2713478902)
> all traces of AssumeUtxo are only gone after the next restart
Yes, after fully validating I have restarted the node and it wants to validate again, which I haven't done (cancelled the process)
👍 pablomartin4btc approved a pull request: "qt: 29.0 translations update"
(https://github.com/bitcoin/bitcoin/pull/32004#pullrequestreview-2673743665)
re-ACK d95d1ddd173fef2654fa0678d1c1f0c9c5e40d93
Minor diffs against sync'ed (updated translations) `master` on:
```
src/qt/locale/bitcoin_et.ts
src/qt/locale/bitcoin_ru.ts
src/qt/locale/bitcoin_sv.ts
```
Checked that the changes on those files were done after the latest PR's refresh.
(https://github.com/bitcoin/bitcoin/pull/32004#pullrequestreview-2673743665)
re-ACK d95d1ddd173fef2654fa0678d1c1f0c9c5e40d93
Minor diffs against sync'ed (updated translations) `master` on:
```
src/qt/locale/bitcoin_et.ts
src/qt/locale/bitcoin_ru.ts
src/qt/locale/bitcoin_sv.ts
```
Checked that the changes on those files were done after the latest PR's refresh.
🤔 polespinasa reviewed a pull request: "test: Use rpc_deprecated only for testing deprecation"
(https://github.com/bitcoin/bitcoin/pull/31977#pullrequestreview-2673757627)
ACK b1c80fab0c026a40f2901958abdeedb9dd36c30d
(https://github.com/bitcoin/bitcoin/pull/31977#pullrequestreview-2673757627)
ACK b1c80fab0c026a40f2901958abdeedb9dd36c30d
💬 polespinasa commented on pull request "test: Use rpc_deprecated only for testing deprecation":
(https://github.com/bitcoin/bitcoin/pull/31977#discussion_r1988884259)
nit: update copyright
(https://github.com/bitcoin/bitcoin/pull/31977#discussion_r1988884259)
nit: update copyright
💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988909339)
Yes, the argument `CNode& node` is always heap allocated. Not sure about "guaranteed". If not heap allocated, then this will be a gross error, resulting in an immediate crash or an error report by Valgrind or a memory sanitizer.
Note that the semantic of this piece of code is not changed by this PR. It was the same before as well - it would eventually `delete` the object.
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1988909339)
Yes, the argument `CNode& node` is always heap allocated. Not sure about "guaranteed". If not heap allocated, then this will be a gross error, resulting in an immediate crash or an error report by Valgrind or a memory sanitizer.
Note that the semantic of this piece of code is not changed by this PR. It was the same before as well - it would eventually `delete` the object.
📝 Lubov66 opened a pull request: "docs: added a badge to the workflow"
(https://github.com/bitcoin/bitcoin/pull/32032)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/32032)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
💬 hebasto commented on issue "build: `-static-pie` builds no-longer working with CMake":
(https://github.com/bitcoin/bitcoin/issues/31843#issuecomment-2713769991)
> > This seems like something that needs to be solved in CMake itself.
>
> Asked here: https://discourse.cmake.org/t/static-pie-is-incompatible-with-checkpiesupported/13696
Upstream issue: https://gitlab.kitware.com/cmake/cmake/-/issues/26757.
(https://github.com/bitcoin/bitcoin/issues/31843#issuecomment-2713769991)
> > This seems like something that needs to be solved in CMake itself.
>
> Asked here: https://discourse.cmake.org/t/static-pie-is-incompatible-with-checkpiesupported/13696
Upstream issue: https://gitlab.kitware.com/cmake/cmake/-/issues/26757.