💬 dergoegge commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1287125133)
Done

💬 dergoegge commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1287125465)
Done

💬 dergoegge commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1287125880)
Done

💬 instagibbs commented on pull request "p2p: ensure mapBlockSource is removed from in ProcessBlock":
(https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1669693642)
Since I think this behavior is fairly long-running, I think I might try to recap the current logic:

Every time a compact block is reconstructed, or full block directly given, we put an entry in a map for one of two reasons:
1) reward if they're a good high-bandwidth peer candidates because they gave us a new most-PoW block
2) punish if they gave us a full block and it has any fundamental issue, or if they handed us a compact block they should have known was faulty(e.g., invalid header)

I
...

💬 MarcoFalke commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1287209576)
Apart from compare, assignment isn't type safe either. For example:

```cpp
uint256 b = Txid{};
Wtxid a {b};
```

compiles.

So my preference would be to add a comment that this is allowed, and remove this code that tries to imply the opposite and attempts to trick reviewers.

Alternatively it could be disallowed properly.

👍 TheCharlatan approved a pull request: "refactor: Remove unused includes from wallet.cpp"
(https://github.com/bitcoin/bitcoin/pull/28200#pullrequestreview-1567463775)
ACK fac3e2fa6adf9cb034ea5ee28c572b25cc7e3c2d

Re https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1669483448

> No. The boost include is still there (via src/wallet/scriptpubkeyman.h:#include <boost/signals2/signal.hpp>

Yeah, totally. Should have checked this before trusting the iwyu suggestion.

💬 Crypt-iQ commented on pull request "p2p: ensure mapBlockSource is removed from in ProcessBlock":
(https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1669750122)
Yup that makes sense

💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1287224406)
Could you say that in the test then? This really throws me off as a reader!

💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#issuecomment-1669753053)
ACK https://github.com/bitcoin/bitcoin/pull/28199/commits/abe8536192c9f2cd6ba9d0e083f23dec4d20841f

https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1283271742 would be nice to be slightly cleaned up for future readers

💬 glozow commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#issuecomment-1669788993)
thanks for the concept acks @dergoegge @jamesob @brunoerg @Empact @jonatack, would appreciate a review of test too :pray:

💬 fjahr commented on pull request "Add ASM optimizations for MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/19181#issuecomment-1669793604)
> There hasn't been much activity lately. What is the status here?
>
> [Finding reviewers](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#finding-reviewers) may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

Still relevant... How good is your ASM, @DrahtBot ? 😁

💬 fanquake commented on pull request "guix: use GCC 12.3.0 to build releases":
(https://github.com/bitcoin/bitcoin/pull/27897#issuecomment-1669797798)
Upstreamed a patch to (re-)add `powerpc64-linux` as a platform definition in Guix: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=65151. Using a self-compiled Guix with this patch, fixes building our cross-compilation toolchain for `powerpc64-linux-gnu`, and means we'd just drop 26ee1fc5998cce6808f022fd1a4e1764115c2d57 from this PR.

Also pushed a patch to update mingw-w64 to `11.0.1`: https://lists.gnu.org/archive/html/guix-patches/2023-08/msg00299.html.

💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1669800569)
Maybe point out in the description how the first commit 8283ca76e4feb37d3514fbd8dc491b8c4965dff1 relates to BIP 324, which doesn't use the ecdh module, but ElligatorSwift instead.

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1287286613)
Have thought more about this and I'm not so convinced anymore.

The connection manager class lives on a lower level, and manages only a fraction of the node's information. And this field is strictly linked to the node's overall status. Right now, `HasDesirableServiceFlags()` requires to know the chain sync status.
So, by placing it inside `CConMan`, we add another level of indirection. Because, most of the time, the peer manager class will call to the `CConMan` setter and subsequently call th
...

👍 TheCharlatan approved a pull request: "refactor: Remove unused boost signals2 from torcontrol"
(https://github.com/bitcoin/bitcoin/pull/28240#pullrequestreview-1567496979)
ACK fa32af22b323e7c58b6b20af6517f4795a72cdc5

The `async_handler` was unused since its introduction, so I don't think it's worth it to keep it around.

💬 TheCharlatan commented on pull request "refactor: Remove unused boost signals2 from torcontrol":
(https://github.com/bitcoin/bitcoin/pull/28240#discussion_r1287241422)
Style nit: These pragmas could be indented like they are here: https://github.com/bitcoin/bitcoin/blob/master/src/util/strencodings.h#L20 , or did you omit any automatic formatting to maintain order?

💬 TheCharlatan commented on pull request "refactor: Remove unused boost signals2 from torcontrol":
(https://github.com/bitcoin/bitcoin/pull/28240#discussion_r1287296853)
Maybe change this to "These messages could be used to dispatch async notifications to an async handler"?

💬 MarcoFalke commented on pull request "refactor: Remove unused boost signals2 from torcontrol":
(https://github.com/bitcoin/bitcoin/pull/28240#discussion_r1287321938)
I wasn't sure if the includes must come in a specific order. I've pushed a new commit to re-order them, which can be easily reverted, if there are issues.

💬 MarcoFalke commented on pull request "Avoid lock order inversion in `Chainstate::ConnectTip` function":
(https://github.com/bitcoin/bitcoin/pull/27684#issuecomment-1669917794)
Maybe `m_tx_inventory_mutex` can be released before `pool.cs` to break that link instead?

💬 theStack commented on pull request "bench: add readblock benchmark":
(https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1287370102)
Okay, that makes sense. My blind guess was that clang-tidy (or any other similar tool) would only look at the passed expression and not care about the effect of the assert as a whole. Seems like even the passed expression is not side-effect free at several places in the code, e.g.
https://github.com/bitcoin/bitcoin/blob/b565485c24c0feacae559a7f6f7b83d7516ca58d/src/init.cpp#L1428

so it indeed might be better to remove recommendations that we don't even follow ourselves.

(Also, I sadly can'
...