💬 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.
💬 ryanofsky commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2858716091)
How worried are we about file corruption here? I thought the main reason we use leveldb and sqlite databases in places like this where we don't need indexing is that they support atomic updates, so you can pull the power cord any time and next time you reboot you will will see some consistent view of the data, even if it's not the latest data. I didn't look too closely at the implementation here but it seems like it is updating data in the files in place, so if writes are interrupted, data could
...
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2858716091)
How worried are we about file corruption here? I thought the main reason we use leveldb and sqlite databases in places like this where we don't need indexing is that they support atomic updates, so you can pull the power cord any time and next time you reboot you will will see some consistent view of the data, even if it's not the latest data. I didn't look too closely at the implementation here but it seems like it is updating data in the files in place, so if writes are interrupted, data could
...