👍 ryanofsky approved a pull request: "util: Abort on failing CHECK_NONFATAL in debug builds"
(https://github.com/bitcoin/bitcoin/pull/32588#pullrequestreview-3063286442)
Code review ACK fa37153288ca420420636046ef6b8c4ba7e5a478, just catching subprocess.CalledProcessError in test fixing up a comment since last review

👍 pablomartin4btc approved a pull request: "doc: Add legacy wallet removal release notes"
(https://github.com/bitcoin/bitcoin/pull/33075#pullrequestreview-3063302337)
ACK fa45ccc15dfc52e798da62548dc43d1bd7889c9a

🤔 janb84 reviewed a pull request: "doc: Add legacy wallet removal release notes"
(https://github.com/bitcoin/bitcoin/pull/33075#pullrequestreview-3063334927)
re ACK fa45ccc15dfc52e798da62548dc43d1bd7889c9a

changes since last ACK:
- removed double mention of `newkeypool `

💬 l0rinc commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#issuecomment-3127754875)
Concept ACK

⚠️ enirox001 opened an issue: "Build warnings from deprecated std::get_temporary_buffer in std::stable_sort calls"
(https://github.com/bitcoin/bitcoin/issues/33080)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

Clang 20 generates deprecation warnings when std::stable_sort internally calls std::get_temporary_buffer (deprecated in C++17, removed in C++26).

### Expected behaviour

Clean builds without deprecation warnings.

### Steps to reproduce

Compile with Clang 20 (observed during fuzz build):

```
cmake --preset=libfuzzer
cmake --build build_fuzz
```

### Relevant log output

```
warning: 'ge
...

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237012262)
We did try this originally, but (very annoyingly) it does not seem to be possible without a primary workflow to call this workflow. The reason is that the `runs-on` field only has access to the following "contexts":

```
github, needs, strategy, matrix, vars, inputs
```

See https://docs.github.com/en/actions/reference/workflows-and-actions/contexts

Whilst `vars` sounds like it would do the job, the problem is that in workflows of the type `on: pull_request` that `vars` (which refer her
...

💬 murchandamus commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3127923208)
@RobinLinus, are you planning on working on this?

💬 theStack commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3127965996)
Concept ACK

Makes sense to keep the maxorphantx option for a while to only show a warning. Changes look good at first glance, will do a deeper dive with focus on test and fuzzing changes (considering https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3120296633) during the week.

📝 fanquake opened a pull request: "[NO MERGE]: TSAN should fail"
(https://github.com/bitcoin/bitcoin/pull/33081)
Ref: https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3114706601.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3128011661)
Updated 1ffc1c9d94b16cdbfb92a26d0f0e75451efad4fe -> 938767d957b7669accfb554a7cbb25141f7e8632 ([kernelApi_45](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_45) -> [kernelApi_46](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_46), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_45..kernelApi_46))

* Fixed symbol visibility for windows static builds and simplified the macro checks in the header a bit.
* Removed the kernel library symbol visibility hack, this
...

🤔 stickies-v reviewed a pull request: "kernel: Introduce initial C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3030139760)
Did another review round while updating py-bitcoinkernel. The main themes are:
- using reference counting internally to let us simplify the interface (e.g. expose `kernel_TransactionUndo` and `kernel_Coin` instead of letting the user manage indexes) as well as replace expensive `_copy` operations with `_get` ones
- note: in the WG we have discussed exposing reference counting through the public interface too, but personally I don't see the benefits for that complexity yet, even if I'm happy
...

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2214073393)
clang-format sorts this alphabetically, but `windows.h` needs to be included before `shellapi.h` otherwise we get [build errors](https://github.com/stickies-v/bitcoin/actions/runs/16352515776/job/46202676758#step:10:953) like:

```
C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\um\shellapi.h(68,1): error C2146: syntax error: missing ';' before identifier 'DECLSPEC_IMPORT' [D:\a\bitcoin\bitcoin\build\src\bitcoin-chainstate.vcxproj]
(compiling source file '../../src/bitcoin-chai
...

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2213968769)
Should this be const? It's not moveable as-is. If intentional, brief docstring would be good?
```suggestion
std::unique_ptr<kernel_BlockUndo, Deleter> m_block_undo;
```

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2213740015)
nit: couple of naming mismatches after latest force pushes:

<details>
<summary>git diff on 1ffc1c9d94</summary>

```diff
diff --git a/src/kernel/bitcoinkernel.h b/src/kernel/bitcoinkernel.h
index b72c001d1b..ec4db4e7c7 100644
--- a/src/kernel/bitcoinkernel.h
+++ b/src/kernel/bitcoinkernel.h
@@ -130,7 +130,7 @@ typedef struct kernel_TransactionOutput kernel_TransactionOutput;
*
* Messages that were logged before a connection is created are buffered in a
* 1MB buffer. Logging c
...

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2216084200)
This looks like it leaks memory since we never deallocate `block`. Perhaps using `std::unique_ptr` here is a better approach? This (+`kernel_block_undo_read`) seems like the most dangerous one, but perhaps good practice to do this in other places we allocate memory, e.g. [here](https://github.com/bitcoin/bitcoin/pull/30595/files#diff-cc28221ef8d0c7294dda4e3df9f70bb6c062006b387468380c2c2cc02b6762c3R421).

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237124698)
We need `kernel_BlockPointer` because the validation interface gives us a non-owning reference. It would imo be a lot nicer if we could generalize this into `kernel_Block`, so I have opened #33078 to improve ownership semantics in the validation interface, after which `kernel_BlockPointer` can then be removed, e.g. as in https://github.com/stickies-v/bitcoin/commits/kernel/remove-blockpointer/

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2216251677)
Since we use reference counting here, would it be useful to document that in the documentation?
```suggestion
* Destroy the block. Handle is invalidated immediately, block is destroyed as soon as no references remain.
```

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237111430)
I would prefer to expose `kernel_TransactionUndo` and `kernel_Coin` handles so we can generalize iterating over these nested containers.

Using shared_ptr and aliasing constructors, we can do so without incurring any copies (but at the cost of allocating shared_ptr and incrementing the reference counter). In my view this is both more ergonomic (by exposing dedicated types) and performant (by avoiding the need for any `_copy` operations (except for the `kernel_ByteArray` ones).

If necessary,
...

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2219798945)
We should probably use `cast_transaction_output` here to make sure we're not miscasting anything?

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2237140589)
I think the implementation would be a lot cleaner and safer if we didn't use `reinterpret_cast` (or only in rare, targeted cases) but instead just implement the `kernel_` structs in the .cpp file (keeping it hidden to the user). It does add minimal overhead by allocating a (usually trivial) struct, but in most cases that is infrequent and the cost negligible. If necessary, we could still re-introduce `reinterpret_cast` in places where we observe that it does affect performance, but I would stron
...

💬 fanquake commented on pull request "guix: warn SOURCE_DATE_EPOCH set in guix-codesign":
(https://github.com/bitcoin/bitcoin/pull/33073#issuecomment-3128081665)
Guix Build (aarch64):
```bash
b8961023686b6aa3cef95f9f938c1c4b9c1e87945ea60c994dbe56c430faf43e guix-build-1bed0f734b3f/output/aarch64-linux-gnu/SHA256SUMS.part
90f9c040b2640cc7840cdb9341170c754b107ef7c681c762e8d69e3641378322 guix-build-1bed0f734b3f/output/aarch64-linux-gnu/bitcoin-1bed0f734b3f-aarch64-linux-gnu-debug.tar.gz
7ead728efa409a754eea8cd9a41471fc65395e3d8020fd78ba8ad5b8a4dce5ba guix-build-1bed0f734b3f/output/aarch64-linux-gnu/bitcoin-1bed0f734b3f-aarch64-linux-gnu.tar.gz
8c849e
...