💬 maflcko commented on pull request "scripted-diff: Remove obsolete comment":
(https://github.com/bitcoin/bitcoin/pull/33826#issuecomment-3510244379)
lgtm ACK 36724205fc1f226d7b5493ed0212c336e7f2ae84

👍 hebasto approved a pull request: "guix: build for Linux HOSTS with `-static-libgcc`"
(https://github.com/bitcoin/bitcoin/pull/33181#pullrequestreview-3441742819)
ACK f06c6e18983139dd63873b3537d2f87b8c6ec752.

💬 maflcko commented on pull request "util: Allow `Assert` (et al.) in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2509568255)
Just for ref, one could write this to please clang-format, but not sure if it is worth it:

```cpp
#define NONFATAL_UNREACHABLE() \
do { \
throw NonFatalCheckError{ \
"Unreachable code reached (non-fatal)", \
std::source_location::current(), \
}; \
} while (0)

💬 stickies-v commented on pull request "kernel: trim Chain interface":
(https://github.com/bitcoin/bitcoin/pull/33820#discussion_r2509575390)
> The previous calls to tip and genesis were at least thread safe

With previous, do you mean master? I'm not sure how the current calls are not thread safe? These are read-only calls without side effects? And both in master and this PR, the caller has no guarantees that the returned tip is indeed the tip because cs_main isn't exposed (regardless of threads)?

> I wonder if we should even keep these methods now.

I agree, but I want to generalize that even more. I think `Range` objects sho
...

💬 maflcko commented on pull request "kernel: trim Chain interface":
(https://github.com/bitcoin/bitcoin/pull/33820#discussion_r2509606630)
> With previous, do you mean master? I'm not sure how the current calls are not thread safe? These are read-only calls without side effects? And both in master and this PR, the caller has no guarantees that the returned tip is indeed the tip because cs_main isn't exposed (regardless of threads)?

I think there could be a rare and theoretical race where the function does not return any tip, but rather throws `std::runtime_error("No entry in the chain at the provided height");` when the height i
...

💬 stickies-v commented on pull request "ci: Enable experimental kernel stuff in most CI tasks":
(https://github.com/bitcoin/bitcoin/pull/33824#issuecomment-3510516230)
Concept ACK for standardizing CI workflows around `dev-mode` and for increasing kernel coverage. CI runtimes seem acceptable.

🤔 janb84 reviewed a pull request: "doc: reference fuzz coverage steps in quick-start"
(https://github.com/bitcoin/bitcoin/pull/33536#pullrequestreview-3442038438)
ACK dee7eec64389aa48daff6f7f3ecbd931af72050a

The author removed the duplicate and redundant code coverage instructions which made me disagree with this PR. The author now only references that there are instructions on generating code coverage for fuzzing elsewhere in the documentation, which seems to me as a reasonable thing and good addition.

Thank you for changing this.

✅ fanquake closed an issue: "RFC: Cancelling waitNext calls in the IPC mining interface"
(https://github.com/bitcoin/bitcoin/issues/33575)

🚀 fanquake merged a pull request: "interfaces: enable cancelling running `waitNext` calls"
(https://github.com/bitcoin/bitcoin/pull/33676)

💬 vasild commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#issuecomment-3510573430)
`84b2ad0334...9989853447`: rebase due to conflicts

> I don't think it makes much sense to split this out; it doesn't reduce the size of the parent PR meaningfully,

I agree it does not help much. It helps _a little_. And that is the point - to get some smaller changes out of the main PR.

> and renaming a function doesn't really seem like it justifies a PR on its own.

Having this in its own PR helped have a [fresh discussion](https://github.com/bitcoin/bitcoin/pull/33565#discussion_r24
...

👍 rkrux approved a pull request: "scripted-diff: Remove obsolete comment"
(https://github.com/bitcoin/bitcoin/pull/33826#pullrequestreview-3442161682)
crACK 36724205fc1f226d7b5493ed0212c336e7f2ae84

✅ fanquake closed a pull request: "Check required interfaces before generating manpages"
(https://github.com/bitcoin/bitcoin/pull/33828)

💬 fanquake commented on pull request "Check required interfaces before generating manpages":
(https://github.com/bitcoin/bitcoin/pull/33828#issuecomment-3510609561)
This duplicates #33085. If the author doesn't follow up there shortly, we could reopen here.

💬 dergoegge commented on issue "RFC: Add multiprocess fuzz target":
(https://github.com/bitcoin/bitcoin/issues/23015#issuecomment-3510614348)
I'm working on making it easier to deploy custom testing workloads with fuzzamoto, so that something as simple as a different binary name isn't a struggle (i.e. doesn't require patches). See https://github.com/dergoegge/fuzzamoto/issues/48, which will hopefully also enable the use of python with our testing framework. I'll circle back here once that is done.

🚀 fanquake merged a pull request: "ci: Use cmake --preset=dev-mode in test-each-commit task"
(https://github.com/bitcoin/bitcoin/pull/33823)

👍 dergoegge approved a pull request: "doc: reference fuzz coverage steps in quick-start"
(https://github.com/bitcoin/bitcoin/pull/33536#pullrequestreview-3442187499)
ACK dee7eec64389aa48daff6f7f3ecbd931af72050a

🚀 fanquake merged a pull request: "doc: reference fuzz coverage steps in quick-start"
(https://github.com/bitcoin/bitcoin/pull/33536)

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2509736434)
I don't like this, why not just remove the const?
```patch
-const CBlockIndex& chain_start{WITH_LOCK(::cs_main, return *Assert(m_node.chainman->m_blockman.LookupBlockIndex(genesis.GetHash())))};
+CBlockIndex& chain_start{WITH_LOCK(::cs_main, return *Assert(m_node.chainman->m_blockman.LookupBlockIndex(genesis.GetHash())))};
```

💬 maflcko commented on pull request "ci: Enable experimental kernel stuff in most CI tasks":
(https://github.com/bitcoin/bitcoin/pull/33824#issuecomment-3510658120)
> The test case producing the ubsan error should just be deleted in my opinion. It does not add anything in terms of coverage. How about:

I was thinking about permitting nullptr when size=0, or even just permitting nullptr for any size, when the data pointer is followed by a size?

I understand the nonnull check can in theory be useful in low-level environments where a nullptr-deref would not be detected as access violation, but there are probably more users using modern systems that may wa
...

💬 l0rinc commented on pull request "util: Allow `Assert` (et al.) in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#discussion_r2509751663)
Bumped into this again - if we remove the `const` from https://godbolt.org/z/fjc6be65M (`int& ref{Assert(a)};`) that seems to also fix the problem - why is that worse than introducing global suppression?