🤔 furszy reviewed a pull request: "wallet: guard against dangling to-be-reverted db transactions"
(https://github.com/bitcoin/bitcoin/pull/29253#pullrequestreview-1843953699)
> I'm trying to understand this, but confused about why executing "ROLLBACK TRANSACTION" would be expected to fail in sqlite. It seems like the fix in [e5217fe](https://github.com/bitcoin/bitcoin/commit/e5217fea1516120643f4a7a55a474648e21e2269) is handling failure to roll back by trying to close and reopen the database. But I don't understand why rolling back a transaction would fail to begin with, unless there was some serious error like disk corruption. Probably to make the this fix clearer an
...

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