💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2614251707)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2559791820

> same, should likely be

Thanks, updated. Agree signed type is a little nicer here (though both types should be safe)

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2614225261)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2553243415

> super-nit: I assume the copyright headers are similar to other code in the repo:

Nice catch, fixed this up

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2614245633)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2559765741

> The original param name is `delta_seconds`

Thanks renamed to deltaSeconds for consistency. In general parameter names don't matter very much but it is good to be consistent.

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2614254876)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2559795042

> Is this an accurate mapping of `std::optional<UniValue>`

Yes, it is. The UniValue fields is serialized as JSON text here. And Cap'n Proto allows text fields to be null so `std::nullopt` value is just represented by an null text field.

💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2614248413)
re: https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2559789059

> we might want to ment why this one doesn't have a corresponding proxy:

Good suggestion, added a version of this comment

📝 maflcko opened a pull request: " refactor: Use NodeClock::time_point for m_addr_token_timestamp "
(https://github.com/bitcoin/bitcoin/pull/34059)
It is a bit confusing to have some code use the deprecated `GetTime`, which returns a duration and not a time point, and other code to use `NodeClock` time points.

Fix one place `m_addr_token_timestamp` to use `NodeClock::time_point`.

Also:

* Extract a `ProcessAddrs` helper, similar to the other `Process*()` helpers, to cut down the `ProcessMessage` with a massive scope.
* Rename the confusing `current_a_time` to `now_seconds`. (The `a` in this context refers to the removed "adjusted"
...

💬 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!