🤔 ryanofsky reviewed a pull request: "multiprocess: Add capnp wrapper for Chain interface"
(https://github.com/bitcoin/bitcoin/pull/29409#pullrequestreview-3571732845)
Thanks for the close review, and sorry for the delay responding and updating this. You found a lot of annoying discrepencies and a real bug in the `estimateSmartFee` `best_height` return value, which should all be fixed now.

<!-- begin push-22 -->
Rebased d9efd1e49d1df154970b6a60229eedde3ba7cffe -> 89cf624fe811d90d43f22d4519d859716d3ea958 ([`pr/ipc-chain.21`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-chain.21) -> [`pr/ipc-chain.22`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-
...

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

> I wonder how schema evolution is handled by Cap'n Proto - what happens if a field is deprecated and removed or new values are inserted, do I have to check all existing values to make sure I can find a new valid id?

Yes you do. You need to pick the next unused ID. I think it actually works pretty well in practice, especially if the id's are mostly in order, but it's also possible to include `# Next unused id: 42` com
...

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

> should we make the param `UInt32` like we did one line below for `ChainstateRole`

Nice find noticing the inconsistency. Both types should be safe because casts between signed and unsigned types are well-defined. I tend to prefer signed types for enums so if the enums use -1, -2, -3 values they will be displayed nicely. But unsigned types could be better for some enums, particularly bitmasks. I updated chainstate rol
...

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

> Could we add a simple explanation comment for these salts?

Not sure about this. Is there a comment you would suggest? I feel like ids are built into the syntax of Cap'n Proto and ubiquitous. If you aren't familiar with Cap'n Proto, I think a comment here would probably just distract you from more semantically relevant parts of the file and leave you confused. Commenting on IDs in capnproto seems like commenting on #
...

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

> May be deliberate, but the parameters in [...] are 32 bit

Good catch. Updated these. Using a bigger type was harmless but not intentional. (It is a compiler error if you use a smaller type.)

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