💬 hebasto commented on issue "The `streams_tests/xor_file` test fails on Windows":
(https://github.com/bitcoin/bitcoin/issues/29014#issuecomment-1845544516)
> So is this going to be some difference between the CI binaries & Guix Windows binaries?

Cross-compiled binaries fail, not only Guix.

💬 fanquake commented on issue "The `streams_tests/xor_file` test fails on Windows":
(https://github.com/bitcoin/bitcoin/issues/29014#issuecomment-1845548049)
> Cross-compiled binaries fail,

Ok, but they don't fail in our CI. So I guess this is some difference between Wine and actual Windows? Needs more info if we are going to be able to debug anything.

💬 maflcko commented on issue "The `streams_tests/xor_file` test fails on Windows":
(https://github.com/bitcoin/bitcoin/issues/29014#issuecomment-1845550256)
> Maybe your Windows doesn't understand the "wbx" flags? (C++17, https://en.cppreference.com/w/cpp/io/c/fopen)

So to clarify, can you re-try by removing the `x`? (I don't have Windows, so I can't try)

💬 stickies-v commented on pull request "mempool: Don't sort in entryAll":
(https://github.com/bitcoin/bitcoin/pull/29019#issuecomment-1845551334)
That is a good point, I'd missed that this just reverts back to the behaviour pre-`entryAll()`. It seems that when iterating over a `boost::multi_index_container` without specifying an index, we default to the insertion order, so i think that would be sufficient guarantees that the `CWallet::transactionAddedToMempool` calls happen in a safe order too.

💬 fanquake commented on issue "The `streams_tests/xor_file` test fails on Windows":
(https://github.com/bitcoin/bitcoin/issues/29014#issuecomment-1845552718)
According to https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fopen-wfopen?view=msvc-170 `x` should be supported, but maybe it is broken.

💬 sdaftuar commented on pull request "refactor: Replace sets of txiter with CTxMemPoolEntryRefs":
(https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1845558921)
> However, using the iterator type in the setEntries set provides little benefit in practice as applied currently. Its purpose, ownership, and safety semantics often remain ambiguous, and it is hardly used for actually iterating through the data structures. So replace it where possible with CTxMemPoolEntryRefs.

I was thinking about this issue of ownership and safety semantics some more -- it seems like that most of the issues around iterators exist with CTxMemPoolEntryRefs as well, which is t
...

💬 fanquake commented on pull request "wallet: No BDB creation, unless -deprecatedrpc=create_bdb":
(https://github.com/bitcoin/bitcoin/pull/28597#issuecomment-1845562237)
Release note being added in #29023.

💬 josibake commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1419154526)
Right, but this is my concern: a bug is found and fixed, and then a log is added that _might_ have helped detect the already fixed bug (in this case, I'm still not convinced this log as written would have helped much). This pattern tends toward log bloat over time.

What I have been suggesting from the beginning is that we keep this PR focused on the bug fix and address logging in a separate PR where we can take a more holistic look. I do think logging the input parameters for coin selection i
...

💬 fanquake commented on pull request "doc: add historical release notes for 26.0":
(https://github.com/bitcoin/bitcoin/pull/29023#issuecomment-1845570160)
Thanks @willcl-ark & @darosior. I've incorporated those changes in an additional commit, which can also be backported to 26.x in #29011.

💬 hebasto commented on issue "The `streams_tests/xor_file` test fails on Windows":
(https://github.com/bitcoin/bitcoin/issues/29014#issuecomment-1845575880)
> > Maybe your Windows doesn't understand the "wbx" flags? (C++17, [en.cppreference.com/w/cpp/io/c/fopen](https://en.cppreference.com/w/cpp/io/c/fopen))
>
> So to clarify, can you re-try by removing the `x`? (I don't have Windows, so I can't try)

With the diff as follows:
```diff
--- a/src/test/streams_tests.cpp
+++ b/src/test/streams_tests.cpp
@@ -29,7 +29,7 @@ BOOST_AUTO_TEST_CASE(xor_file)
BOOST_CHECK_EXCEPTION(xor_file.ignore(1), std::ios_base::failure, HasReason{"AutoFil
...

💬 pablomartin4btc commented on pull request "Fix: Disable both "Mask Values" and "Transaction View" if no wallet is selected":
(https://github.com/bitcoin-core/gui/pull/780#issuecomment-1845584174)
> I'm not sure if this is the right approach: user might want to enable `Mask values` before opening his wallet?

Interesting point of view, I agree with it... Not sure if other wallet apps do it but ok, I'll remove that "fix" and perhaps other reviewers give their opinions on it. Thanks for the review!

💬 willcl-ark commented on pull request "doc: add historical release notes for 26.0":
(https://github.com/bitcoin/bitcoin/pull/29023#issuecomment-1845588929)
ACK ca5937553b4b4dde53995d0b66e30150401023eb

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1845593963)
> The only thing I want to clarify is the downgrade + encrypt situation. So now the wallet loads and not "corrupted" but if the user would generate new descriptors (as proposed in future PRs) the wallet will use the pre-encryption HD key. The scenario seems pretty rare, but still slightly concerning as the pre-encryption HD key could've been compromised.

We have the `version` which stores the version of the last client which opened the wallet. Maybe we should use that to also determine whethe
...

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1419184599)
I don't think this explainer is relevant in this section, nor really relevant at all. The wallet's hdkey does not have to be in any active descriptor - the user could have imported new active descriptors with different keys and the hdkey should not be changed.

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1419188377)
Activeness of descriptors does not necessarily correlate to the hdkey. The user could have imported new active descriptors with different keys and the hdkey would not change.

Also warnings in wallet loading don't really go anywhere. They show up in the log, but people aren't going to be looking at their log file unless the wallet isn't loading.

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1419193242)
`LoadActiveHDPubKey` already does this.

🚀 fanquake merged a pull request: "refactor: Remove unused and fragile string interface from arith_uint256"
(https://github.com/bitcoin/bitcoin/pull/28924)

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1419197086)
We always generate new descriptors for migrated wallets, regardless of a preexisting HD seed. I don't see why setting this flag would be contradictory with that.

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1419201996)
Done

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1419202326)
Taken