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

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

Good question!

It is quite some work to do it everywhere (I am working on it), because `Span` and `std::span` differ in their interface. (One is lacking methods of the other, and vice-versa).

However, as long as the replacement compiles, it *should* be safe.

Moreover, `FuzzBufferType` is a distinct type, mostly to denote the type of a byte-view, which will be passed to `FuzzedData
...

👍 dergoegge approved a pull request: "fuzz: Use std::span in FuzzBufferType"
(https://github.com/bitcoin/bitcoin/pull/30229#pullrequestreview-2107338508)
utACK fabc9b02331ad6d5cbae3d351721e7e5d9585df0

💬 m3dwards commented on pull request "ci: move ASan job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1632985880)
Initially I changed it as I didn't understand why it needed the `sed`, but after I did understand, I thought it was nicer for someone to be able to run the USDT tests outside of CI by changing an environment variable rather than changing the contents of a file.

Happy to change it back to the `sed` approach.

💬 m3dwards commented on pull request "ci: move ASan job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1632994981)
Yes, there are two issues that this fixes. One is with the RPC server and another is with `-proxy`. Both issues stem from the fact that the containers have IPV6 but only on the loopback interface (`::1`).

I have made a separate PR for the `-proxy` one which is https://github.com/bitcoin/bitcoin/pull/30245#issuecomment-2154778040

There is an existing (closed) issue for the bind problem which is https://github.com/bitcoin/bitcoin/issues/13155 but this is harder to fix as it's in libevent. I
...