💬 instagibbs commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466473683)
```Suggestion
// we don't conflict inputs with it, consider evicting it under RBF rules. We rely on v3 rules
```

💬 instagibbs commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466475345)
aside: In relaxed topos we just attempt the eviction of *all* of them

💬 instagibbs commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466484490)
this return value needs to be explained very well

🤔 instagibbs reviewed a pull request: "policy: enable sibling eviction for v3 transactions"
(https://github.com/bitcoin/bitcoin/pull/29306#pullrequestreview-1843901103)
very compact!

alternative log lines are a bit noisy, but understandable and I have no good suggestions that reduce that much

💬 instagibbs commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466538340)
needs test coverage

💬 instagibbs commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466531988)
did you consider internalizing this logic inside `ApplyV3` directly to reduce conceptual sprawl

👍 fanquake approved a pull request: "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`"
(https://github.com/bitcoin/bitcoin/pull/29287#pullrequestreview-1844036451)
ACK 5fb8f0f80fc41cc636da56864195244d8fd9116e - downstream is also green, so i'll fixup the PR there.

💬 fanquake commented on pull request "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`":
(https://github.com/bitcoin/bitcoin/pull/29287#discussion_r1466555308)
This should really be done globally, but I can follow up, as I have another global change to make at the same time.

💬 fanquake commented on pull request "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`":
(https://github.com/bitcoin/bitcoin/pull/29287#discussion_r1466556573)
This comment could be a bit confusing, because it says we don't want to use `--enable-debug`, because it overrides cflags, but then we override cflags in the next line. Not a blocker though.

📝 BrandonOdiwuor opened a pull request: "test: Check object hashes in wait_for_getheaders"
(https://github.com/bitcoin/bitcoin/pull/29318)
Fixes https://github.com/bitcoin/bitcoin/issues/18614

Previously, wait_for_getheaders only looked for the presence of a recent "getheaders" message. Additionally checking the hashstop inside the message should make tests involving wait_for_getheaders more robust.

This PR is progress towards closing issue: https://github.com/bitcoin/bitcoin/issues/18614 after PR: https://github.com/bitcoin/bitcoin/pull/18690 (merged) which introduced this check for wait_for_getdata

👍 fanquake approved a pull request: "build: Pass sanitize flags to instrument `libsecp256k1` code"
(https://github.com/bitcoin/bitcoin/pull/28875#pullrequestreview-1844052794)
ACK cbea49c0d32badb975fbf22d44f8e25cc7972af7

💬 mzumsande commented on pull request "validation: fix misleading checkblockindex comments":
(https://github.com/bitcoin/bitcoin/pull/29299#issuecomment-1910472518)
Changed the title as suggested.

> I also think it would be good to split up the assert and make it stricter, so feel free to use the code from [#29261 (comment)](https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1908513362)

The suggestion looks good to me just by reading the code, but I want to test it more before including it. Will either update the PR in a few days, or alternatively (I don't mind either way) that could be a follow-up to this trivial doc-only PR.

💬 stratospher commented on pull request "test: p2p: check disconnect due to lack of desirable service flags":
(https://github.com/bitcoin/bitcoin/pull/29279#discussion_r1466564903)
358561e: nit: s/0/`NODE_NONE`

🤔 stratospher reviewed a pull request: "test: p2p: check disconnect due to lack of desirable service flags"
(https://github.com/bitcoin/bitcoin/pull/29279#pullrequestreview-1844051383)
Concept ACK! i think flaky tests are possible because disconnection could happen before [`p2p_conn.wait_for_connect()`](https://github.com/bitcoin/bitcoin/blob/7699a1aab8fa2a7a602391025f3cfcde29ff2613/test/functional/test_framework/test_node.py#L728) in `add_outbound_connection()`?

🚀 fanquake merged a pull request: "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`"
(https://github.com/bitcoin/bitcoin/pull/29287)

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466573331)
I installed Podman 4.8.3 from source on my machine. However I don't understand what you mean by `run --replace`. It seems we're only using podman in the `RESTART_CI_DOCKER_BEFORE_RUN`, but still call `docker run` everywhere. Can we just swap `docker run` with `podman run` or is it more complicated?

💬 fanquake commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466579076)
I don't have docker installed on any machine I use for CI. Only podman, and a docker -> podman alias (podman-docker).

💬 fanquake commented on pull request "ci: Use DEBUG=1 in depends for MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27495#discussion_r1466585572)
Pushed `release/18.x` now to see what breaks. Will switch to the tag once it's available.

💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1910522489)
> if there is a failure during the WalletBatch destruction (independently on if it is due to a fatal or transient and recoverable error, like a busy db or a loss of the db connection)

Yes I've started reviewing both PR's and the code changes seem reasonable, I just don't understand what motivated this PR, because "failure during the WalletBatch destruction" seems like something I would never expect to happen unless someone modified the code or manually corrupted the data on disk. So I'm wond
...

💬 real-or-random commented on pull request "build: Pass sanitize flags to instrument `libsecp256k1` code":
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1466598461)
My take on `SECP_CFLAGS` is that it's an internal of the libsecp256k1 build system, and users are not supposed to set it (and are not guaranteed that it does anything meaningful). The proper user variable is just `CFLAGS`.

You could either export `CFLAGS="-g $SANITIZER_CFLAGS"` temporarily, or perhaps add a CFLAGS arg to `ac_configure_args`.

We could also expose the `SECP_CFLAGS` "officially" in the lib. Then it should probably be renamed to `SECP256K1_CFLAGS`. But I think it's a bit arbi
...