🤔 mzumsande reviewed a pull request: "test: headers sync timeout"
(https://github.com/bitcoin/bitcoin/pull/32677#pullrequestreview-2954748917)
Code Review ACK 8d257ed5937a47e1a60b7fe085f2e984eb60a956
(https://github.com/bitcoin/bitcoin/pull/32677#pullrequestreview-2954748917)
Code Review ACK 8d257ed5937a47e1a60b7fe085f2e984eb60a956
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2164597067)
UsageFromPeer and UsageByPeer appear to be identical functions?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2164597067)
UsageFromPeer and UsageByPeer appear to be identical functions?
🤔 pablomartin4btc reviewed a pull request: "test: add functional test for upgradewallet rpc"
(https://github.com/bitcoin/bitcoin/pull/32803#pullrequestreview-2954825150)
You need to add this (CI "no wallet..." failure) to `test/functional/rpc_upgradewallet.py`:
```
def skip_test_if_missing_module(self):
self.skip_if_no_wallet()
```
(https://github.com/bitcoin/bitcoin/pull/32803#pullrequestreview-2954825150)
You need to add this (CI "no wallet..." failure) to `test/functional/rpc_upgradewallet.py`:
```
def skip_test_if_missing_module(self):
self.skip_if_no_wallet()
```
💬 pablomartin4btc commented on pull request "test: add functional test for upgradewallet rpc":
(https://github.com/bitcoin/bitcoin/pull/32803#issuecomment-3001520895)
As @maflcko mentioned, not sure how useful would be to keep the RPC, and maybe you can create the PR for its removal. I thought perhaps you could move this test to `wallet_backwards_compatibility.py`, but again not sure about its purpose.
cc @achow101, @furszy
(https://github.com/bitcoin/bitcoin/pull/32803#issuecomment-3001520895)
As @maflcko mentioned, not sure how useful would be to keep the RPC, and maybe you can create the PR for its removal. I thought perhaps you could move this test to `wallet_backwards_compatibility.py`, but again not sure about its purpose.
cc @achow101, @furszy
🤔 hodlinator reviewed a pull request: "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)"
(https://github.com/bitcoin/bitcoin/pull/31713#pullrequestreview-2952377149)
New push in response to all concerns from https://github.com/bitcoin/bitcoin/pull/31713#pullrequestreview-2948159254
(https://github.com/bitcoin/bitcoin/pull/31713#pullrequestreview-2952377149)
New push in response to all concerns from https://github.com/bitcoin/bitcoin/pull/31713#pullrequestreview-2948159254
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163825798)
Thanks for the test! Added as the last commit (78db9e119136154bd733c95fc6379c780703234a). Had to increase the depth from `100'000` to `1000'000` for it to cause stack overflow. Could simplify `Clone()`ing.
> so this basically flattens out the tree structure to make sure the desctructor's call stack has at most 1 indirection? Would `TreeEval` complicate this too much?
I've brought back an earlier version of the destructor I had that destroys entire `vector<Node>`s at once in a separate comm
...
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163825798)
Thanks for the test! Added as the last commit (78db9e119136154bd733c95fc6379c780703234a). Had to increase the depth from `100'000` to `1000'000` for it to cause stack overflow. Could simplify `Clone()`ing.
> so this basically flattens out the tree structure to make sure the desctructor's call stack has at most 1 indirection? Would `TreeEval` complicate this too much?
I've brought back an earlier version of the destructor I had that destroys entire `vector<Node>`s at once in a separate comm
...
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163833758)
I prefer moving whole `vector`s. See reply to https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2160320908.
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163833758)
I prefer moving whole `vector`s. See reply to https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2160320908.
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163090187)
Added to reference in commit message.
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163090187)
Added to reference in commit message.
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163987318)
Broke out removal of `NodeRef` & `MakeNodeRef()` as mentioned in other comment.
Let me know if you still see potential in for a non-contrived scripted diff.
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163987318)
Broke out removal of `NodeRef` & `MakeNodeRef()` as mentioned in other comment.
Let me know if you still see potential in for a non-contrived scripted diff.
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163836697)
It got a bit complicated since `NodeRef` is internally `const` (`std::unique_ptr<const Node<Key>>`). Did it as a separate commit for now: effcf701474eacd85a24da4d1707120fc756aa91
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163836697)
It got a bit complicated since `NodeRef` is internally `const` (`std::unique_ptr<const Node<Key>>`). Did it as a separate commit for now: effcf701474eacd85a24da4d1707120fc756aa91
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163180665)
Was doing that in earlier pushes of the PR but changed back to `!= nullopt` since it disambiguates pointer/optional cases and is closer to master.
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163180665)
Was doing that in earlier pushes of the PR but changed back to `!= nullopt` since it disambiguates pointer/optional cases and is closer to master.
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163967956)
Broke out removal of `NodeRef` & `MakeNodeRef()` into its own commit: c4d30115f5cefe3945ac8b69c021a551eb2b6a72
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163967956)
Broke out removal of `NodeRef` & `MakeNodeRef()` into its own commit: c4d30115f5cefe3945ac8b69c021a551eb2b6a72
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2164001056)
Agree I went overboard in those 3 cases, :+1: reverted.
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2164001056)
Agree I went overboard in those 3 cases, :+1: reverted.
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163882445)
Good find, but would rather leave that for another PR. This one has ballooned enough IMO just from wanting to remove `unique_ptr`.
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2163882445)
Good find, but would rather leave that for another PR. This one has ballooned enough IMO just from wanting to remove `unique_ptr`.
💬 luke-jr commented on pull request "checkqueue: set MAX_SCRIPTCHECK_THREADS to nCores - 1":
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-3001622066)
Maybe this is something best solved by runtime benchmarking? (Or perhaps just at startup, since early blocks may not give us the parallelization we need for it)
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-3001622066)
Maybe this is something best solved by runtime benchmarking? (Or perhaps just at startup, since early blocks may not give us the parallelization we need for it)
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3001627814)
Does it ever make sense to have multiple announcements for the same wtxid to be `m_reconsider`? I'm wondering if `AddChildrenToWorkSet` should skip wtxids for which a reconsider-announcement already exists.
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3001627814)
Does it ever make sense to have multiple announcements for the same wtxid to be `m_reconsider`? I'm wondering if `AddChildrenToWorkSet` should skip wtxids for which a reconsider-announcement already exists.
💬 luke-jr commented on pull request "docs: adds correct updated documentation links":
(https://github.com/bitcoin/bitcoin/pull/32699#discussion_r2164763920)
```suggestion
std::string stable_version = strprintf("v%d.0", CLIENT_VERSION_MAJOR + 1);
```
(https://github.com/bitcoin/bitcoin/pull/32699#discussion_r2164763920)
```suggestion
std::string stable_version = strprintf("v%d.0", CLIENT_VERSION_MAJOR + 1);
```
💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2164783505)
Yes, that is a known and expected limitation. The purpose is to have cheap checks here as everything should already be checked by the parser. There is no way for these specific `PubkeyProvider`s to be created outside of this file anyways.
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2164783505)
Yes, that is a known and expected limitation. The purpose is to have cheap checks here as everything should already be checked by the parser. There is no way for these specific `PubkeyProvider`s to be created outside of this file anyways.
📝 hebasto opened a pull request: "cmake: Use `HINTS` instead of `PATHS` in `find_*` commands"
(https://github.com/bitcoin/bitcoin/pull/32805)
According to the CMake documentation, `HINTS` "should be paths computed by system introspection, such as a hint provided by the location of another item already found", which is precisely the case in the `FindQRencode` module.
Entries in `HINTS` are searched before those in `PATHS`. On macOS, Homebrew’s `libqrencode` will therefore be located at its real path rather than via the symlink in the default prefix.
A backport to 29.x is required for https://github.com/bitcoin/bitcoin/pull/32804,
...
(https://github.com/bitcoin/bitcoin/pull/32805)
According to the CMake documentation, `HINTS` "should be paths computed by system introspection, such as a hint provided by the location of another item already found", which is precisely the case in the `FindQRencode` module.
Entries in `HINTS` are searched before those in `PATHS`. On macOS, Homebrew’s `libqrencode` will therefore be located at its real path rather than via the symlink in the default prefix.
A backport to 29.x is required for https://github.com/bitcoin/bitcoin/pull/32804,
...
💬 hebasto commented on pull request "Fix build on macOS when `qt@6` is installed":
(https://github.com/bitcoin/bitcoin/pull/32804#issuecomment-3001718898)
Please review https://github.com/bitcoin/bitcoin/pull/32805 first.
(https://github.com/bitcoin/bitcoin/pull/32804#issuecomment-3001718898)
Please review https://github.com/bitcoin/bitcoin/pull/32805 first.