💬 achow101 commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2122204265)
In ca5781e23a8f299ff4f143d2355218f551e65944 "config: allow setting -proxy per network"
nit: I think using `Split()` would make this easier to read
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2122204265)
In ca5781e23a8f299ff4f143d2355218f551e65944 "config: allow setting -proxy per network"
nit: I think using `Split()` would make this easier to read
💬 achow101 commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2122190719)
In ca5781e23a8f299ff4f143d2355218f551e65944 "config: allow setting -proxy per network"
nit: `Split()`? (from `src/util/string.h`)
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2122190719)
In ca5781e23a8f299ff4f143d2355218f551e65944 "config: allow setting -proxy per network"
nit: `Split()`? (from `src/util/string.h`)
💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2121983471)
Yes, but when breaking apart a struct and two of the new variable names match struct field names with identical types, it would be nice to have some kind of guard rail.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2121983471)
Yes, but when breaking apart a struct and two of the new variable names match struct field names with identical types, it would be nice to have some kind of guard rail.
💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2122154034)
Just realized a different possible approach: passing `HEADER_COMMITMENT_PERIOD` and `REDOWNLOAD_BUFFER_SIZE` values into the `HeadersSyncState`-ctor. Right now that aspect of the class is hardcoded for mainnet, despite the ctor accepting `Consensus::Params`. Having these values be sent in would enable tests to send in different (smaller) variables for regtest, while still testing behavior around the threshold.
In fact, it would fit nicely with other values updated during the release process t
...
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2122154034)
Just realized a different possible approach: passing `HEADER_COMMITMENT_PERIOD` and `REDOWNLOAD_BUFFER_SIZE` values into the `HeadersSyncState`-ctor. Right now that aspect of the class is hardcoded for mainnet, despite the ctor accepting `Consensus::Params`. Having these values be sent in would enable tests to send in different (smaller) variables for regtest, while still testing behavior around the threshold.
In fact, it would fit nicely with other values updated during the release process t
...
💬 enirox001 commented on issue "wallet: render BIP388 wallet policies":
(https://github.com/bitcoin/bitcoin/issues/32659#issuecomment-2932640309)
Would be open to explore how we can add those `policy` and `keys` fields to the `getdescriptorinfo` and `listdescriptors` RPCs for BIP 388 wallet policies. I'll be diving into the BIP and the existing code in the next few days, and definitely checking out that older HWI PR for insights.
(https://github.com/bitcoin/bitcoin/issues/32659#issuecomment-2932640309)
Would be open to explore how we can add those `policy` and `keys` fields to the `getdescriptorinfo` and `listdescriptors` RPCs for BIP 388 wallet policies. I'll be diving into the BIP and the existing code in the next few days, and definitely checking out that older HWI PR for insights.
🤔 achow101 reviewed a pull request: "Feature: Use different datadirs for different signets"
(https://github.com/bitcoin/bitcoin/pull/29838#pullrequestreview-2890052727)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29838#pullrequestreview-2890052727)
Concept ACK
💬 achow101 commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r2122232664)
In 69139049194e55a7fd99b7125b9afccb8585922d "test: Add test for diffrent signet data directories"
This test is not sufficient. All it checks is that the test framework has computed the expected datadir. It does not check that the directory even exists, or that the node is using that as the data directory.
I think this will need to read the debug log to see that it has the expected `Using data directory` line, but it would also probably be better if the node itself could report the current
...
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r2122232664)
In 69139049194e55a7fd99b7125b9afccb8585922d "test: Add test for diffrent signet data directories"
This test is not sufficient. All it checks is that the test framework has computed the expected datadir. It does not check that the directory even exists, or that the node is using that as the data directory.
I think this will need to read the debug log to see that it has the expected `Using data directory` line, but it would also probably be better if the node itself could report the current
...
📝 hebasto opened a pull request: "refactor: Drop unused `#include <boost/operators.hpp>`"
(https://github.com/bitcoin/bitcoin/pull/32668)
From https://api.cirrus-ci.com/v1/task/5082537556443136/logs/ci.log:
```
[10:04:20.418] /ci_container_base/src/node/mini_miner.cpp should remove these lines:
[10:04:20.418] - #include <boost/operators.hpp> // lines 8-8
```
(https://github.com/bitcoin/bitcoin/pull/32668)
From https://api.cirrus-ci.com/v1/task/5082537556443136/logs/ci.log:
```
[10:04:20.418] /ci_container_base/src/node/mini_miner.cpp should remove these lines:
[10:04:20.418] - #include <boost/operators.hpp> // lines 8-8
```
💬 hebasto commented on pull request "depends: Bump boost to 1.88.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/32665#discussion_r2122243801)
Did I forget to explicitly mention the [`tuple`](https://www.boost.org/doc/libs/latest/libs/tuple/doc/html/tuple_users_guide.html) library?
See: https://github.com/bitcoin/bitcoin/blob/f999c3775c12ef732a1920dd52257a8cd34cdcc8/src/txrequest.cpp#L18
(https://github.com/bitcoin/bitcoin/pull/32665#discussion_r2122243801)
Did I forget to explicitly mention the [`tuple`](https://www.boost.org/doc/libs/latest/libs/tuple/doc/html/tuple_users_guide.html) library?
See: https://github.com/bitcoin/bitcoin/blob/f999c3775c12ef732a1920dd52257a8cd34cdcc8/src/txrequest.cpp#L18
💬 achow101 commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#issuecomment-2932692510)
ACK 6ee32aaaca4a46d42594bc16479868c76cc67fd8
(https://github.com/bitcoin/bitcoin/pull/29032#issuecomment-2932692510)
ACK 6ee32aaaca4a46d42594bc16479868c76cc67fd8
💬 achow101 commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2932718871)
> I think the main conceptual question is what should happen in the case of a stalling situation where the addnode peer is actually slow and causing congestion?
I'd like to see this addressed before moving forward here.
> Maybe one could add yet another option to make manually specified nodes be treated with extra leniency?
It could be a `whitebind`/`whitelist` option.
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2932718871)
> I think the main conceptual question is what should happen in the case of a stalling situation where the addnode peer is actually slow and causing congestion?
I'd like to see this addressed before moving forward here.
> Maybe one could add yet another option to make manually specified nodes be treated with extra leniency?
It could be a `whitebind`/`whitelist` option.
🚀 achow101 merged a pull request: "signet: omit commitment for some trivial challenges"
(https://github.com/bitcoin/bitcoin/pull/29032)
(https://github.com/bitcoin/bitcoin/pull/29032)
💬 hebasto commented on pull request "depends: Bump boost to 1.88.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/32665#issuecomment-2932742341)
> Would we not want to bump the boost version in [AddBoostIfNeeded.cmake](https://github.com/bitcoin/bitcoin/blob/f999c3775c12ef732a1920dd52257a8cd34cdcc8/cmake/module/AddBoostIfNeeded.cmake#L29) too?
The minimum supported Boost version is a separate concern from the Boost version used in depends.
It deserves its own PR with analysis of available packages on supported systems.
(https://github.com/bitcoin/bitcoin/pull/32665#issuecomment-2932742341)
> Would we not want to bump the boost version in [AddBoostIfNeeded.cmake](https://github.com/bitcoin/bitcoin/blob/f999c3775c12ef732a1920dd52257a8cd34cdcc8/cmake/module/AddBoostIfNeeded.cmake#L29) too?
The minimum supported Boost version is a separate concern from the Boost version used in depends.
It deserves its own PR with analysis of available packages on supported systems.
💬 hebasto commented on pull request "build: Find Boost in config mode":
(https://github.com/bitcoin/bitcoin/pull/32667#discussion_r2122285965)
IIRC, this comment was about "proper CMake files" shipped with source archives.
For example, Ubuntu 22.04 ships Boost 1.74 with [package configuration files](https://packages.ubuntu.com/jammy/amd64/libboost1.74-dev/filelist):
```
/usr/lib/x86_64-linux-gnu/cmake/Boost-1.74.0/BoostConfig.cmake
/usr/lib/x86_64-linux-gnu/cmake/Boost-1.74.0/BoostConfigVersion.cmake
/usr/lib/x86_64-linux-gnu/cmake/BoostDetectToolset-1.74.0.cmake
/usr/lib/x86_64-linux-gnu/cmake/boost_headers-1.74.0/boost_header
...
(https://github.com/bitcoin/bitcoin/pull/32667#discussion_r2122285965)
IIRC, this comment was about "proper CMake files" shipped with source archives.
For example, Ubuntu 22.04 ships Boost 1.74 with [package configuration files](https://packages.ubuntu.com/jammy/amd64/libboost1.74-dev/filelist):
```
/usr/lib/x86_64-linux-gnu/cmake/Boost-1.74.0/BoostConfig.cmake
/usr/lib/x86_64-linux-gnu/cmake/Boost-1.74.0/BoostConfigVersion.cmake
/usr/lib/x86_64-linux-gnu/cmake/BoostDetectToolset-1.74.0.cmake
/usr/lib/x86_64-linux-gnu/cmake/boost_headers-1.74.0/boost_header
...
💬 ajtowns commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2932802336)
reACK a189d636184b1c28fa4a325b56c1fab8f44527b1
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2932802336)
reACK a189d636184b1c28fa4a325b56c1fab8f44527b1
💬 achow101 commented on issue "Unusual "Wallet requires newer version" Error with wallet.dat on macOS, Even with Older Client":
(https://github.com/bitcoin/bitcoin/issues/32548#issuecomment-2932808428)
@l3x3l I started writing an alternative to pywallet that should be able to achieve what you want to do: https://github.com/achow101/wallet-manipulator
You can install it by using `pip install .` and then the command `wallet-manipulator <path to file> export --importable privkeys` should give you the private keys in the wallet that can be imported into a new wallet with `importdescriptors`.
It would also be useful to know what the version of the wallet is. You can get that with `wallet-manipula
...
(https://github.com/bitcoin/bitcoin/issues/32548#issuecomment-2932808428)
@l3x3l I started writing an alternative to pywallet that should be able to achieve what you want to do: https://github.com/achow101/wallet-manipulator
You can install it by using `pip install .` and then the command `wallet-manipulator <path to file> export --importable privkeys` should give you the private keys in the wallet that can be imported into a new wallet with `importdescriptors`.
It would also be useful to know what the version of the wallet is. You can get that with `wallet-manipula
...
💬 theuni commented on pull request "depends: Bump boost to 1.88.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/32665#discussion_r2122308373)
`multi_index` drags in a ton of header-only libs, CMake takes care of installing its dependencies. Is tuple a problem for some reason?
(https://github.com/bitcoin/bitcoin/pull/32665#discussion_r2122308373)
`multi_index` drags in a ton of header-only libs, CMake takes care of installing its dependencies. Is tuple a problem for some reason?
💬 theuni commented on pull request "depends: Bump boost to 1.88.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/32665#discussion_r2122313091)
That include should be perfectly safe to remove, btw. It's included by (at least) `boost/multi_index/sequenced_index.hpp`.
(https://github.com/bitcoin/bitcoin/pull/32665#discussion_r2122313091)
That include should be perfectly safe to remove, btw. It's included by (at least) `boost/multi_index/sequenced_index.hpp`.
💬 l3x3l commented on issue "Unusual "Wallet requires newer version" Error with wallet.dat on macOS, Even with Older Client":
(https://github.com/bitcoin/bitcoin/issues/32548#issuecomment-2932876340)
Wow! Thank you. I’ll start reading your application and follow up with
what happens next.
l3x3l
>--o--<
On Mon, Jun 2, 2025 at 5:01 PM Ava Chow ***@***.***> wrote:
> *achow101* left a comment (bitcoin/bitcoin#32548)
> <https://github.com/bitcoin/bitcoin/issues/32548#issuecomment-2932808428>
>
> @l3x3l <https://github.com/l3x3l> I started writing an alternative to
> pywallet that should be able to achieve what you want to do:
> https://github.com/achow101/wallet-manipulator
>
...
(https://github.com/bitcoin/bitcoin/issues/32548#issuecomment-2932876340)
Wow! Thank you. I’ll start reading your application and follow up with
what happens next.
l3x3l
>--o--<
On Mon, Jun 2, 2025 at 5:01 PM Ava Chow ***@***.***> wrote:
> *achow101* left a comment (bitcoin/bitcoin#32548)
> <https://github.com/bitcoin/bitcoin/issues/32548#issuecomment-2932808428>
>
> @l3x3l <https://github.com/l3x3l> I started writing an alternative to
> pywallet that should be able to achieve what you want to do:
> https://github.com/achow101/wallet-manipulator
>
...
💬 BitcoinMechanic commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2933103177)
reNACK. 100 kilobyte OP RETURNS add absolutely nothing to Bitcoin.
They'd have to actually be happening out of band currently for the concerns about fee estimation/block propagation/mining centralization to actually be relevant here, even then they'd be massively overblown.
There are currently almost no OP RETURNs greater than 80 bytes. All this PR will do is open up another spam-highway along a route that is thankfully not practical with this filter left alone like it should be.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2933103177)
reNACK. 100 kilobyte OP RETURNS add absolutely nothing to Bitcoin.
They'd have to actually be happening out of band currently for the concerns about fee estimation/block propagation/mining centralization to actually be relevant here, even then they'd be massively overblown.
There are currently almost no OP RETURNs greater than 80 bytes. All this PR will do is open up another spam-highway along a route that is thankfully not practical with this filter left alone like it should be.