🤔 stickies-v reviewed a pull request: "log: Use `__func__` for -logsourcelocations"
(https://github.com/bitcoin/bitcoin/pull/34088#pullrequestreview-3587361950)
Concept ACK. I agree that for some functions (such as your example), the verbosity is a downside.

Have you considered just capping the `function_name()` to, say, 20 characters, when printing? (could even make it fixed length to make logs a bit more uniform for readability) I think that'd be about as helpful without having to introduce our own `SourceLocation`?

💬 maflcko commented on pull request "log: Use `__func__` for -logsourcelocations":
(https://github.com/bitcoin/bitcoin/pull/34088#issuecomment-3665069468)
> Have you considered just capping the `function_name()` to, say, 20 characters, when printing? (could even make it fixed length to make logs a bit more uniform for readability) I think that'd be about as helpful without having to introduce our own `SourceLocation`?

I don't think this works, because the return type is included in the function signature. I think printing something short but wrong is worse than printing the full thing.

Some examples:

```
std::variant<int, double, float,
...

⚠️ rkrux opened an issue: "GUI crashes in regtest"
(https://github.com/bitcoin-core/gui/issues/918)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

The GUI crashes in regtest upon loading. It did work once initially but has been crashing since, tried quite a few times.

### Expected behaviour

The GUI should not crash in regtest, just like it doesn't in main or test environments.

### Steps to reproduce

Following is the command I have used. Also, tried a few variants of the `-datadir, -walletdir` args by adding and removing `/regtest
...

🤔 stickies-v reviewed a pull request: "scripted-diff: [doc] Unify stale copyright headers"
(https://github.com/bitcoin/bitcoin/pull/34084#pullrequestreview-3587528249)
Approach ACK. I've moved to a dateless copyright notice for new code, personally. The scripted-diff keeping the initial publication dates seems like the approach with the least friction.

🤔 maflcko reviewed a pull request: "fuzz: Add fuzz target for block index tree and related validation events"
(https://github.com/bitcoin/bitcoin/pull/31533#pullrequestreview-3587318193)
lgtm, but I haven't reviewed the ABC part and I wonder why it is needed.

review ACK db2d39f642979f929261e5f1cd67f0c2f2ca045f 🍶

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU
...

💬 maflcko commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626777661)
nit: Could use `!bm.IsBlockPruned(*index)` for a stricter check?

💬 maflcko commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626714452)
nit: Could use `IsValid(BLOCK_VALID_TREE)` for a stricter check? Also, to avoid a (silent) conflict with https://github.com/bitcoin/bitcoin/pull/32950 ?

💬 maflcko commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626894721)
nit: Could use the stricter `!bm.IsBlockPruned(prune_block)` check?

💬 maflcko commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626748203)
nit: Could use `IsValid(BLOCK_VALID_TRANSACTIONS)` for a stricter check?

💬 maflcko commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626858638)
q: Maybe I am missing something, but what is the benefit of re-implementing this. `ConnectTip` is already `protected`, so it may be possible to mock it to inject invalidity?

💬 maflcko commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626711907)
nit: Could use `IsValid(BLOCK_VALID_TREE)` for a stricter check?

💬 maflcko commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2626902739)
same nit here for the asserts in the third commit.

👍 sedited approved a pull request: "fuzz: Add fuzz target for block index tree and related validation events"
(https://github.com/bitcoin/bitcoin/pull/31533#pullrequestreview-3587573179)
Re-ACK db2d39f642979f929261e5f1cd67f0c2f2ca045f

💬 rkrux commented on pull request "rpc, doc: clarify the response of listtransactions RPC":
(https://github.com/bitcoin/bitcoin/pull/32737#discussion_r2626938085)
Updated, good catch!

📝 maflcko opened a pull request: "move-only: MAX_BLOCK_TIME_GAP to src/qt"
(https://github.com/bitcoin-core/gui/pull/919)
`MAX_BLOCK_TIME_GAP` was used in some incorrect heuristics, which were removed in commit e30b6ea194fee3bb95a45e7b732a99566b88f1f5.

This leaves a single module in src/qt using the constant.

Instead of exposing it in a central kernel header, just move it to the single gui module that uses it.

👍 rkrux approved a pull request: "scripted-diff: [doc] Unify stale copyright headers"
(https://github.com/bitcoin/bitcoin/pull/34084#pullrequestreview-3587704436)
ACK fa5f29774872d18febc0df38831a6e45f3de69cc

Previously, I had noticed that the copyright dates were updated in the files that were affected in the pull requests by few contributors. Fixing all in one go appears to be a good idea to me.

I built and ran the functional tests, all work.
The Github UI is crashing due to the size of the diff, thus I checked out the code locally and opened the commit diff in a file to count the number of occurrences of "The Bitcoin Core developers" phrase that match
...

💬 fanquake commented on pull request "move-only: MAX_BLOCK_TIME_GAP to src/qt":
(https://github.com/bitcoin-core/gui/pull/919#issuecomment-3665316633)
cc @sedited

👍 hodlinator approved a pull request: "ci: Pin native tests on cross-builds to same commit"
(https://github.com/bitcoin/bitcoin/pull/34080#pullrequestreview-3587725439)
crACK faa8ee62f5c1606217fbe9eacdd504ec133920a4

I'm surprised the Windows runners are able to find the commit resulting from a merge between PR & master that happened on an Ubuntu runner. But looking at the CI logs that does appear to be the case.

Agree that it would be nice to use the frozen merge commit for all jobs, but if it makes re-running an all or nothing proposition (https://github.com/bitcoin/bitcoin/pull/34080#issuecomment-3660339716), I agree on avoiding it.

Since we didn't p
...

💬 hodlinator commented on pull request "ci: Pin native tests on cross-builds to same commit":
(https://github.com/bitcoin/bitcoin/pull/34080#discussion_r2627058913)
I'll punt on devising an example diff for now unless the ACKs get invalidated.

💬 hodlinator commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2627103854)
nit: Maybe this could be made more distinct from warnings by changing the name?

```suggestion
std::optional<std::conditional_t<std::is_same_v<ErrorType, void>, Monostate, ErrorType>> error{};
```

Would hopefully decrease misunderstandings like I had myself in https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-3573338179, since in that case it would be:

```C++
explicit operator bool() const { return !m_fail_data || !m_fail_data->error; }
```

🤔 stickies-v reviewed a pull request: "log: Use `__func__` for -logsourcelocations"
(https://github.com/bitcoin/bitcoin/pull/34088#pullrequestreview-3587759522)
> I don't think this works, because the return type is included in the function signature. I think printing something short but wrong is worse than printing the full thing.

Hmm, good point. And also, the function name is implementation-specific so manually trying to parse it is probably a bad idea. Approach ACK.