💬 instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2077537805)
done
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2077537805)
done
💬 instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2077538094)
done
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2077538094)
done
💬 instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2077538389)
done
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2077538389)
done
💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2858452233)
@Sjors
> ACK [8b73630](https://github.com/bitcoin/bitcoin/commit/8b73630c6fe64158bf123da5e22bd0a71ab305c9).
Thank you for your review!
Could you please double-check the commit hash in your comment?
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2858452233)
@Sjors
> ACK [8b73630](https://github.com/bitcoin/bitcoin/commit/8b73630c6fe64158bf123da5e22bd0a71ab305c9).
Thank you for your review!
Could you please double-check the commit hash in your comment?
💬 fanquake commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077545934)
> What alternatives are you suggesting for configuring the Developer Command Prompt?
Directly running the command that somebody who owns a Windows computer would run, that doesn't require installing NPM, and then executing somebody else's JavaScript.
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077545934)
> What alternatives are you suggesting for configuring the Developer Command Prompt?
Directly running the command that somebody who owns a Windows computer would run, that doesn't require installing NPM, and then executing somebody else's JavaScript.
💬 Sjors commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2858457623)
@hebasto fixed, that was from a different PR, but I checked that I used the right commit for my Windows guix build.
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2858457623)
@hebasto fixed, that was from a different PR, but I checked that I used the right commit for my Windows guix build.
💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2858461589)
Friendly ping @achow101 @theStack @luke-jr @sipsorcery :)
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2858461589)
Friendly ping @achow101 @theStack @luke-jr @sipsorcery :)
💬 hebasto commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077561339)
Since this action isn’t new to our codebase, could we continue this discussion elsewhere—in another issue or pull request—while leaving this PR as it stands?
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077561339)
Since this action isn’t new to our codebase, could we continue this discussion elsewhere—in another issue or pull request—while leaving this PR as it stands?
💬 Sjors commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2858496851)
Have you considered simply having one block per file? Typical blk files are 130 MB so for "modern" blocks it would 50x the number of files. But is that actually a problem? It's a lot simpler if we can just have `$HASH.dat`, maybe grouped in a directory per 10k blocks.
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2858496851)
Have you considered simply having one block per file? Typical blk files are 130 MB so for "modern" blocks it would 50x the number of files. But is that actually a problem? It's a lot simpler if we can just have `$HASH.dat`, maybe grouped in a directory per 10k blocks.
💬 maflcko commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2077573425)
unrelated, but it looks like there are typos:
Mow much -> How much [Typo in "Mow much"]
RIPEMD10 -> RIPEMD160 [Incorrect algorithm name]
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2077573425)
unrelated, but it looks like there are typos:
Mow much -> How much [Typo in "Mow much"]
RIPEMD10 -> RIPEMD160 [Incorrect algorithm name]
💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2858505652)
Can we set "30.0" milestone here?
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2858505652)
Can we set "30.0" milestone here?
💬 TheCharlatan commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2858550825)
> Have you considered simply having one block per file? Typical blk files are 130 MB, so for "modern" blocks it would 50x the number of files. But is that actually a problem? It's a lot simpler if we can just have $HASH.dat, maybe grouped in a directory per 10k blocks.
I'm not sure what you are suggesting here. Are you suggesting we create ~900k files and then have some subdivision within those files into 10k groups where each of those has a single `$HASH.dat` with all the headers and file po
...
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2858550825)
> Have you considered simply having one block per file? Typical blk files are 130 MB, so for "modern" blocks it would 50x the number of files. But is that actually a problem? It's a lot simpler if we can just have $HASH.dat, maybe grouped in a directory per 10k blocks.
I'm not sure what you are suggesting here. Are you suggesting we create ~900k files and then have some subdivision within those files into 10k groups where each of those has a single `$HASH.dat` with all the headers and file po
...
🤔 rkrux reviewed a pull request: "lint: Remove string exclusion from locale check"
(https://github.com/bitcoin/bitcoin/pull/32434#pullrequestreview-2821711457)
crACK fa24fdcb7f474e6959ae4d8fe9759d365c6c021b
The documentation made me wonder whether the dev is supposed to run this lint manually but I verified that it is a part of the CI as well, just like the other python linters.
(https://github.com/bitcoin/bitcoin/pull/32434#pullrequestreview-2821711457)
crACK fa24fdcb7f474e6959ae4d8fe9759d365c6c021b
The documentation made me wonder whether the dev is supposed to run this lint manually but I verified that it is a part of the CI as well, just like the other python linters.
💬 instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2858558881)
> I believe this "DEPRECATED" definition is up for debate. Marking a toggle that has worked for my mempool policy and will work for my mempool policy in the future as "DEPRECATED" is FALSE & MISLEADING. It still works.
Deprecation isn't a value judgment but a statement that it will be removed in a future version.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2858558881)
> I believe this "DEPRECATED" definition is up for debate. Marking a toggle that has worked for my mempool policy and will work for my mempool policy in the future as "DEPRECATED" is FALSE & MISLEADING. It still works.
Deprecation isn't a value judgment but a statement that it will be removed in a future version.
💬 pablomartin4btc commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#issuecomment-2858591705)
>> Otherwise one could set 2 diff proxies for Tor(?).
> I think that is ok, assuming the expectation is that later options override earlier ones. For example, without this PR, `-proxy=x -onion=y` is supposed to set the Tor proxy to `y`, overriding the earlier `x`. Updated `tor.md` to mention that.
Ok.
> > So, shouldn't `-proxy=0=cjdns` be the default if `-cjdnsreachable` when `-proxy=addr:port`?
>
> Could be, but I do not like these "automatically set `x` only if `y` and `z` are tru
...
(https://github.com/bitcoin/bitcoin/pull/32425#issuecomment-2858591705)
>> Otherwise one could set 2 diff proxies for Tor(?).
> I think that is ok, assuming the expectation is that later options override earlier ones. For example, without this PR, `-proxy=x -onion=y` is supposed to set the Tor proxy to `y`, overriding the earlier `x`. Updated `tor.md` to mention that.
Ok.
> > So, shouldn't `-proxy=0=cjdns` be the default if `-cjdnsreachable` when `-proxy=addr:port`?
>
> Could be, but I do not like these "automatically set `x` only if `y` and `z` are tru
...
💬 Sjors commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2858637715)
> Are you suggesting we create ~900k files
Yes. If we're going to redesign block storage, it's seems good to wonder why can't let the file system handle things.
> with all the headers and file pointers for the blocks in that division
One block per file. We can still have a single file for the block index, which would contain the header (not just the hash) and validation state. Block files themselves would be in a predictable location, so we wouldn't need an index for that.
> My impre
...
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2858637715)
> Are you suggesting we create ~900k files
Yes. If we're going to redesign block storage, it's seems good to wonder why can't let the file system handle things.
> with all the headers and file pointers for the blocks in that division
One block per file. We can still have a single file for the block index, which would contain the header (not just the hash) and validation state. Block files themselves would be in a predictable location, so we wouldn't need an index for that.
> My impre
...
💬 hebasto commented on issue "Depends toolchain doesn't contain enough info to build from depends on a fresh NixOS install":
(https://github.com/bitcoin/bitcoin/issues/32428#issuecomment-2858643986)
Kind of related: https://discourse.nixos.org/t/nixpkgs-commit-that-mysteriously-broke-my-cmake-environment/52152
(https://github.com/bitcoin/bitcoin/issues/32428#issuecomment-2858643986)
Kind of related: https://discourse.nixos.org/t/nixpkgs-commit-that-mysteriously-broke-my-cmake-environment/52152
💬 rkrux commented on pull request "Wallet: Fix Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in AddWalletDescriptor":
(https://github.com/bitcoin/bitcoin/pull/32344#discussion_r2077674278)
Nit: Although the test name is present but it might be helpful to mention why the loop is there, i.e., to update the descriptor by calling `AddWalletDescriptor` multiple times with the same descriptor.
(https://github.com/bitcoin/bitcoin/pull/32344#discussion_r2077674278)
Nit: Although the test name is present but it might be helpful to mention why the loop is there, i.e., to update the descriptor by calling `AddWalletDescriptor` multiple times with the same descriptor.
🤔 rkrux reviewed a pull request: "Wallet: Fix Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in AddWalletDescriptor"
(https://github.com/bitcoin/bitcoin/pull/32344#pullrequestreview-2821828845)
Concept ACK fa0993462b3bc8db40c1a6ea2e7f5fd4a22353a6
I notice while running the unit tests that it warns about a lack of assertions. Maybe it's expecting a boost check?
```bash
wallet/test/wallet_tests.cpp:76: Entering test case "update_non_range_descriptor"
Test case wallet_tests/update_non_range_descriptor did not check any assertions
wallet/test/wallet_tests.cpp:76: Leaving test case "update_non_range_descriptor"; testing time: 38813us
```
(https://github.com/bitcoin/bitcoin/pull/32344#pullrequestreview-2821828845)
Concept ACK fa0993462b3bc8db40c1a6ea2e7f5fd4a22353a6
I notice while running the unit tests that it warns about a lack of assertions. Maybe it's expecting a boost check?
```bash
wallet/test/wallet_tests.cpp:76: Entering test case "update_non_range_descriptor"
Test case wallet_tests/update_non_range_descriptor did not check any assertions
wallet/test/wallet_tests.cpp:76: Leaving test case "update_non_range_descriptor"; testing time: 38813us
```
💬 rkrux commented on pull request "Wallet: Fix Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in AddWalletDescriptor":
(https://github.com/bitcoin/bitcoin/pull/32344#discussion_r2077685997)
Nit: Tying these 2 if blocks with an `else if` would be nice, although not necessary because the first if returns.
(https://github.com/bitcoin/bitcoin/pull/32344#discussion_r2077685997)
Nit: Tying these 2 if blocks with an `else if` would be nice, although not necessary because the first if returns.