💬 hebasto commented on pull request "build: warn on self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30235#issuecomment-2157737186)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/227.

💬 fanquake commented on pull request "build: add `-Wundef`":
(https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2157739910)
> Any reason this is still in draft? I agree it seems useful do enable before the CMake switch.

Rebased, and fixed the conflict. Added another change for the Win64 CI. We'll probably need to fixup the failing ARM and previous releases job upstream:
```bash
In file included from minisketch/src/fields/generic_common_impl.h:12,
from minisketch/src/fields/generic_4bytes.cpp:12:
minisketch/src/fields/../int_utils.h:162:7: error: "HAVE_CLZ" is not defined, evaluates to 0 [-Werr
...

💬 TheCharlatan commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2157745629)
Re https://github.com/bitcoin/bitcoin/pull/30132#pullrequestreview-2106239011

> is this tested anywhere?

No, and I'm not sure it should be given we don't support this. Maybe we can add a comment and assert that just wiping the block index db is not supported for now?

💬 hebasto commented on pull request "build: add `-Wundef`":
(https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2157769746)
> This was the multiprocess job failure:

cc @ryanofsky

💬 cbergqvist commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1632112358)
(Would be nice to add something like
```
# Node3, -bind=... and -listenonion.
self.expected.append(
[
[f"-bind=127.0.0.1:{port}", "-listenonion"],
[(loopback_ipv4, port)]
],
)
port += 1
```
But would then also be nice to verify whether a port was being used for onion traffic or not, to distinguish Node3 from Node2, which I don't know how to do).

💬 cbergqvist commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1632107735)
Thanks for taking my suggestion in https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1607299718! To increase readability one can also use the named loop-variable here, sorry I didn't mention it before:
```suggestion
assert_equal(binds, set(expected_services))
```

💬 cbergqvist commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1632113314)
Feels confusing to have the test framework diverge in behavior this much from default `bitcoind` behavior.

Would be less surprising if the test framework avoided Tor port collisions through spreading out the ports among the nodes as is done for (P2P)Port and RPCPort. A suggested alternative to your current workaround is in 6b785873557696cc611d58fdcac5a3d54622082c.

🤔 ismaelsadeeq reviewed a pull request: "test: doc: fix units in tx-size standardness test (s/vbytes/weight units)"
(https://github.com/bitcoin/bitcoin/pull/30254#pullrequestreview-2107249409)
ACK d1581c6048478cf70c5fb9ec5ebc178f16b376b8

C.I Failure is unrelated https://github.com/bitcoin/bitcoin/actions/runs/9436375635/job/25991001694?pr=30254

💬 ismaelsadeeq commented on pull request "test: doc: fix units in tx-size standardness test (s/vbytes/weight units)":
(https://github.com/bitcoin/bitcoin/pull/30254#discussion_r1632929909)
Are these `bytes` values `vbytes`?

👍 willcl-ark approved a pull request: "build: Remove --enable-gprof"
(https://github.com/bitcoin/bitcoin/pull/30257#pullrequestreview-2107256462)
crACK fa780e1c25e8e98253e32d93db65f78a0092433f

Agree on the rationale given here for removal.

I've previously tried to use `gprof` in place of `perf` (when `addr2line` was broken/unusably slow on my Ubuntu kernel) and hit against many of the pain points ("requirements") @maflcko mentions in OP, which also had me wondering how often this is really used...

I did find a reference to `gprof` remaining in depends: https://github.com/bitcoin/bitcoin/blob/7fd4905c403ccb233f489e2f6a6aa3cd53b033
...

💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#issuecomment-2157846717)
Updated https://github.com/bitcoin/bitcoin/commit/b44f00251a77a3360888eeb10ec903375eb1e0f8 -> https://github.com/bitcoin/bitcoin/commit/f004b1625bcb9b833f36af47b53675e3dd77135b ([serialize-size-for-crecipient-00](https://github.com/josibake/bitcoin/tree/serialize-size-for-crecipient-00) -> [serialize-size-for-crecipient-01](https://github.com/josibake/bitcoin/serialize-size-for-crecipient-01), [compare](https://github.com/josibake/bitcoin/compare/serialize-size-for-crecipient-00..serialize-size-
...

💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1632944441)
Update, thanks @S3RK !

💬 maflcko commented on pull request "build: Remove --enable-gprof":
(https://github.com/bitcoin/bitcoin/pull/30257#issuecomment-2157852153)
> I did find a reference to `gprof` remaining in depends:

I think this is unrelated, because it relates to compiling the qrencode package, not Bitcoin Core itself. Happy to review a separate pull, if there is one.

> I think this is also unneeded, as libqrencode seems to default to off in the configure script:



depends uses cmake to compile the qrencode package, but it seems off there as well https://github.com/fukuchi/libqrencode/blame/715e29fd4cd71b6e452ae0f4e36d917b43122ce8/CMakeLi
...

🤔 glozow reviewed a pull request: "test: doc: fix units in tx-size standardness test (s/vbytes/weight units)"
(https://github.com/bitcoin/bitcoin/pull/30254#pullrequestreview-2107278451)
ACK d1581c6048478cf70c5fb9ec5ebc178f16b376b8

functional test CI failure looks unrelated

💬 glozow commented on pull request "test: doc: fix units in tx-size standardness test (s/vbytes/weight units)":
(https://github.com/bitcoin/bitcoin/pull/30254#discussion_r1632947111)
Both AFAICT. Presumably the idea here is to show how weight units are calculated from bytes, so it doesn't make much sense to start from vbytes.

💬 fanquake commented on pull request "build: Remove --enable-gprof":
(https://github.com/bitcoin/bitcoin/pull/30257#issuecomment-2157858346)
Nothing should be required here in regards to qrencode. I agree that this can just be removed.

💬 dergoegge commented on pull request "fuzz: Use std::span in FuzzBufferType":
(https://github.com/bitcoin/bitcoin/pull/30229#issuecomment-2157858331)
I'm out of the loop on converting `Span` to `std::span`, why is ok to do this here but not everywhere?

💬 josibake commented on pull request "refactor: reserve memory allocation for transaction outputs":
(https://github.com/bitcoin/bitcoin/pull/30093#issuecomment-2157864160)
Concept ACK

I noticed you have benchmarks, would be nice to have the comparison of old vs new in the PR description.

👍 TheCharlatan approved a pull request: "doc: fixup deps doc after #30198"
(https://github.com/bitcoin/bitcoin/pull/30227#pullrequestreview-2107320741)
ACK e6636ff4ec594a38f2e2c4bda3a9549bbc07118e

🚀 fanquake merged a pull request: "ci: Native Windows CI job cleanup"
(https://github.com/bitcoin/bitcoin/pull/30242)