🤔 ryanofsky reviewed a pull request: "refactor: Add util::Result failure types and ability to merge result values"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-3571956306)
Thanks for the reviews! The feedback's been very helpful and led to a few updates here.

<!-- begin push-70 -->
Rebased f0aff63b5ad51566e626d5b24eee08eb81df54a1 -> 074cb32216824cc05433ec44cd3d41e0cc5a14cd ([`pr/bresult2.69`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.69) -> [`pr/bresult2.70`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.70), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult2.69-rebase..pr/bresult2.70))<!-- end --> due to conflict with #3
...

💬 ryanofsky commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2614478713)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2594047330

> Would anything be gained from making the template parameter an r-value? (Earlier commits do explicit `std::move()`s, justifying the function's name, but that justification appears to have dissipated).

There isn't a great way of forcing this to be an rvalue, since just replacing `&` && would make it a universal reference and make using the deduced `SrcResult` type more awkward. I also don't think it would be too hel
...

💬 ryanofsky commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2614527855)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2599090229

> nit Q in [3c535e2](https://github.com/bitcoin/bitcoin/commit/3c535e299efbf445ccd33c633ed455399d9785cd) "wallet: fix clang-tidy warning performance-no-automatic-move": I got the impression our expectation is that all commits should pass CI. So I would expect this change to come before or in the same commit that would cause CI failure. Is that only valid for the HEAD commit when it comes to commits that resolve clang-tid
...

💬 ryanofsky commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2614537274)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2608405722

> I'm guessing the reason for the existence of a separate `SuccessHolder` type is in order to specialize away a minimal subset of functionality in `SuccessHolder<void, ...>`. If that's part of the reason, it could be admitted in the comment block for the main `SuccessHolder` template?

Yes exactly. Added a comment mentioning the void type to make this clearer.

💬 ryanofsky commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2614454078)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2593959586

> Isn't it closer to: `tuple<optional<SuccessType>, unique_ptr<tuple<FailureType, MessagesType>>>`

The current comment here says "Logically, a result object is equivalent to `tuple<variant<SuccessType, ErrorType>, MessagesType>`".

This is just trying to get across the idea that a `Result` holds a success value or a failure value, plus some messages.

IMO, adding unique_ptr to this description would make it more
...

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3646978232)
Many thanks!

