💬 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)

💬 josibake commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2157883475)
I didn't look too closely into the CI failure reason, but seems like a relatively simple fix, i.e. fixing an signed int overflow:

```
SUMMARY: UndefinedBehaviorSanitizer: signed-integer-overflow node/mini_miner.cpp:196:33
MS: 0 ; base unit: 0000000000000000000000000000000000000000
artifact_prefix='./'; Test unit written to ./crash-2315c89458602360eb93362328da5c0ef21f9864

Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/fu
...

🚀 fanquake merged a pull request: "doc: fixup deps doc after #30198"
(https://github.com/bitcoin/bitcoin/pull/30227)