💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2437862407)
Done in #33591 (d7109669b54d299e84dfe90d2ebf591ff673f51c)

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2437897737)
Thanks -- I'm going to make it "virtual size with all in-mempool..." rather than "virtual kilobytes", which I think is more legible?

🤔 kannapoix reviewed a pull request: "rpc: Handle -named argument parsing where '=' character is used"
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3348459569)
Code review ACK: f53dbbc5057b6f676db4be9bc720898149f293fc

💬 kannapoix commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2438509207)
>we need to mention that all the parameters' names correspond to the method.

Oh, I missed that point even though I read the comment—thanks for the explanation.
I agree the getrawchangeaddress error message is still reasonable, so I think the current implementation makes sense.

💬 maflcko commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#discussion_r2438644126)
```
# echo 'XGFkZAAAAGRkZWXuXP/fcGcqb2hlcirYfg9D/uXc5eXcRZJ55eXl5eXl5eXlIiL19QAFABD3XERc
AVxhYQcAAADl5f//5eVhYWHl5eX//+Xl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl
5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eX/Km8xMTQyMjgxMUMKYWFhYWFhYQAAAAAA
YWFhYWFhYWFhYWFhYWFhe2FhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFh8mWkovx0AAAA
AAAAAGFhYWFhYWFhYWFhgKoL//v/Kv/////l5eXl5f//ZGRy5eX//2Ry5eX///9kZHLl5f//ZHLl
5f//5eXl5eXl5eXl5Wfl//9kZHLl5f//ZHLl5f///2RkcuXl//9kcuXl//8=' | base64 --decode > ./crash_cm_1cfc
...

⚠️ maflcko opened an issue: "fuzz: connman fuzz target: runtime error: null pointer passed as argument 2, which is declared to never be null"
(https://github.com/bitcoin/bitcoin/issues/33643)
```
# echo 'XGFkZAAAAGRkZWXuXP/fcGcqb2hlcirYfg9D/uXc5eXcRZJ55eXl5eXl5eXlIiL19QAFABD3XERc
AVxhYQcAAADl5f//5eVhYWHl5eX//+Xl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl
5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eXl5eX/Km8xMTQyMjgxMUMKYWFhYWFhYQAAAAAA
YWFhYWFhYWFhYWFhYWFhe2FhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFh8mWkovx0AAAA
AAAAAGFhYWFhYWFhYWFhgKoL//v/Kv/////l5eXl5f//ZGRy5eX//2Ry5eX///9kZHLl5f//ZHLl
5f//5eXl5eXl5eXl5Wfl//9kZHLl5f//ZHLl5f///2RkcuXl//9kcuXl//8=' | base64 --decode > ./crash_cm_1cfc
...

