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

📝 ismaelsadeeq converted_to_draft a pull request: "Fee Estimation: Ignore all transactions that are CPFP'd"
(https://github.com/bitcoin/bitcoin/pull/30079)
Another attempt at #25380 with an alternate approach

This PR updates `CBlockPolicyEstimator` to ignore all transactions that are CPFP'd by some child when a new block is received.
This fixes the assumption that all transactions are confirmed solely because of their fee rate.
As some transactions are confirmed due to a CPFP by some child.



This approach linearize all transactions removed from the mempool because of the new block, and only ignore transactions whose mining score is not
...

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2157939780)
Marking as draft, as I attempt to fix the fuzzing overflow error.

💬 hebasto commented on pull request "build: add `-Wundef`":
(https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2157962927)
> We'll probably need to fixup the failing ARM and previous releases job upstream:
>
> ```shell
> 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 [-Werror=undef]
> 162 | #elif HAVE_CLZ
> | ^~~~~~~~
> ```

The upstream issue has been addressed in https://github.com/sipa/minisketch/pull/88.
...

💬 ismaelsadeeq commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#issuecomment-2157981878)
Rebased to fix conflict after https://github.com/bitcoin/bitcoin/pull/28366 was merged.
[b24b7a9a6..845d16c1b](https://github.com/bitcoin/bitcoin/compare/b24b7a9a6a1a10024c639e9d4ee27aa4d0ce653e..845d16c1bbffb574b22c79ce08e2b6eba31865bc)

💬 maflcko commented on pull request "ci: move ASan job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1633026790)
> I thought it was nicer for someone to be able to run the USDT tests outside of CI by changing an environment variable

This is not enough. They will have to set up the exact system (kernel) outside the CI, as is used inside the CI. Either in a VM or on metal.

Also the new build arg may invalidate the layer cache of unrelated images.

Seems fine, if you feel strongly. Happy to ACK either approach.

👍 stickies-v approved a pull request: "log: use error level for critical log messages"
(https://github.com/bitcoin/bitcoin/pull/30255#pullrequestreview-2107384597)
ACK b90f1c36ff0e469bc64830ff35e591a9aab63fb2 - more consistent logging