💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1495827183)
Haven't tried, but I expect this will fix it:

```patch
// Ensure that each UTXO has at least an effective value of 1 sat
+ if (max_output_groups + max_spendable >= MAX_MONEY + group_pos.size()) break;
const CAmount eff_value{fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(1, MAX_MONEY - max_spendable - max_output_groups + group_pos.size())};
```

💬 Sjors commented on pull request "Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1954224796)
tACK b67b6ab3b69d45034b17f4704a1f3cedb07e9af6

Tested on macOS 14.2.1, Windows 11 Home (using cross compiled Guix build) and Ubuntu 23.10.

I didn't test Windows native. It might be worth splitting off the last commit to get this merged faster. I'm assuming it's more important to get rid of boost process than it is to support native Windows builds.

Commit f6a3b5ef643d4993f0ae3faa0c5ff1ae62ad71d8 which switches the external signer code to use cpp-subprocess looks correct.

I perused the
...

💬 Sjors commented on pull request "Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#discussion_r1495576450)
b67b6ab3b69d45034b17f4704a1f3cedb07e9af6: It does not? `(requires Boost::Process)`

💬 fjahr commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1495836330)
I would either expect that the internally set option can not be overridden outside because the developers have decided that this test has to run this setting to keep good coverage no matter what the user thinks or that the test is skipped when the internal option is not compatible with what is provided from outside. Depends a bit on how important v1 test coverage is for us in the short term I think.

💬 sipa commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1495837621)
Here is a patch that merges the two processing loops into one (deduplicating some logic between them), and also makes the tail feerate explicit (even though only `FeeFrac{0, 1}` is used for now).

```patch
diff --git a/src/util/feefrac.cpp b/src/util/feefrac.cpp
index a970adedc30..569f32f4615 100644
--- a/src/util/feefrac.cpp
+++ b/src/util/feefrac.cpp
@@ -22,7 +22,7 @@ void BuildDiagramFromUnsortedChunks(std::vector<FeeFrac>& chunks, std::vector<Fe
}
}

-std::partial_ordering
...

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1495839770)
to make sure I'm not misreading, this is a bump of the default feerate value by 10x? Making sure since I don't see any tests that had to be changed.

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-1954241752)
Rebased after #29441 landed which contains two commits from this PR.

💬 instagibbs commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#discussion_r1495847533)
I'm likely misreading this, but `DEFAULT_MAX_RAW_TX_FEE_RATE` doesn't seem to actually be used anywhere except `PSBTOperationsDialog::broadcastTransaction()`?

👋 maflcko's pull request is ready for review: "lint: Check for missing bitcoin-config.h includes"
(https://github.com/bitcoin/bitcoin/pull/29408)

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1495861406)
taken, thanks

💬 instagibbs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#issuecomment-1954265365)
rebased

💬 fjahr commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#issuecomment-1954281008)
tACK bf5662c678455fb47c402f8520215726ddfe7a94

💬 maflcko commented on pull request "lint: Check for missing bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1954281330)
Added the check to error on redundant (harmless) includes as well, since it was trivial. Should be trivial to re-ACK.

💬 maflcko commented on pull request "lint: Check for missing bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1954286731)
Rebased, for silent merge conflict in `src/bench/wallet_ismine.cpp`. I guess this immediately shows that the check is useful :)

💬 fanquake commented on pull request "serialization: c++20 endian/byteswap/clz modernization":
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1954289542)
@theuni want to rebase this now that #29404 is in?

💬 sipa commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1495884531)
Since this "in case of non-coinbase first transaction, treat existence of 64-byte transaction as malleation" is new behavior, would it make sense to move it to a separate commit at the end of the PR? (A clearer comment would also be helpful, because I briefly thought this was a consensus change before it was pointed out to be it only triggers in case the block would already be invalid).

💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1495894984)
Moving to `bip352.h` sounds good too.

✅ so7ow closed an issue: "When selecting a custom data directory on first launch of Bitcoin Core GUI on Mac, where is that setting stored?"
(https://github.com/bitcoin-core/gui/issues/798)

💬 so7ow commented on issue "When selecting a custom data directory on first launch of Bitcoin Core GUI on Mac, where is that setting stored?":
(https://github.com/bitcoin-core/gui/issues/798#issuecomment-1954310379)
Thank you! Confirmed this works:

```
rm ~/Library/Preferences/org.bitcoin.Bitcoin-Qt.plist
killall -u YourUserName cfprefsd
```

💬 instagibbs commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1495954173)
I also had the same misconception the first time I reviewed.