💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2004100921)
I wrote this because the following line (`static_assert((step & (step-1)) == 0)`) doesn't actually assert that `step` has a single bit set (which is what I wanted to check); it also succeeds if `step` is zero. But using `std::has_single_bit()` is better, so we can remove `static_assert(step > 0)`, good catch.

💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2004197157)
I think it's okay that it's initially dead code because it makes the test less fragile in general. In theory, a different project could cherry-pick just this test-only commit, and later make some other changes that would have caused this test to break (when it's really a false-positive failure), but because of taking this commit, that doesn't happen (the test continues to pass). Or, a similar way to think of it is, the test could have been written this way to begin with (and that would not have
...

💬 jamesob commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2738021374)
> I suggest a regtest-only deployment is defined so functional testing can be re-instated. Currently there is zero consensus coverage at the functional level from what I can tell: [4c57901](https://github.com/bitcoin/bitcoin/commit/4c5790105e7fb3a2cd207403af769fbbccd7296d)

Done in https://github.com/bitcoin/bitcoin/pull/31989/commits/5bb734b9d3628395c031fdf0eb0fbb249fe578e3.

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin-wallet -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/19460#issuecomment-2738025949)
Updated 240bc4798a3c4e991e153d6660509c78323fe937 -> ce32dc3958c9e3610bd7113551a8a43396d0019d ([`pr/ipc-connect.42`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-connect.42) -> [`pr/ipc-connect.43`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-connect.43), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-connect.42..pr/ipc-connect.43)) to fix CI failure in tool_wallet.py https://cirrus-ci.com/task/4513424845045760 where `bitcoin-wallet` throws an exception because the t
...

💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2004255350)
I would like to avoid having a false sense of security by putting this before the fix, which usually indicates that the desired behavior is reproduced and is retained after this fix - which isn't the case here exactly. And the dead code part also bothers me for a similar reason.

💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2004259676)
I forgot to include this in the full patch, my bad

💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2004261310)
if you decide to keep this test (which was meant to demonstrate the safety of the change), please remove the logging part

💬 ryanofsky commented on pull request "Multiprocess bitcoin":
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2738079385)
Updated ecd8e80fdef8675991d5bb3f9effd83a4ac97d2a -> 26c2136a2bc7fec9697f61eadf422c439e0735a2 ([`pr/ipc.215`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc.215) -> [`pr/ipc.216`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc.216), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc.215..pr/ipc.216)) to CI fix https://cirrus-ci.com/task/5047408398172160?logs=ci#L2206 caused by wallet_encryption.py not checking wallet process log file

💬 mabu44 commented on pull request "net: Block v2->v1 transport downgrade if !fNetworkActive":
(https://github.com/bitcoin/bitcoin/pull/32073#issuecomment-2738096709)
ACK 6869fb417096b43ba7f74bf767ca3e41b9894899
Reviewed the code and tested it with the command provided in https://github.com/bitcoin/bitcoin/pull/32073#issuecomment-2731477125. I was able to reproduce the problem on master only once and never on this branch.

💬 hodlinator commented on issue "Linux download needs installation instructions":
(https://github.com/bitcoin/bitcoin/issues/32097#issuecomment-2738114741)
> https://bitcoin.stackexchange.com/questions/122646/libxcb-xinerama0-library-required-by-bitcoin-qt

(This seems to have led to #30061 with the same title).

💬 hodlinator commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2003066433)
Thread https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2001211668:

Now that the dependencies for bitcoin-qt double from 12->24 it almost feels like they should be in their own list, so we don't accidentally introduce dependencies on them from the CLI binaries?

I can work on a patch for that if there is interest.

💬 hodlinator commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2003451280)
Should ninja-build and CRB be mentioned in build-unix.md or dependencies.md?

👍 hodlinator approved a pull request: "build: Switch to Qt 6"
(https://github.com/bitcoin/bitcoin/pull/30997#pullrequestreview-2697929054)
ACK 94967c353ed8041115625b6b3c9acba7961e1f20

- Beginner at CMake/Qt, but couldn't spot anything malicious-looking in CMake changes nor Qt 6 patches.
- Have not tested MacOS builds. Only Linux, Windows Native, Windows Cross, see [Concept review](https://github.com/bitcoin/bitcoin/pull/30997#pullrequestreview-2572359973).

<details><summary>sha256sum-detour</summary>

> hebasto:
> [My Guix build:](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2736277550)

Surprisingly to me
...

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2738142996)
@hodlinator
> Building on `x86_64`. Maybe just a dirty tree leaking into guix somehow, but surprised that one of the builds matched. :\
> Also built aarch64-linux-gnu, arm-linux-gnueabihf, powerpc64-linux-gnu, riscv64-linux-gnu and x86_64-linux-gnu.

... and hashes do _not_ match https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2736277550, right?

💬 hodlinator commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2738151903)
> @hodlinator
>
> > Building on `x86_64`. Maybe just a dirty tree leaking into guix somehow, but surprised that one of the builds matched. :\
> > Also built aarch64-linux-gnu, arm-linux-gnueabihf, powerpc64-linux-gnu, riscv64-linux-gnu and x86_64-linux-gnu.
>
> ... and hashes do _not_ match [#30997 (comment)](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2736277550), right?

Yes, they do *not* match:

<details><summary>hashes</summary>

```
₿ find guix-build-$(git rev-
...

💬 mabu44 commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2738161185)
In reference to my first comment https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2731126667, I note that the imported descriptor and the one returned in the error message are not equal. I imagine that the one in the error message represents the public key for the provided private key. This could reduce the clarity of the error message that no longer enables a direct comparison with the imported descriptors to understand which one generated the error. On the other end, we do not return
...

💬 willcl-ark commented on issue "test: No unit test covers BIP342 tapscript signatures":
(https://github.com/bitcoin/bitcoin/issues/32012#issuecomment-2738172847)
I also cannot reproduce OP:

```bash
will@ubuntu in …/src/core/bitcoin on  master [$!?] via △ v3.31.6 : 🐍 (core)
$ git diff -p | cat
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index a35306b6935..d2904fb3f14 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -1480,7 +1480,7 @@ bool SignatureHashSchnorr(uint256& hash_out, ScriptExecutionData& execdata, cons
uint8_t ext_flag, key_version;
switch (sigversion) {
case SigVersion::TAPR
...

🤔 sipa reviewed a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2700118245)
Big change: `std::vector<ClusterSet>` is gone, replaced by `m_main_clusterset` and `m_staging_clusterset`.
* New helper functions `GetTopLevel()`, `GetSpecifiedLevel()`, `GetClusterSet()` made this fairly easy to do.
* Simplifications resulted in `ClearLocator`, `GetConflicts`, `PullIn`, `MakeTransactionsMissing`, `StartStaging`, `CommitStaging`, `ExtractCluster`.
* `CopyTo` was renamed to `CopyToStaging`.
* `LevelDown` was renamed to `MoveToMain`.
* `MakeTransactionsMissing` was renamed to
...

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004315650)
`NEEDS_SPLIT_OPTIMAL` is gone now after further discussion. Marking resolved.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004306838)
@ajtowns That comment was put in the wrong commit. I've fixed it.