💬 sipa commented on pull request "test: add some txgraph tests":
(https://github.com/bitcoin/bitcoin/pull/34057#discussion_r2614390979)
I think you can test more by repeating these getters/checks in between transaction additions.

E.g.:
* Add tx A.
* Check `GetTransactionCount`, `GetWorstMainChunk`, `GetIndividualFeerate`.
* Add tx B, and the B->A dependency.
* Check `GetTransactionCount`, `GetWorstMainChunk`, `GetIndividualFeerate` again.
* Add tx C, and the C->B dependency.
* Check `GetTransactionCount`, `GetWorstMainChunk`, `GetIndividualFeerate` again.
* Add tx D, and the D->C dependency.
* Check `GetTransactionCount`, `GetW
...

💬 sipa commented on pull request "test: add some txgraph tests":
(https://github.com/bitcoin/bitcoin/pull/34057#issuecomment-3646694648)
Concept ACK.

Nice start, we can certainly use more unit tests in this area, as significant parts of `txgraph` are only tested through fuzz tests, If you want to add more, looking for unkilled mutants found by @brunoerg is a good idea.

💬 sipa commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3646732825)
@optout21 Cool, approach ACK. You'll need to make sure both commits compile, though.

💬 HowHsu commented on pull request "test: add some txgraph tests":
(https://github.com/bitcoin/bitcoin/pull/34057#discussion_r2614459931)
True, thanks, Sipa.

💬 rustaceanrob commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2614495045)
Side-effect of a past commit. Fixed.

💬 rustaceanrob commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2614499974)
Changed to void cast for style consistency

💬 rustaceanrob commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2614523808)
I have used this exact suggestion and added you as a commit co-author. Indeed, the salt is the primary defense against grinding here. I was not sure and discussed out of band, but I think siphash should be faster. As far as the "balancing" concept, this avoids having to invert a field element (subtracting an element is addition of the inverse), so I think this is strictly an improvement. Thanks for the suggestion. I was not aware of `arith_uint256` prior to your review.

💬 rustaceanrob commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2614526263)
Changed the "accelerated" log to "SwiftSync" for consistency

📝 stratospher opened a pull request: "test: fix race condition in p2p_v2_misbehaving.py peerid assertion"
(https://github.com/bitcoin/bitcoin/pull/34060)
fixes https://github.com/bitcoin/bitcoin/issues/34035

remove the hard-coded peer id from the debug message in `p2p_v2_misbehaving.py`.

asyncio's non-deterministic task scheduling might cause [peer2](https://github.com/bitcoin/bitcoin/blob/938d7aacabd0bb3784bb3e529b1ed06bb2891864/test/functional/p2p_v2_misbehaving.py#L181)'s connection to happen before [peer1](https://github.com/bitcoin/bitcoin/blob/938d7aacabd0bb3784bb3e529b1ed06bb2891864/test/functional/p2p_v2_misbehaving.py#L179)'s. sinc
...

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