💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1426489158)
Rewrote the docs in this file, I think it's much more clear now

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1426489285)
Added the inheritance check, but the second half is redundant with when we check the child, no?

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1426489422)
Added, thanks

💬 maflcko commented on pull request "[WIP] C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1855569550)
Oh, I guess clang-15 can use libc++-16, but not libstdc++, according to https://godbolt.org/z/dbKWeE8Te and https://github.com/llvm/llvm-project/commit/babdef27c503c0bbbcc017e9f88affddda90ea4e ?

💬 maflcko commented on pull request "fuzz: Improve fuzzing stability for minisketch harness":
(https://github.com/bitcoin/bitcoin/pull/29064#issuecomment-1855571815)
lgtm. Could squash the simple test fix into one commit?

💬 dergoegge commented on pull request "fuzz: Improve fuzzing stability for minisketch harness":
(https://github.com/bitcoin/bitcoin/pull/29064#issuecomment-1855575173)
Fuzzer now crashes on:

```
$ echo "HwoKCQoKCgoKCgoKCgkKCgoKvQoLCgoKCgkKCgoKvQoLCgoKCgoJCgoKCgpBCgoKCgpBDg==" | base64 --decode > minisketch.crash
$ FUZZ=minisketch ./src/test/fuzz/fuzz minisketch.crash
test/fuzz/minisketch.cpp:73 minisketch_fuzz_target: Assertion `dec.size() == num_diff' failed.
```

I think this stems from the fact that the minisketch implementation now might differ between the sketches in the test.

💬 fanquake commented on pull request "build: Require libc++-16 or later":
(https://github.com/bitcoin/bitcoin/pull/29077#discussion_r1426530173)
Given https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1855569550 and https://godbolt.org/z/n4oTE9sTE the `libstdc++-10` requirement isn't going to work here? Looks like this really needs to be clang-16.

💬 maflcko commented on pull request "build: Require libc++-16 or later":
(https://github.com/bitcoin/bitcoin/pull/29077#discussion_r1426531861)
CI is green, though? :sweat_smile:

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1426534962)
I understand your point.
`TrimToSize` docstring says
```cpp
/** Remove transactions from the mempool until its dynamic size is <= sizelimit.
```
Now `TrimToSize`removes transaction even if the mempool is not full if their are 0 fee transactions.
the docstring should be updated to reflect the operation and maybe the name?

💬 maflcko commented on pull request "build: Require libc++-16 or later":
(https://github.com/bitcoin/bitcoin/pull/29077#discussion_r1426535623)
So as long as you don't need deferred concepts instantiation, it should be fine.

📝 maflcko opened a pull request: "build: Bump guix time-machine to unlock riscv64 metal"
(https://github.com/bitcoin/bitcoin/pull/29078)
Closes https://github.com/bitcoin/bitcoin/issues/29020

💬 maflcko commented on pull request "build: Bump guix time-machine to unlock riscv64 metal":
(https://github.com/bitcoin/bitcoin/pull/29078#issuecomment-1855611758)
Is there an easy way to print all affected/changed/bumped dependencies in the Bitcoin Core build graph?

💬 hebasto commented on pull request "Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1855630481)
Rebased and implemented @Sjors's [suggestions](https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1854029824).

Thank you @Sjors for testing!

🤔 Sjors reviewed a pull request: "wallet: Have the wallet store the key for automatically generated descriptors"
(https://github.com/bitcoin/bitcoin/pull/26728#pullrequestreview-1781522971)
059d847810051b52d33b5b16711a031c3fd199a9 looks good, except that you need to call `SetActiveHDKey()` in `CWallet::EncryptWallet` (to avoid needlessly running the upgrade code).

> Note that key rotation on encryption can happen only once in a wallet's lifetime - when the wallet is encrypted the first. We do not do key rotation when the passphrase changes.

TIL you can't turn off encryption once it's turned on (`walletpassphrasechange` does not allow `""` as the password, unlike `createwallet
...

💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1426540509)
ce5495a4b27fe9646c9933dc39f0cb4e8510eaff: I still think it's nicer to check

```cpp
// Upgrade the wallet to have a global HD key, or in the special case
// where the user downgraded and then encrypted / decrypted.
if(!pwallet->IsWalletFlagSet(WALLET_FLAG_GLOBAL_HD_KEY) || enc_status != IsCrypted()) {
```

here rather than inside `UpgradeToGlobalHDKey`, so that it's clear to people reading this code when they need to open `wallet.cpp` and look at the function body.

Alternatively, you
...

💬 hebasto commented on pull request "Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1855651903)
> For anyone testing with HWI, consider also testing #24313.

Here is the combined [`231130-replace-bp+pr24313`](https://github.com/hebasto/bitcoin/tree/231130-replace-bp%2Bpr24313) branch.

💬 maflcko commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#issuecomment-1855661692)
I wonder if this should instead be put in the GUI window and RPC help for the encrypt action, otherwise it seems easy to miss?

The GUI spreads the information over three pop-up windows, which doesn't seem great, when it can be put into just one Window.

📝 maflcko opened a pull request: "fuzz: Limit p2p fuzz targets to MAX_PROTOCOL_MESSAGE_LENGTH"
(https://github.com/bitcoin/bitcoin/pull/29079)
Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=65039

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1426611988)
Thank you, `HasNonStandardInput` makes more sense, rename to `HasNonStandardInput` in 94cd47f3460

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1426612278)
Yes, fixed now