🤔 musaHaruna reviewed a pull request: "test: [move-only] binary utils to utils.py"
(https://github.com/bitcoin/bitcoin/pull/33633#pullrequestreview-3348893757)
Code Review ACK [fa75ef4](https://github.com/bitcoin/bitcoin/pull/33633/commits/fa75ef4328f638221bcf85fcbefa885122084622)

PR looks straight forward, moving binaries to util.py files, help make them reusable and not bound to the `Test framework`.

> The config is written by the build system, so I don't think it is coupled with the TestFramework class itself.

Also confirmed that the config is written by the build system located at `bitcoin/build/test/config.ini`.
From my understanding [b
...

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2438856790)
I think it is fine without comment, because the files are numbered in order and not too many, so one can just read/grep them all to search what they are looking for.

💬 fanquake commented on issue "fuzz: connman fuzz target: runtime error: null pointer passed as argument 2, which is declared to never be null":
(https://github.com/bitcoin/bitcoin/issues/33643#issuecomment-3414432758)
cc @vasild

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2438862940)
I don't think `MAKEJOBS=-j0` was ever supported by the CI. It will eventually be passed down to the test runner, which fails:

```
test/functional/test_runner.py", line 714, in __init__
assert num_tests_parallel >= 1
^^^^^^^^^^^^^^^^^^^^^^^
AssertionError
```

So I think I'll leave this as-is for now, to keep the code move-only, as the goal is not to change it. It can be changed in a separate PR, if there is need.

Just as a hint: If you want to use `nproc`, with the cu
...

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2438863485)
Ah, very nice catch. I had considered this case, but forgot to set the empty string before pushing the branch. Fixed now and pushed.

💬 maflcko commented on issue "ci: short read: expected xxxxxxxxx bytes but got xxxxxxxxx: unexpected EOF":
(https://github.com/bitcoin/bitcoin/issues/33640#issuecomment-3414500066)
Normally, network requests are retried in CI to work around intermittent network issues. I guess it could be tried here as well.

I can take a look next week.

💬 janb84 commented on pull request "test: [move-only] binary utils to utils.py":
(https://github.com/bitcoin/bitcoin/pull/33633#discussion_r2438919936)
Just to clarify, i'm not saying that it coupled to the test_framework, but that it is coupled to the config.
For a followup I could see something like the code below, for this PR I like that it is more or less a code move.

Rough example; **not** ment to be a nit:
```suggestion
def get_binary_paths(bin_path, exe_ext):
"""Get paths of all binaries from environment variables or their default values"""

paths = types.SimpleNamespace()
binaries = {
"bitcoin": "BITCOIN_
...

💬 maflcko commented on pull request "test: [move-only] binary utils to utils.py":
(https://github.com/bitcoin/bitcoin/pull/33633#discussion_r2438934710)
thx, I'll leave as-is for now, but this can certainly be pushed in the future, if and once there is additional need to uncouple from the config

💬 Raimo33 commented on pull request "refactor: optimize block index comparisons (1.4-7.7x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#issuecomment-3414573082)
Approach ACK

I have tested the new block index comparator but I’ll refrain from acking the added benchmarks/tests

✅ Raimo33 closed a pull request: "node: optimize CBlockIndexWorkComparator"
(https://github.com/bitcoin/bitcoin/pull/33334)

💬 Raimo33 commented on pull request "node: optimize CBlockIndexWorkComparator":
(https://github.com/bitcoin/bitcoin/pull/33334#issuecomment-3414577094)
Follow up in #33637

📝 vasild opened a pull request: "fuzz: don't pass (maybe) nullptr to memcpy()"
(https://github.com/bitcoin/bitcoin/pull/33644)
If `ConsumeBytes()` returns an empty vector, then its `data()` method may or may not return a null pointer [1]. Its `size()` method will return 0 and `memcpy(dst, maybenull, 0)` will be called from `FuzzedSock::Accept()`.

Given that the `len` argument is 0, `memcpy()` should not try to dereference the maybenull argument, but nevertheless the sanitizer is upset:

```
./src/test/fuzz/util/net.cpp:337:43: runtime error: null pointer passed
as argument 2, which is declared to never be null
`
...

💬 vasild commented on issue "fuzz: connman fuzz target: runtime error: null pointer passed as argument 2, which is declared to never be null":
(https://github.com/bitcoin/bitcoin/issues/33643#issuecomment-3414868958)
Couldn't reproduce, but is pretty obvious, _should_ be fixed by https://github.com/bitcoin/bitcoin/pull/33644

Did I base64 decode wrongly in some way (I had to manually remove the newlines from the command above)?

```
SHA256 (crash_cm_1cfcffc33a) = 4ac50f0fa637d94fa48430e371f51ae97bb34fd99261a11e78daaa283f620a3b
```

👍 rkrux approved a pull request: "wallet: allow label for non-ranged external descriptor (if `internal=false`) & disallow label for ranged descriptors"
(https://github.com/bitcoin/bitcoin/pull/31514#pullrequestreview-3349361024)
lgtm crACK 664657ed134365588914c2cf6a3975ce368a4f49

Good find!
Though I find the last paragraph in the PR description slightly confusing:

> If you remove label from external descriptor import object - it will import no problem:
Even tho it should NOT, as the descriptor is ranged.

If the label is not present at all, the ranged descriptors can/should be imported. Instead, I find the text in this comment to be helpful and feel it can be added in the PR description: https://github.com/bit
...