🚀 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

💬 stickies-v commented on pull request "log: use error level for critical log messages":
(https://github.com/bitcoin/bitcoin/pull/30255#discussion_r1633005257)
I don't think this should be `LogError` as this can be recovered from, e.g. [here](https://github.com/bitcoin/bitcoin/blob/bd642ee15bda313bcf801cf63e4428c7a7d252c8/src/validation.cpp#L6332). Ideally, we just return error messages here and let the callsite log - but barring that I'd just leave as is since all the call sites that do lead to node termination will already log on the error level?

💬 stickies-v commented on pull request "log: use error level for critical log messages":
(https://github.com/bitcoin/bitcoin/pull/30255#discussion_r1633007398)
nit: since we're calling `GetNotifications().fatalError()` next, I think this log line can just be removed?

💬 maflcko commented on issue "RPC wont bind without an IP address on a non-localhost interface":
(https://github.com/bitcoin/bitcoin/issues/13155#issuecomment-2157988114)
Reopening for now, because it came up again. cc @m3dwards

💬 maflcko commented on pull request "log: use error level for critical log messages":
(https://github.com/bitcoin/bitcoin/pull/30255#discussion_r1633033058)
My understanding is that the notification does not include `err.what()`, so the debug log in this line is still needed.

💬 maflcko commented on pull request "log: use error level for critical log messages":
(https://github.com/bitcoin/bitcoin/pull/30255#discussion_r1633037105)
Good point, deferred for the follow-up that handles warnings.

⚠️ maflcko reopened an issue: "RPC wont bind without an IP address on a non-localhost interface"
(https://github.com/bitcoin/bitcoin/issues/13155)
If the only interface which has an IP address and is up is lo, one of the two default rpcbinds (:: and 0.0.0.0) will fail with "libevent: getaddrinfo: address family for nodename not supported".

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1632970572)
nit 0044c1dad9ffc3d58afd06d7533f16beb0d0a829:

Would prefer to not have magic number comments

```suggestion
// Don't consider replacements that would cause us to remove a large number of mempool entries.
// This limit is not increased in a package RBF. Use the aggregate number of transactions.
```