📝 marcofleon opened a pull request: "fuzz: Fix bugs in `clusterlin_postlinearize_tree` target"
(https://github.com/bitcoin/bitcoin/pull/34061)
Addresses two issues in the `clusterlin_postlinearize_tree` target:

1. The loop iteration while creating tree dependency graphs was incorrect.
2. We were accidentally passing in `post_linearization` to `PostLinearize` instead of the copy we just made, resulting in an ineffective check.

💬 fanquake commented on pull request "fuzz: Fix bugs in `clusterlin_postlinearize_tree` target":
(https://github.com/bitcoin/bitcoin/pull/34061#issuecomment-3647001408)
cc @sipa @instagibbs @glozow

💬 maflcko commented on pull request "net: Waste less time in socket handling":
(https://github.com/bitcoin/bitcoin/pull/34025#discussion_r2614627662)
> because time_point doesn't support being an atomic.

Duration does not either. I think it is fine to type `.load()` or `.store()`, where needed.

I guess there is no rush here, and this can be done, when all other places (non-atomic) are switched.

💬 sipa commented on pull request "fuzz: Fix bugs in `clusterlin_postlinearize_tree` target":
(https://github.com/bitcoin/bitcoin/pull/34061#issuecomment-3647032758)
ACK a70a14a3f4f4d66025302c00eb4ff8108835cc5d

⚠️ stickies-v opened an issue: "RFC: separate kernel logging infrastructure"
(https://github.com/bitcoin/bitcoin/issues/34062)
tl;dr: kernel logging is cumbersome. I propose we separate it so we can enable structured logging and a much simplified logging interface without overcomplicating node logging.

## Motivation

The `bitcoinkernel` library (#27587) [exposes](https://github.com/bitcoin/bitcoin/blob/b26762bdcb941096ccc6f53c3fb5a7786ad735e7/src/kernel/bitcoinkernel.h#L698-L778) functionality to interface with the kernel logging. This includes registering callbacks for log statements, level/category filtering, string
...

📝 billymcbip converted_to_draft a pull request: "test: Improve STRICTENC/DERSIG unit tests in script_tests.json"
(https://github.com/bitcoin/bitcoin/pull/33973)
Use the `DERSIG` flag instead of the `STRICTENC` flag for signature encoding tests. `STRICTENC` rules are a superset of `DERSIG` rules, and only `DERSIG` is a consensus flag. Also improve test descriptions and add two error cases to the `CHECKMULTISIG NOT` section to make it easier to understand how these tests work.

Tests pass on `6224b272ac291afd3af89f91e1deaccf233718bf`:
```
cmake -B build -DENABLE_WALLET=OFF
cmake --build build --parallel 8
ctest --test-dir build --parallel 8
```
``
...

💬 ryanofsky commented on issue "RFC: separate kernel logging infrastructure":
(https://github.com/bitcoin/bitcoin/issues/34062#issuecomment-3647259207)
Nice work. The new interface proposed here and implemented in your branch seem much better than the current interface, and well thought out. I feel like we could pretty easily wire it into the existing Logger class without introducing a new class and new macros though, and without needing to change validation & util code.

I wonder if there may be difficulties doing that I'm not aware of, or if it just seemed easier to start with a clean state in the current branch.

I do think it would preferab
...

📝 rustaceanrob opened a pull request: "Make `transaction_indentifier` hex string constructor evaluated at comptime"
(https://github.com/bitcoin/bitcoin/pull/34063)
Suggested by @l0rinc as a comment in #34004

There are tests that utilize `FromHex` that will only fail during runtime if malformed. Adds a compile time constructor that can be caught by LSPs

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-3647326460)
I have rewritten `SpanningForestState::UpdateChunk` to no longer traverse the chunk by walking dependencies, but by looping over all dependencies of the chunk directly, and using the `top_setinfo` to figure out which ones to update. It's a few percent faster, simpler, and additionally allowed dropping the tracking of parent dependencies of a transaction entirely.

👍 instagibbs approved a pull request: "fuzz: Fix bugs in `clusterlin_postlinearize_tree` target"
(https://github.com/bitcoin/bitcoin/pull/34061#pullrequestreview-3572724599)
ACK a70a14a3f4f4d66025302c00eb4ff8108835cc5d

💬 davidgumberg commented on pull request "doc: guix: Troubleshooting zdiff3 issue and uninstalling.":
(https://github.com/bitcoin/bitcoin/pull/32442#discussion_r2614933849)
The mistake I made was that `or running...` should be inside of the parentheses. The items in the parentheses don't "need" to be parallel with "Uninstall" even if you respect the parallel structure device as a rule (which it isn't.)

I moved the guix install script instructions inside the parentheses.

💬 andrewtoth commented on pull request "coins: remove SetFresh method from CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/33018#issuecomment-3647464529)
@sedited @l0rinc thank you both for your reviews! I rebased and updated with your suggestions.

🤔 l0rinc requested changes to a pull request: "test: Add test on skip heights in CBlockIndex"
(https://github.com/bitcoin/bitcoin/pull/33661#pullrequestreview-3572005492)
I might have gone a bit overboard, but I have tried explaining every change that I suggested in context.

For convenience, here's the full local diff I did during review (this is the latest state, some of the suggestions may reflect a previous one):
```C++
BOOST_AUTO_TEST_CASE(get_skip_height_test)
{
// Even heights: clear the lowest set bit.
BOOST_CHECK_EQUAL(GetSkipHeight(0b00010010),
0b00010000);
BOOST_CHECK_EQUAL(GetSkipHeight(0b00100010),

...

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614369333)
Could unify these to `BOOST_REQUIRE` instead of `assert`

💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2614388593)
can you use brace init consistently to avoid narrowing? And corecheck suggests const reference - though we can likely inline it instead...