💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3127615193)
Thanks for the reviews!

Updated 3d099384a60c8f64c8d67a9f1e7b8649a9c54313 -> e1f139bd5fc9ee737d2b08307fca2b33354c5747 ([`pr/mine.29`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.29) -> [`pr/mine.30`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.30), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.29..pr/mine.30)) just fixing comments to clarify things.

---

re: https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3116367414

> It would be better to
...

💬 ajtowns commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2236858410)
Sounds fine.

Another option would be to move `info_for_relay` from txmempool.h into net_processing.cpp:

```c++
/** Returns info for a transaction if its entry_sequence < last_sequence */
static TxMempoolInfo info_for_relay(const CTxMemPool& mempool, const GenTxid& gtxid, uint64_t last_sequence)
{
return std::visit([&](const auto& id) {
LOCK(mempool.cs);
auto i{mempool.GetIter(id)};
return (i.has_value() && i.value()->GetSequence() < last_sequence) ? mempo
...

👍 ryanofsky approved a pull request: "rpc: Handle -named argument parsing where '=' character is used"
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3063259562)
Code review ACK fa37153288ca420420636046ef6b8c4ba7e5a478. Thanks for the updates! Only change since last review was clarifying a comment and some variable names and adding new tests to cover cases where non-string positional parameters containing '=' are passed.

👍 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.
```