💬 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.

💬 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
...

💬 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
...

💬 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

💬 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.

🤔 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
```

💬 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.

💬 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
...

💬 fanquake commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077721654)
I'm not confident anyone is going to followup with this, and don't see the rush to merge this PR. The fact that this code is already being used, doesn't warrent adding more of it.

🚀 fanquake merged a pull request: "Remove the legacy wallet and BDB dependency"
(https://github.com/bitcoin/bitcoin/pull/28710)

✅ fanquake closed an issue: "Proposed Timeline for Legacy Wallet and BDB removal"
(https://github.com/bitcoin/bitcoin/issues/20160)

💬 fanquake commented on issue "Proposed Timeline for Legacy Wallet and BDB removal":
(https://github.com/bitcoin/bitcoin/issues/20160#issuecomment-2858784044)
Closing, given #28710 is merged.

💬 theStack commented on pull request "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)":
(https://github.com/bitcoin/bitcoin/pull/32305#discussion_r2077753175)
Good point, I agree that the `all` predicate makes much more sense than `any` here, not sure what motivated me to use the latter back then. Fixed.

💬 theStack commented on pull request "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)":
(https://github.com/bitcoin/bitcoin/pull/32305#discussion_r2077754605)
Good idea, done.

💬 theStack commented on pull request "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)":
(https://github.com/bitcoin/bitcoin/pull/32305#discussion_r2077754899)
Leaving that one for a follow-up.

💬 theStack commented on pull request "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)":
(https://github.com/bitcoin/bitcoin/pull/32305#discussion_r2077754185)
Right, strictly speaking they are not needed. I added them because we seem to do this also in many other functional tests, and I think it's nice to differentiate between "attribute is not available" and "attribute doesn't match the expected value" more explicitly by failing in different lines. Happy to drop them though if others feel strongly.

💬 theStack commented on pull request "test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373)":
(https://github.com/bitcoin/bitcoin/pull/32305#issuecomment-2858796140)
Thanks for the review @rkrux, I rebased on master and addressed most of your review comments.

💬 ismaelsadeeq commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077754724)
In "doc: Mention bitcoin wrapper executable in documentation" b0989e3095f1964fdb523cdd4b44f0b198417014

I think it would be better to provide a single usage instruction for users. Otherwise, having separate instructions for both `bin/bitcoin` and `bin/bitcoind` in all places could be confusing.

Instead, I suggest we standardize on `bin/bitcoin` in all places, while still supporting the previous one for backward compatibility.

The release notes have clearly indicate this change to users
...

🤔 ismaelsadeeq reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2821963515)
Concept ACK

💬 maflcko commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2077794853)
nit: "should recorded" looks like a typo

should recorded → should record