💬 LarryRuane commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1115061429)
I like the way you're calculating the number of blocks needed rather than hard-coding the number; I verified that this value (when I run the test) is 240, which matches what @jonatack suggested.

💬 Mijay36089 commented on issue "Hidden fee (about 15% of sum) while send":
(https://github.com/bitcoin/bitcoin/issues/27120#issuecomment-1440964953)
Where did you send it to what wallet ?

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1115299326)
What you described means that the peer clearly has the transaction, thus it can safely be removed from the corresponding set ("try" means it's ok if the tx is not there). What's the problem with that?

💬 S3RK commented on pull request "wallet: skip R-value signature grinding for external signers":
(https://github.com/bitcoin/bitcoin/pull/26032#issuecomment-1441341890)
> The downside of this is approach is that I had to sprinkle `const bool can_grind_r = !wallet->IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER);` in several different places. @furszy's commit does it in three places. This is fine right now, but at some point we might have some external signers that do support grinding. We'd then have to update the check everywhere.

What if we encapsulate the logic in `Wallet::CanGrindR()`?

The ability to grind signatures is a characteristic of a wallet IIUC
...

💬 willcl-ark commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1115361129)
Closing as I think this is resolved

💬 willcl-ark commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1115385741)
Updated in 56e370fbb9413260723c598048392219b1055ad0

💬 willcl-ark commented on pull request "docs: add ramdisk guide for running tests on OSX":
(https://github.com/bitcoin/bitcoin/pull/27124#issuecomment-1441415357)
ACK 2f84ad7b9e62dd710940c2f265b65973b94864d7

💬 fanquake commented on pull request "Fix various libbitcoinkernel DLL build problems":
(https://github.com/bitcoin/bitcoin/pull/27146#issuecomment-1441455198)
Concept ACK

💬 fanquake commented on pull request "build: Build `libbitcoinconsensus` from its own convenience library":
(https://github.com/bitcoin/bitcoin/pull/24994#issuecomment-1441457340)
Concept NACK. In general, I don't like this approach of refactoring code, and adding libraries, to workaround issues with build tooling, especially when its a problem that only exists for a single platform (let alone the DEBUG build, and ). I think this should just be closed in light of #27146 and the migration towards CMake.

🤔 TheCharlatan requested changes to a pull request: "Fix various libbitcoinkernel DLL build problems"
(https://github.com/bitcoin/bitcoin/pull/27146)

💬 TheCharlatan commented on pull request "Fix various libbitcoinkernel DLL build problems":
(https://github.com/bitcoin/bitcoin/pull/27146#discussion_r1115408165)
Can you comment on what this `-static` flag is here for?

💬 TheCharlatan commented on pull request "Fix various libbitcoinkernel DLL build problems":
(https://github.com/bitcoin/bitcoin/pull/27146#discussion_r1115441530)
Is there a way to check this? I tried running:
```
make distclean
./autogen.sh
CONFIG_SITE=~/bitcoin/depends/x86_64-w64-mingw32/share/config.site ./configure \
--with-experimental-kernel-lib \
--enable-experimental-util-chainstate \
--with-experimental \
--prefix ~/bitcoin/install_dir \
--disable-static
make install
```
But I'm getting a bunch of linker errors:
```
/usr/bin/x86_64-w64-mingw32-ld: /usr/lib/gcc/x86_64-w64-mingw32/10-posix/libssp.a(ssp.o):(.text+0xe0): multiple defi
...

💬 fanquake commented on pull request "clang-tidy: Add more `performance-*` checks and related fixes":
(https://github.com/bitcoin/bitcoin/pull/26642#issuecomment-1441472941)
> Maybe keep this PR diff CI-driven? (I mean, the CI [task](https://cirrus-ci.com/task/4897437224009728) passes now)

If there are additional "fixes" being pointed out, which are correct, and based on the same rules that we are trying to apply, I don't really see the point of leaving those changes out, just to have to fix them later (upgrading our tooling here is another point).

I've been a little bit concerned that a lot of these clang-tidy changes have just been blindly following the tool
...

🚀 fanquake merged a pull request: "docs: add ramdisk guide for running tests on OSX"
(https://github.com/bitcoin/bitcoin/pull/27124)

✅ hebasto closed a pull request: "build: Build `libbitcoinconsensus` from its own convenience library"
(https://github.com/bitcoin/bitcoin/pull/24994)

💬 Ely1969 commented on pull request "build: Build `libbitcoinconsensus` from its own convenience library":
(https://github.com/bitcoin/bitcoin/pull/24994#issuecomment-1441549203)
Very nicr

💬 Sjors commented on pull request "wallet: skip R-value signature grinding for external signers":
(https://github.com/bitcoin/bitcoin/pull/26032#issuecomment-1441567529)
@S3RK done!

💬 vasild commented on pull request "doc: clarify that LOCK() does AssertLockNotHeld() internally":
(https://github.com/bitcoin/bitcoin/pull/27116#discussion_r1115561797)
Yes, if `LOCK()` is moved, then it would have to be reviewed either way.

💬 Ely1969 commented on pull request "doc: clarify that LOCK() does AssertLockNotHeld() internally":
(https://github.com/bitcoin/bitcoin/pull/27116#discussion_r1115568025)
Hoe to merge

💬 naumenkogs commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1441727779)
Concept ACK