🤔 mzumsande reviewed a pull request: "init: [gui] Avoid UB/crash in InitAndLoadChainstate"
(https://github.com/bitcoin/bitcoin/pull/32987#pullrequestreview-3069373388)
Tested ACK fac90e5261b811739ada56e06ea793a12f9c2c3d

Unrelated, but just mentioning that reindex with the GUI is not great: I tried it with a regtest chain of 1000 empty blocks and the app froze for a few seconds / my OS asked me whether it should terminate the non-responding program. Didn't try it with mainnet, but it's probably not a smooth experience.

📝 l0rinc opened a pull request: "refactor: remove unused `ser_writedata16be` and `ser_readdata16be`"
(https://github.com/bitcoin/bitcoin/pull/33093)
Remove dead code from serialization logic - extracted from https://github.com/bitcoin/bitcoin/pull/31868

💬 l0rinc commented on pull request "[IBD] specialize block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-3134384578)
Rebased to solve conflict, the PR is still in draft mode until I remeasure its effect on IBD - and I need to find a nicer way to do the single-byte specializations, since they have a measurable effect but make the code super-ugly... Suggestions are welcome.
In the meantime I have split out the first refactor into https://github.com/bitcoin/bitcoin/pull/33093.

📝 l0rinc opened a pull request: "refactor: unify container presence checks"
(https://github.com/bitcoin/bitcoin/pull/33094)
### Summary
Instead of counting occurrences in sets and maps, the C++20 `::contains` method expresses the intent unambiguously and can return early on first encounter.

### Context
Applied clang‑tidy's [readability‑container‑contains](https://clang.llvm.org/extra/clang-tidy/checks/readability/container-contains.html) check, though many cases required manual changes since tidy couldn't fix them automatically.

### Changes
The changes made here were:

| From | To
...

💬 Christewart commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#issuecomment-3134394012)
I believe this deserves a release note as it can break existing calls to `dumptxoutset` if you were previously omitting the `type` parameter?

> This parameter can be omitted if a separate \"rollback\" named parameter is specified indicating the height or hash of a specific historical block.

💬 kevkevinpal commented on pull request "qa: test that we do not disconnect a peer for submitting an invalid compact block":
(https://github.com/bitcoin/bitcoin/pull/33083#issuecomment-3134529754)
ACK [c157438](https://github.com/bitcoin/bitcoin/pull/33083/commits/c1574381168573c561ebddf1945d2debefb340f7)

I was able to reproduce the errors with the diffs provided above, and was also able to validate that the master branch tests pass when the diffs are applied as well

💬 ryanofsky commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#issuecomment-3134531569)
> I believe this deserves a release note as it can break existing calls to `dumptxoutset` if you were previously omitting the `type` parameter?

Yes it would be good to add a release note saying that `dumptxoutset` can no longer be called with just a filename to produce a snapshot with the latest UTXO state. It now needs an additional `type` argument that can be set to "latest" to match previous behavior. It can also take other arguments and is now capable of rolling back the chain and produci
...

💬 maflcko commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#issuecomment-3134929957)
29.0 is already tagged (archived), built, and released for a long time, so it isn't possible to change the release notes there now.

👋 l0rinc's pull request is ready for review: "refactor: unify container presence checks"
(https://github.com/bitcoin/bitcoin/pull/33094)

💬 rkrux commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-3135054321)
Thanks for addressing the comments, I will take one final look and then a-c-k.

Tagging @furszy for review as he noticed this issue as well few months back.

📝 maflcko opened a pull request: "ci: Remove unused ci/lint_run_all.sh"
(https://github.com/bitcoin/bitcoin/pull/33095)
As per the file content, this can be deleted now. (Stale re-runs are no longer performed for `lint` right now and the CI will be reworked anyway)

✅ maflcko closed a pull request: "ci: Remove unused ci/lint_run_all.sh"
(https://github.com/bitcoin/bitcoin/pull/33095)

💬 maflcko commented on pull request "ci: Remove unused ci/lint_run_all.sh":
(https://github.com/bitcoin/bitcoin/pull/33095#issuecomment-3135184921)
Hmm, let's just do it in the other pull.

👍 maflcko approved a pull request: "Migrate CI to hosted Cirrus Runners"
(https://github.com/bitcoin/bitcoin/pull/32989#pullrequestreview-3070236084)
everything looks good and this is probably ready for final review before merge? I just found some nits, which can be done along with follow-ups like https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2236533948. Also, the merge_base issue mentioned above can be addressed in a follow-up.


However, I am not familiar with the GitHub-style CI, including the (docker) caching, but the pull request motivation claims that many are familiar with this, so it should be easy to find reviewers for i
...

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2241797039)
nit in 01746e60eb82b9bedb3afa1e791b5abce75ebf75: Unused? This task is using nproc?

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2241816383)
nit in 4a1c5b04691266f1441fd966a4be65140f9a5cc4: Outdated doc?

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2241782043)
You can also remove the now unused stuff:

```
ci/lint_run.sh:# Only used in .cirrus.yml. Refer to test/lint/README.md on how to run locally.
ci/lint_run_all.sh:# Only used in .cirrus.yml for stale re-runs of old pull request tasks. This

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2241802051)
nit in https://github.com/bitcoin/bitcoin/commit/01746e60eb82b9bedb3afa1e791b5abce75ebf75: Any reason to change this setting for the other tasks, but not for this one? For reference, the task has 3 mac-arm cpu threads.

💬 ajtowns commented on pull request "p2p: never check tx rejections by txid":
(https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3135258539)
> TLDR, this PR removes the rejection cache filtering part of all txid-based tx requests.

There's three cases here I think:

* orphan resolution from wtxidrelay peers (WTXIDRELAY)
* tx relay from segwit-supporting, non-wtxidrelay peers (NODE_WITNESS, no WTXIDRELAY, 0.13.1 to 0.20.x, Jan 2021)
* tx relay from non-segwit peers

The last case case is trivial -- non-segwit peers can be treated as doing wtxid relay for caching because all the txs they relay will have the same value for tx
...

💬 maflcko commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-3135294275)
This seems to be causing CI failures:

* https://cirrus-ci.com/task/5499213573783552?logs=ci#L1556
* https://cirrus-ci.com/task/5576386364047360?logs=ci#L1556
* https://cirrus-ci.com/task/6693205086830592?logs=ci#L1556
* ...