💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1419126483)
> > What you are suggesting is ok for a post-mortem comment.
>
> I think you're misunderstanding my point. I am saying in the context of this PR (which is a bug fix), I don't think the log line you are adding here adds value and that a better thing to add in this PR would be a check that BnB is never returning change, which either errors or logs a warning.

I'm following the discussion. The starting point was "the logging line is not useful". So, I presented the arguments for which this lin
...

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

💬 fanquake commented on issue "Build error on Ubuntu 22.04.3 LTS":
(https://github.com/bitcoin/bitcoin/issues/29017#issuecomment-1845540998)
@centaur1 does that mean this is solved and issue should be closed?

✅ centaur1 closed an issue: "Build error on Ubuntu 22.04.3 LTS"
(https://github.com/bitcoin/bitcoin/issues/29017)

💬 maflcko commented on pull request "build: Require C++20 compiler":
(https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1845542104)
The benchmarks on https://corecheck.dev/bitcoin/bitcoin/pulls/28349 are grey ("normal"/expected)

💬 centaur1 commented on issue "Build error on Ubuntu 22.04.3 LTS":
(https://github.com/bitcoin/bitcoin/issues/29017#issuecomment-1845542435)
Yes. I thought I hit "comment and close" but didn't.

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