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

💬 fjahr commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1495983754)
CI failed on this line: https://cirrus-ci.com/task/5549867297144832?logs=ci#L2880

📝 paplorinc opened a pull request: "benchmark/speedup: optimize TryParseHex calls considerably"
(https://github.com/bitcoin/bitcoin/pull/29458)