💬 darosior commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2737618918)
Thanks for testing!

💬 yancyribbens commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2737655586)
> This is not accurate. You could have two UTXOs of different output types that have the same effective value but differing fees. Counterexample:

Yes, I redacted that comment later in the thread as my understand of the problem improved. As I posted last, there's even a test [here](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L427) that tests two UTXOs with the same effective_value yet different fees.

> However, since [CoinGrinder got merged](https://gi
...

💬 yancyribbens commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2737675105)
However, doesn't the waste score account for long_term_fee_rate as well as fee_rate? In other-words, just comparing `fee` amounts and not `long_term_fee_rate` would not be enough to know if comparing two UTXOs have different was scores..

⚠️ luisschwab opened an issue: "Incorrect balance when dealing with coinbase UTXOs that will be mature on _h_ + 1"
(https://github.com/bitcoin/bitcoin/issues/32098)
Bitcoin Core is reporting the wrong balance when dealing with coinbase UTXOs that will become mature on the next block.

Scenario:
On regtest, I mine 101 blocks. Since coinbase maturity height is 100 blocks, I have 1 mature UTXO (50 BTC) at height 101. However, at height 102, another coinbase UTXO (50 BTC) will have matured and will be accepted by the mempool as an input to a transaction. Therefore, my _**spendable**_ balance at height 102 will be 50+50 BTC (2 UTXOs available for selection).

Cu
...

💬 luisschwab commented on issue "Incorrect balance when dealing with coinbase UTXOs that will be mature on `h + 1`":
(https://github.com/bitcoin/bitcoin/issues/32098#issuecomment-2737775525)
Apparently this is a simple fix:

```diff
scr/wallet/wallet.cpp

int CWallet::GetTxBlocksToMaturity(const CWalletTx& wtx) const
{
AssertLockHeld(cs_wallet);

if (!wtx.IsCoinBase()) {
return 0;
}
int chain_depth = GetTxDepthInMainChain(wtx);
assert(chain_depth >= 0); // coinbase tx should not be conflicted
- return std::max(0, (COINBASE_MATURITY+1) - chain_depth);
+ return std::max(0, COINBASE_MATURITY - chain_depth);
}
```

💬 maflcko commented on pull request "test: Fix intermittent issue in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/pull/32092#discussion_r2004125240)
thx, done

💬 maflcko commented on issue "Incorrect balance when dealing with coinbase UTXOs that will be mature on `h + 1`":
(https://github.com/bitcoin/bitcoin/issues/32098#issuecomment-2737798747)
I presume this was last touched in commit bf3a20a6e8cafdf723ef101af078df303ea06fec, which explains that there may be a race condition where the transaction is rejected, because the 100th block has not been relayed yet to the peer when the transaction is sent.

💬 maflcko commented on issue "Failure to run Fuzz tests when running with corpus":
(https://github.com/bitcoin/bitcoin/issues/32089#issuecomment-2737863589)
> libc++abi: terminating due to uncaught exception of type std::__1::ios_base::failure: DataStream::read(): end of data: unspecified iostream_category error

This looks like an upstream packaging bug or asan bug on your platform, given that it passes fine when asan is disabled on your platform.

💬 hebasto commented on issue "[rfc] build: Reject unclean configure?":
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2737914378)
FWIW, at the end of configuration, Qt 6 prints:
```
If reconfiguration fails for some reason, try removing 'CMakeCache.txt' from the build directory
Alternatively, you can add the --fresh flag to your CMake flags.
```

🤔 maflcko reviewed a pull request: "contrib: Make deterministic-coverage error messages more readable"
(https://github.com/bitcoin/bitcoin/pull/32074#pullrequestreview-2700009626)
rebased with a move-only change, and replied to all feedback

💬 maflcko commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2004192240)
I found `Err(...)?` nicer as an early-return statement, but no strong opinion

💬 maflcko commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2004194037)
In Rust shadowing is a feature, but happy to change, no strong opinion.

💬 maflcko commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2004190137)
thx, done

💬 maflcko commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2004196829)
Nice. I think this is sufficient as an approach for a dev-only tool. Happy to push it here, or review your follow-up. Just let me know.

💬 maflcko commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2004186423)
Yeah, the string ends up in `Err` eventually, so it should be type-safe enough for nothing to go wrong. At least I can't see any risk here.

👋 maflcko's pull request is ready for review: "contrib: Make deterministic-coverage error messages more readable"
(https://github.com/bitcoin/bitcoin/pull/32074)

👍 hodlinator approved a pull request: "ci: Test cross-built Windows executables on Windows natively"
(https://github.com/bitcoin/bitcoin/pull/31176#pullrequestreview-2700044703)
re-ACK 25b56fd9b469f8e5d36f0132c3b79a5214e3372a

Changes since [previous ACK](https://github.com/bitcoin/bitcoin/pull/31176#pullrequestreview-2661348106):
- Added commit switching to Bash by default which lets us avoid annoying "&& `" in later commit which was required by PowerShell. Confirmed CI still seems to be executing unit/functional/fuzz tests.
- Added comment explaining why we can't use ctest.

💬 maflcko commented on issue "[rfc] build: Reject unclean configure?":
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2737988575)
> If reconfiguration fails for some reason, try removing 'CMakeCache.txt' from the build directory
> Alternatively, you can add the --fresh flag to your CMake flags.

I don't think I've seen issues where re-configuration fails. So far the linked issues here were about linker errors.

🤔 LarryRuane reviewed a pull request: "improve MallocUsage() accuracy"
(https://github.com/bitcoin/bitcoin/pull/28531#pullrequestreview-2699829126)
@l0rinc - thank you for the excellent review and great suggestions. I especially like making `MallocUsage()` a `constexpr` function.

I'll force-push a commit taking all of them except moving the first (test-only) commit (I left a comment explaining my reasoning).

💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2004163332)
Good point, I am going to back this change out until we see evidence for needing it. I think I "deduced" this based on macOS CI failing while the others were passing. But it's been too long.

Just now, after searching for a while with no luck, I asked X's Grok (I know, we shouldn't trust it too much), and it said LLVM's implementation of `std::unordered_map` that the node includes a next pointer but no previous pointer. You can indeed see that in the source code: https://github.com/llvm/llvm-
...